Skip to content

[HTTP/3] Fix NullReferenceException on Timeout when streaming request body#59204

Merged
CarnaViire merged 5 commits intodotnet:mainfrom
CarnaViire:fix-http3-timeout-nre
Sep 21, 2021
Merged

[HTTP/3] Fix NullReferenceException on Timeout when streaming request body#59204
CarnaViire merged 5 commits intodotnet:mainfrom
CarnaViire:fix-http3-timeout-nre

Conversation

@CarnaViire
Copy link
Member

Brings the logic for canceling streaming request body from HTTP/2 to HTTP/3.

The problem was that cancellation token was not applied to SendContentAsync, which resulted in Http3WriteStream not being disposed properly.

Fixes #57619

@ghost
Copy link

ghost commented Sep 16, 2021

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

Issue Details

Brings the logic for canceling streaming request body from HTTP/2 to HTTP/3.

The problem was that cancellation token was not applied to SendContentAsync, which resulted in Http3WriteStream not being disposed properly.

Fixes #57619

Author: CarnaViire
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Some questions about the test, but looks good, thanks!

// Send headers
await requestStream.FlushAsync();

await requestStream.WriteAsync(message).AsTask().WaitAsync(TimeSpan.FromSeconds(10));
Copy link
Member

Choose a reason for hiding this comment

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

How do we make sure that the 0.3 seconds timeout of HttpClient doesn't fire before this WriteAsync?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. I don't think we can get to 100% sure with timeouts. But I will rework the test and increase the timeout. Will add a second test for an external cancellation token, which will induce the same logic but will also have consistent behavior.

@CarnaViire
Copy link
Member Author

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

Thanks!

@CarnaViire
Copy link
Member Author

Outerloop test failures are unrelated. Most of them are System.Net.WebSockets.Client.Tests.CancelTest.ConnectAsync_Cancel_ThrowsCancellationException -- will create an issue.

@ManickaP
Copy link
Member

ConnectAsync_Cancel_ThrowsCancellationException

#58355 ???

@CarnaViire CarnaViire merged commit d8acd7e into dotnet:main Sep 21, 2021
@CarnaViire CarnaViire deleted the fix-http3-timeout-nre branch September 21, 2021 19:41
@karelz karelz added this to the 7.0.0 milestone Sep 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
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.

[HTTP/3] Http3RequestStream.BufferFrameEnvelope null reference error

3 participants