[HTTP/3] Abort response stream on dispose if content not finished #57156

ManickaP · 2021-08-10T18:14:03Z

Sends abort read/write if H/3 stream is disposed before respective contents are finished.

Fixes #56129

cc: @CarnaViire

ghost · 2021-08-10T18:14:12Z

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

Issue Details

Sends abort read/write if H/3 stream is disposed before respective contents are finished.

Fixes #56129

cc: @CarnaViire

Author:	ManickaP
Assignees:	-
Labels:	`area-System.Net.Http`
Milestone:	-

ManickaP · 2021-08-10T18:17:13Z

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

@@ -1187,6 +1192,25 @@ private async ValueTask<bool> ReadNextDataFrameAsync(HttpResponseMessage respons
        public void Trace(string message, [CallerMemberName] string? memberName = null) =>
            _connection.Trace(StreamId, message, memberName);

+        private void AbortStream()
+        {
+            // ToDo : locking??? we don't have any in H/3 stream, we don't have a sync root or anything.


I was looking at H2Stream corresponding method:

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

Line 1292 in ae0e5e8

private void CloseResponseBody()

And the main difference is that we lack any locking mechanism in H3Stream, which kind of worries me.

TL;DR: if we check the flag and it's not EOF and on the next line when we call Abort the flag is changed already - it is not much better than just calling Abort without any checks, is it?

We actually had a bit of a discussion on using flags to symbolize EOFs in my first attempt on cancellation PR #54334 (comment)

I guess we might want to put some proper locking here. as HTTP/2 has.. or just always call abort ant let quic stream/msquic handle the race? If we imagine Dispose being called synchronously after finishing reading the response -- I imagine graceful shutdown should be complete inside QuicStream by that time. So Abort will be just a no-op. But I may be wrong. Always calling Abort may lead to unexpected behavior in streaming scenarios 🤔 but I feel like this check without any locks will not be much better that always aborting.

@geoffkizer what do you think?

I'd say that checking is still better then not checking at all. There's a potential for race, but in a happy path (dispose the response after you've finished the reading), this should work and not send any aborts.

Anyway, I was surprised that there's no locking mechanism at all. So my question is, do we use any other technique I'm missing? Or is this an oversight that just works because we're testing the right thing? Or are we just not thread-safe on the level of a response message thus we don't care here?

ManickaP · 2021-08-10T18:18:57Z

@CarnaViire add #56683: could you point me at the test which might hit that problem? I'll try to repro it locally and then see if I can fix it as well.

CarnaViire · 2021-08-10T20:50:06Z

add #56683: could you point me at the test which might hit that problem? I'll try to repro it locally and then see if I can fix it as well.

The test is

runtime/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs

Line 546 in acb6b9b

    
           public async Task ResponseCancellation_BothCancellationTokenAndDispose_Success()

Namely here you can remove ODE from expected exceptions

runtime/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http3.cs

Line 609 in acb6b9b

if (ex is not OperationCanceledException and not ObjectDisposedException)

The ThrowIfDisposed checks I proposed to remove are

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

Lines 494 to 496 in acb6b9b

    
           internal override void AbortRead(long errorCode) 
        
           { 
        
               ThrowIfDisposed();

runtime/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

Lines 522 to 524 in acb6b9b

    
           internal override void AbortWrite(long errorCode) 
        
           { 
        
               ThrowIfDisposed();

But I thought you touched MsQuicStream, but you did not. So I can do the change myself in a separate PR tomorrow.

ManickaP · 2021-08-11T07:50:00Z

But I thought you touched MsQuicStream, but you did not. So I can do the change myself in a separate PR tomorrow.

Ok then, do it. I won't try to cram it in this PR.

ManickaP · 2021-08-12T18:49:33Z

Newly added test is failing in CI, need to investigate, hold off reviews for now please.

…ntents are finsihed

…ding state within Send* methods.

ManickaP · 2021-08-18T17:31:50Z

This is ready for review @CarnaViire @geoffkizer

CarnaViire · 2021-08-19T09:21:04Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs

                {
                    throw GetConnectionAbortedException(_state);
                }
+
+                // Change the state in the same lock where we check for final states to prevent coming back from Aborted/ConnectionClosed.
+                Debug.Assert(_state.SendState != SendState.Pending);


Nit: should we do an actual throw here? for Pending and Finished states. Both of them would mean someone tries to do a send with an another concurrent send in flight -- that we don't support, and Send state machine, fragile as it is, may blow up because of it. Concurrent reads do throw now. But we can choose not to do it now, your choice.

I also feel bad that we don't have any final state after we send FIN flag (like ReadsCompleted) so that means we also can't prevent user calling Send after passing the FIN flag which I would guess will result in msquic exception (INVALID_STATE most possibly). But that's out of scope of this PR, just me ranting. I guess that's what #55817 is for in the end.

I share the same grievances and worries as you. I chose to just shuffle the code around to fix the problem on hand instead of doing the ultimate right thing, which I think we should leave to #55817. We've lived with just the assert until now, this change shouldn't make any worse (hopefully 😅 )

CarnaViire

LGTM. I commented my observations but we may not act on it now.

ManickaP · 2021-08-19T16:45:52Z

/backport to release/6.0

github-actions · 2021-08-19T16:46:03Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1147767776

ViktorHofer · 2021-08-19T16:54:20Z

port to release/6.0

@ManickaP do you want this to go into RC1 or RC2? The branch that you picked is RC2.

ManickaP · 2021-08-19T17:14:38Z

@ViktorHofer RC2

karelz · 2021-08-24T06:24:32Z

/backport to release/6.0-rc1

github-actions · 2021-08-24T06:24:46Z

Started backporting to release/6.0-rc1: https://github.com/dotnet/runtime/actions/runs/1161485673

ManickaP requested review from CarnaViire, geoffkizer and wfurt August 10, 2021 18:14

dotnet-issue-labeler bot added the area-System.Net.Http label Aug 10, 2021

ManickaP commented Aug 10, 2021

View reviewed changes

ManickaP mentioned this pull request Aug 13, 2021

[QUIC] Abort after Dispose is no-op #57318

Merged

jaredpar mentioned this pull request Aug 17, 2021

Feed unreliability affecting CI #55449

Closed

ManickaP and others added 2 commits August 17, 2021 16:39

Sends abort read/write if H/3 stream is disposed before respective co…

febcfa8

…ntents are finsihed

Minor tweaks in abort conditions

2cdc73e

ManickaP force-pushed the mapichov/56129_send_abort_dispose branch 2 times, most recently from 03537c9 to af6a189 Compare August 17, 2021 17:07

runfoapp bot mentioned this pull request Aug 17, 2021

RunContinueWithStressTestsNoState timing out in CI #2271

Closed

ManickaP force-pushed the mapichov/56129_send_abort_dispose branch from af6a189 to 60949a3 Compare August 18, 2021 12:51

Prevent reverting SendState from Aborted/ConnectionClosed back to sen…

3ad021f

…ding state within Send* methods.

ManickaP force-pushed the mapichov/56129_send_abort_dispose branch from 60949a3 to 3ad021f Compare August 18, 2021 12:54

CarnaViire reviewed Aug 19, 2021

View reviewed changes

CarnaViire approved these changes Aug 19, 2021

View reviewed changes

ManickaP merged commit b75e55b into dotnet:main Aug 19, 2021

ManickaP deleted the mapichov/56129_send_abort_dispose branch August 19, 2021 10:44

github-actions bot mentioned this pull request Aug 19, 2021

[release/6.0] [HTTP/3] Abort response stream on dispose if content not finished #57741

Closed

karelz added this to the 7.0.0 milestone Aug 20, 2021

github-actions bot mentioned this pull request Aug 24, 2021

[release/6.0-rc1] [HTTP/3] Abort response stream on dispose if content not finished #57999

Merged

CarnaViire mentioned this pull request Aug 24, 2021

[HTTP/3] Enable IncompleteResponseStream_ResponseDropped_CancelsRequestToServer #58024

Merged

karelz mentioned this pull request Aug 24, 2021

[HTTP/3] Cancellation of response download not reported correctly on server #56129

Closed

ghost locked as resolved and limited conversation to collaborators Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HTTP/3] Abort response stream on dispose if content not finished #57156

[HTTP/3] Abort response stream on dispose if content not finished #57156

ManickaP commented Aug 10, 2021

ghost commented Aug 10, 2021

ManickaP Aug 10, 2021 •

edited

Loading

CarnaViire Aug 10, 2021

ManickaP Aug 11, 2021

ManickaP commented Aug 10, 2021 •

edited

Loading

CarnaViire commented Aug 10, 2021

ManickaP commented Aug 11, 2021

ManickaP commented Aug 12, 2021

ManickaP commented Aug 18, 2021

CarnaViire Aug 19, 2021

ManickaP Aug 19, 2021

CarnaViire left a comment

ManickaP commented Aug 19, 2021

github-actions bot commented Aug 19, 2021

ViktorHofer commented Aug 19, 2021

ManickaP commented Aug 19, 2021

karelz commented Aug 24, 2021

github-actions bot commented Aug 24, 2021

[HTTP/3] Abort response stream on dispose if content not finished #57156

[HTTP/3] Abort response stream on dispose if content not finished #57156

Conversation

ManickaP commented Aug 10, 2021

ghost commented Aug 10, 2021

ManickaP Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

CarnaViire Aug 10, 2021

Choose a reason for hiding this comment

ManickaP Aug 11, 2021

Choose a reason for hiding this comment

ManickaP commented Aug 10, 2021 • edited Loading

CarnaViire commented Aug 10, 2021

ManickaP commented Aug 11, 2021

ManickaP commented Aug 12, 2021

ManickaP commented Aug 18, 2021

CarnaViire Aug 19, 2021

Choose a reason for hiding this comment

ManickaP Aug 19, 2021

Choose a reason for hiding this comment

CarnaViire left a comment

Choose a reason for hiding this comment

ManickaP commented Aug 19, 2021

github-actions bot commented Aug 19, 2021

ViktorHofer commented Aug 19, 2021

ManickaP commented Aug 19, 2021

karelz commented Aug 24, 2021

github-actions bot commented Aug 24, 2021

ManickaP Aug 10, 2021 •

edited

Loading

ManickaP commented Aug 10, 2021 •

edited

Loading