Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable ReadAsync_CancelPendingValueTask_ThrowsCancellationException for Http1 #72854

Merged
merged 1 commit into from
Jul 27, 2022

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Jul 26, 2022

Contributes to #72586.

I checked test result DB and it seems that only the variant in Http1CloseResponseStreamConformanceTests hangs.

@ghost
Copy link

ghost commented Jul 26, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Contributes to #72586.

I checked test result DB and it seems that only the variant in Http1CloseResponseStreamConformanceTests hangs.

Author: rzikm
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@rzikm rzikm requested a review from stephentoub July 26, 2022 12:37
@stephentoub
Copy link
Member

I checked test result DB and it seems that only the variant in Http1CloseResponseStreamConformanceTests hangs.

It's surprising that on the raw version the task case would hang but the valuetask one wouldn't.

@wfurt
Copy link
Member

wfurt commented Jul 26, 2022

I see this locally:

   System.Net.Http.Functional.Tests: [Long Running Test] 'System.Net.Http.Functional.Tests.Http1CloseResponseStreamConformanceTests.ReadAsync_CancelPendingValueTask_ThrowsCancellationException', Elapsed: 00:02:27
   System.Net.Http.Functional.Tests: [Long Running Test] 'System.Net.Http.Functional.Tests.Http1CloseResponseStreamConformanceTests.ReadAsync_CancelPendingValueTask_ThrowsCancellationException', Elapsed: 00:04:27

The problem is not in the Task IMHO. I see the HttpConnection Disposed (finalized) but the underlying _stream is not so we get stuck in _stream.ReadAsync

Copy link
Member

@CarnaViire CarnaViire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get the disabling in so it wouldn't hurt others, and investigate later

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants