Send RST_STREAM instead EOF when sending request body is cancelled #58548

alnikola · 2021-09-02T11:16:32Z

ghost · 2021-09-02T11:16:40Z

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

Issue Details

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

alnikola · 2021-09-02T11:19:54Z

I need to say that I still have a concern regarding how Kestrel will react to such RST_STREAM with CANCEL. We expect that it will continue sending the response body (if any), but we need to verify it.

… section 5.1

alnikola · 2021-09-06T12:39:58Z

@geoffkizer I discussed this behavior with the Kestrel team and we decided that when the request body is skipped the RST_STREAM with CANCEL should be sent after the response body receival has completed. This is required by the HTTP/2 stream states transition rules as described RFC 7540 section 5.1.

cc: @Tratcher

If this state is reached as a result of sending a RST_STREAM
frame, the peer that receives the RST_STREAM might have already
sent -- or enqueued for sending -- frames on the stream that
cannot be withdrawn. An endpoint MUST ignore frames that it
receives on closed streams after it has sent a RST_STREAM frame.
An endpoint MAY choose to limit the period over which it ignores
frames and treat frames that arrive after this time as being in
error.

alnikola · 2021-09-06T14:04:24Z

/azp run runtime-libraries stress-http

azure-pipelines · 2021-09-06T14:04:37Z

Azure Pipelines successfully started running 1 pipeline(s).

ManickaP

Looks good, just one question. Thanks!

ManickaP · 2021-09-07T13:22:08Z

src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs

-                if (frame == null && expectEndOfStream)
+
+                // Check if it was a zero-byte read or we got a RST_STREAM with the CANCEL code.
+                if ((frame == null || frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8) && expectEndOfStream)


Just a suggestion. I had to lookup operator precedence of && and || to make sure I understand the condition correctly.

Suggested change

if ((frame == null || frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8) && expectEndOfStream)

if ((frame == null || (frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8)) && expectEndOfStream)

I see the confusion, but then we need to add even more parenthesis because there are more || and && operations. Don't you think it could make the code less readable?

Suggested change

if ((frame == null || frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8) && expectEndOfStream)

if ((frame == null || ((frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8)) && expectEndOfStream))

Now I'm fully confused what the right order of evaluation is...

&& takes precedence over || so the order is

frame == null
OR

frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8 && expectEndOfStream

What about this?

Suggested change

if ((frame == null || frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8) && expectEndOfStream)

if ((frame == null

|| (frame.Type == FrameType.RstStream

&& ((RstStreamFrame)frame).ErrorCode == 0x8))

&& expectEndOfStream)

Or for simplicity, move expectedEndOfStream to the beginning:

Suggested change

if ((frame == null || frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8) && expectEndOfStream)

if (expectEndOfStream

&& (frame == null

|| (frame.Type == FrameType.RstStream

&& ((RstStreamFrame)frame).ErrorCode == 0x8)))

I think we need to change the code to make order of || and && explicit. It is not something anyone wants to keep looking up and I bet that 99% of developers don't remember it. I will rather survive more parenthesis than mind-blowing thinking about || and && precedence.

This way then?

Suggested change

if ((frame == null || frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8) && expectEndOfStream)

if (expectEndOfStream

&& (frame == null || (frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8)))

IMO at minimum this:

Suggested change

if ((frame == null || frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8) && expectEndOfStream)

if (expectEndOfStream

&& (frame == null || (frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8)))

Haha, that's what I suggested initially (with the exception of moving expectEndOfStream at the beginning) 😄

ManickaP · 2021-09-07T13:36:32Z

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

@@ -1307,6 +1316,13 @@ private void CloseResponseBody()
                {
                    Cancel();
                }
+                else if (_sendRstOnResponseClose)


This is called with response Dispose. Which works great for cancelling partially consumed responses. But for fully consumed response - if the user doesn't Dispose the response - we would be waiting for GC to collect the stream and send the RST. Shouldn't we rather send it immediately after we reach the end of response stream?

Note that I'm not saying this is wrong, just thinking of possible scenarios that could happen.

Hmm. You are right, delaying sending seems as an issue. Do you know how and where we can react to reaching the end of response stream?

