Skip to content

Fix HTTP/3 loopback server WaitForCancellationAsync and aborting on finalization#60395

Merged
CarnaViire merged 1 commit intodotnet:mainfrom
CarnaViire:h3-fix-wait-for-cancellation
Oct 14, 2021
Merged

Fix HTTP/3 loopback server WaitForCancellationAsync and aborting on finalization#60395
CarnaViire merged 1 commit intodotnet:mainfrom
CarnaViire:h3-fix-wait-for-cancellation

Conversation

@CarnaViire
Copy link
Member

@CarnaViire CarnaViire commented Oct 14, 2021 •
edited
Loading

Fixed WaitForCancellationAsync to take into account both read and write cancellations. (Partially?) fixed not aborting on finalization.

I have additional questions on disposing being part of Http3ReadStream and not Http3WriteStream, and also the fact that _connection.RemoveStream is called only in Http3RequestStream.Dispose and in Http3ReadStream -- if Http3ReadStream was never created and Http3RequestStream was left for finalization it seems that Http3RequestStream instance will live until it's connection is disposed/finalized? I decided it will be better to investigate as a separate PR.
UPD: answered below

Fixes #58234
Contributes to Fixes #60141

@ghost ghost added the area-System.Net.Http label Oct 14, 2021
@ghost
Copy link

ghost commented Oct 14, 2021

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

Issue Details

Fixed WaitForCancellationAsync to take into account both read and write cancellations. (Partially?) fixed not aborting on finalization.

I have additional questions on disposing being part of Http3ReadStream and not Http3WriteStream, and also the fact that _connection.RemoveStream is called only in Http3RequestStream.Dispose and in Http3ReadStream -- if Http3ReadStream was never created and Http3RequestStream was left for finalization it seems that Http3RequestStream instance will live until it's connection is disposed/finalized? I decided it will be better to investigate as a separate PR.

Fixes #58234
Contributes to #60141

Author: CarnaViire
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@ManickaP
Copy link
Member

ManickaP commented Oct 14, 2021

if Http3ReadStream was never created and Http3RequestStream was left for finalization it seems that Http3RequestStream instance will live until it's connection is disposed/finalized?

No:

runtime/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs

Lines 311 to 314 in f77e586

if (disposeSelf)
{
await DisposeAsync().ConfigureAwait(false);
}

@CarnaViire
Copy link
Member Author

CarnaViire commented Oct 14, 2021

After thinking through the disposal behavior and Http3ReadStream vs Http3WriteStream -- it is sufficient to have disposal/abortion in Http3ReadStream.

  1. No streaming from server -- even if there is streaming from client and Http3WriteStream is created, disposal/abortion happens in the end of SendAsync as shown above in @ManickaP's comment

  2. Streaming from server -- Http3ReadStream is created. In case it is not duplex, Http3WriteStream is either finished already or not created at all. In case it is duplex -- disposing HttpResponse will lead to Http3ReadStream and respective proper disposal/abortion of Http3RequestStream -- it is a recognized pattern for cancelling so users should not expect Http3WriteStream (streaming from custom HttpContent) to work after that. Throwing away HttpResponse while holding on Http3WriteStream (streaming from custom HttpContent) also doesn't seem as a valid scenario.

With that, I believe this PR should fully fix #60141

@CarnaViire CarnaViire merged commit edf638f into dotnet:main Oct 14, 2021
@CarnaViire CarnaViire deleted the h3-fix-wait-for-cancellation branch October 14, 2021 14:28
@karelz karelz added this to the 7.0.0 milestone Nov 4, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ManickaP ManickaP ManickaP approved these changes

@geoffkizer geoffkizer Awaiting requested review from geoffkizer

@wfurt wfurt Awaiting requested review from wfurt

+1 more reviewer

@mkmibrahim mkmibrahim mkmibrahim approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Http

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

[HTTP/3] Http3RequestStream is not properly aborted on finalization [HTTP3] Loopback server WaitForCancellationAsync logic is wrong

4 participants

@CarnaViire @ManickaP @mkmibrahim @karelz