I'd say in this else block:

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

Lines 1114 to 1118 in 49ea27e

else

{

// We've hit EOF. Pull in from the Http2Stream any trailers that were temporarily stored there.

MoveTrailersToResponseMessage(responseMessage);

}

geoffkizer · 2021-09-07T22:22:16Z

@geoffkizer I discussed this behavior with the Kestrel team and we decided that when the request body is skipped the RST_STREAM with CANCEL should be sent after the response body receival has completed. This is required by the HTTP/2 stream states transition rules as described RFC 7540 section 5.1.

Sounds right to me.

Send RST_STREAM instead EOF when sending request body is cancelled

54b86b1

alnikola added this to the 7.0.0 milestone Sep 2, 2021

alnikola requested a review from a team September 2, 2021 11:16

dotnet-issue-labeler bot added the area-System.Net.Http label Sep 2, 2021

alnikola added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 2, 2021

Send RST_STREAM on a response body close as it's required by RFC 7540…

85b8f3f

… section 5.1

alnikola removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 6, 2021

ManickaP reviewed Sep 7, 2021

View reviewed changes

Add more parethesis

98cc1be

ManickaP approved these changes Sep 8, 2021

View reviewed changes

alnikola merged commit c462a68 into dotnet:main Sep 8, 2021

alnikola deleted the alnikola/58498-rst-insted-eof-body-not-sent branch September 8, 2021 12:36

ghost locked as resolved and limited conversation to collaborators Oct 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send RST_STREAM instead EOF when sending request body is cancelled #58548

Send RST_STREAM instead EOF when sending request body is cancelled #58548

alnikola commented Sep 2, 2021

ghost commented Sep 2, 2021

alnikola commented Sep 2, 2021

alnikola commented Sep 6, 2021

alnikola commented Sep 6, 2021

azure-pipelines bot commented Sep 6, 2021

ManickaP left a comment

ManickaP Sep 7, 2021 •

edited

Loading

alnikola Sep 7, 2021

ManickaP Sep 7, 2021

alnikola Sep 7, 2021

karelz Sep 7, 2021 •

edited

Loading

karelz Sep 7, 2021

alnikola Sep 7, 2021

karelz Sep 7, 2021 •

edited

Loading

alnikola Sep 7, 2021

ManickaP Sep 7, 2021

ManickaP Sep 7, 2021

alnikola Sep 7, 2021

ManickaP Sep 7, 2021

geoffkizer commented Sep 7, 2021

	if ((frame == null \|\| frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8) && expectEndOfStream)
	if ((frame == null \|\| (frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8)) && expectEndOfStream)

	if ((frame == null \|\| frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8) && expectEndOfStream)
	if ((frame == null \|\| ((frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8)) && expectEndOfStream))

-                if ((frame == null || frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8) && expectEndOfStream)
+                if (expectEndOfStream
+                    && (frame == null
+                        || (frame.Type == FrameType.RstStream
+                            && ((RstStreamFrame)frame).ErrorCode == 0x8)))

	if ((frame == null \|\| frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8) && expectEndOfStream)
	if (expectEndOfStream
	&& (frame == null \|\| (frame.Type == FrameType.RstStream && ((RstStreamFrame)frame).ErrorCode == 0x8)))

	else
	{
	// We've hit EOF. Pull in from the Http2Stream any trailers that were temporarily stored there.
	MoveTrailersToResponseMessage(responseMessage);
	}

Send RST_STREAM instead EOF when sending request body is cancelled #58548

Send RST_STREAM instead EOF when sending request body is cancelled #58548

Conversation

alnikola commented Sep 2, 2021

ghost commented Sep 2, 2021

alnikola commented Sep 2, 2021

alnikola commented Sep 6, 2021

alnikola commented Sep 6, 2021

azure-pipelines bot commented Sep 6, 2021

ManickaP left a comment

Choose a reason for hiding this comment

ManickaP Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelz Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelz Sep 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffkizer commented Sep 7, 2021

ManickaP Sep 7, 2021 •

edited

Loading

karelz Sep 7, 2021 •

edited

Loading

karelz Sep 7, 2021 •

edited

Loading