Implement HttpProtocolException for HTTP/3 #72095

rzikm · 2022-07-13T13:27:42Z

Follow-up on #71432.

ghost · 2022-07-13T13:27:56Z

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

Issue Details

Follow-up on #71432.

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

antonfirsov · 2022-07-13T13:53:37Z

We have plenty of tests for HTTP/2 exercising error scenarios, for example:

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

Lines 300 to 316 in a248f40

    
           public async Task Http2_StreamResetByServerBeforeHeadersSent_RequestFails() 
        
           { 
        
               using (Http2LoopbackServer server = Http2LoopbackServer.CreateServer()) 
        
               using (HttpClient client = CreateHttpClient()) 
        
               { 
        
                   Task<HttpResponseMessage> sendTask = client.GetAsync(server.Address); 
        
                   Http2LoopbackConnection connection = await server.EstablishConnectionAsync(); 
        
                   int streamId = await connection.ReadRequestHeaderAsync(); 
        
                   // Send a reset stream frame so that the stream moves to a terminal state. 
        
                   RstStreamFrame resetStream = new RstStreamFrame(FrameFlags.None, (int)ProtocolErrors.INTERNAL_ERROR, streamId); 
        
                   await connection.WriteFrameAsync(resetStream); 
        
                   await AssertProtocolErrorAsync(sendTask, ProtocolErrors.INTERNAL_ERROR); 
        
               } 
        
           }

Is it possible to cover at least a few HTTP/3 cases to verify that we are throwing this exception (including at least one response stream case)?

stephentoub · 2022-07-13T14:26:56Z

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

+            Priority = 0b00100000,

-            ValidBits =     0b00101101
+            ValidBits = 0b00101101


Was this formatting alignment intentionally removed?

ManickaP · 2022-07-13T16:47:09Z

Please don't merge this before #71969 unless you're 100% sure it will not create conflicts. I really need to get that PR in main.

CarnaViire · 2022-07-14T11:55:00Z

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

                        // Our stream was reset.
-                        Exception? abortException = _connection.AbortException;
-                        throw new HttpRequestException(SR.net_http_client_execution_error, abortException ?? ex);
+                        throw new HttpRequestException(SR.net_http_client_execution_error, HttpProtocolException.CreateHttp3StreamException(code));


Is it ok that we are ignoring both _connection.AbortException and ex now?

I don't think there is any value of including ex in the HttpProtocolException (it will always be the same and the error code is already in HttpProtocolException.

To tell the truth, I am not 100% sure what the right behavior here is. I didn't notice anything in the RFC which would let server close the stream with other error codes than the two handled above. I would consider it protocol violation and tear down the connection, but the RFC does not AFAIK prohibit using any other code, so we should just pass it by.

As for the _connection.AbortException, there is no harm in checking first and prioritizing it if there is some already. This branch should be pretty rare anyway as explained above.

CarnaViire · 2022-07-14T11:57:33Z

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

                    Exception abortException = _connection.Abort(ex);
                    throw new IOException(SR.net_http_client_execution_error, new HttpRequestException(SR.net_http_client_execution_error, abortException));
-                case Http3ConnectionException:
+                case HttpProtocolException:


Judging by CreateHttp3StreamException in HttpProtocolException it seems to me like HttpProtocolException may not always be connection-level. Is that right? This line does not seem as equivalent change then. Or am I missing something?

The HttpProtocolException thrown from stream is not on a code path which would affect this. In other words, only connection-level exceptions can be encountered here

CarnaViire · 2022-07-14T12:03:43Z

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

+            _output.WriteLine(outerEx.InnerException.Message);
+            HttpProtocolException protocolEx = Assert.IsType<HttpProtocolException>(outerEx.InnerException);


Should it be this instead?

Suggested change

_output.WriteLine(outerEx.InnerException.Message);

HttpProtocolException protocolEx = Assert.IsType<HttpProtocolException>(outerEx.InnerException);

HttpProtocolException protocolEx = Assert.IsType<HttpProtocolException>(outerEx.InnerException);

_output.WriteLine(protocolEx.Message);

Also, if I'm wrong and it's for diagnostics purposes in case it was not a HttpProtocolException, would it be better to include all the info about exceptions, even the top one? Meaning, should we log ToString instead of just Message

I copied it from HttpClientHandlerTest.Http2 implementation, I can change it

This was somewhat accidental in my PR, we don't have to print anything, though it's informative for manual validation of HttpProtocolException's message.

antonfirsov · 2022-07-14T14:08:42Z

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

            await new[] { clientTask, serverTask }.WhenAllOrAnyFailed(20_000);
        }

+


Suggested change

antonfirsov · 2022-07-14T14:33:29Z

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

                case QuicException e when (e.QuicError == QuicError.ConnectionAborted):
                    // Our connection was reset. Start aborting the connection.
                    Exception abortException = _connection.Abort(ex);
                    throw new IOException(SR.net_http_client_execution_error, new HttpRequestException(SR.net_http_client_execution_error, abortException));


I'm wondering why are we throwing IOException here? Aren't these errors supposed to surface as HttpProtocolException from HttpContent's Http3ReadStream (which seems to delegate to this method)?

antonfirsov · 2022-07-14T14:37:38Z

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

+            HttpProtocolException protocolEx = Assert.IsType<HttpProtocolException>(outerEx.InnerException);
+            _output.WriteLine(protocolEx.Message);
+            Assert.Equal(errorCode, protocolEx.ErrorCode);
+        }


I would add yet another test (or a parameteric variant of existing tests), that validates the exception thrown from the response content stream. See AssertProtocolErrorForIOExceptionAsync in http2 tests (I should have renamed that method ...).

antonfirsov

LGTM, thanks!

rzikm · 2022-07-14T19:13:12Z

/backport to release/7.0-preview7

github-actions · 2022-07-14T19:13:32Z

Started backporting to release/7.0-preview7: https://github.com/dotnet/runtime/actions/runs/2672573752

rzikm added 2 commits July 13, 2022 14:09

Replace Http3ProtocolException with HttpProtocolException

5eff2eb

Correctly process incoming protocol errors

3bbea56

ghost added the area-System.Net.Http label Jul 13, 2022

ghost assigned rzikm Jul 13, 2022

rzikm mentioned this pull request Jul 13, 2022

Utilize ProtocolException for HTTP/2 in HttpStress #72034

Merged

Fix HTTP3 stress

a3af68c

rzikm requested a review from antonfirsov July 13, 2022 13:33

stephentoub reviewed Jul 13, 2022

View reviewed changes

Add some tests

b97cf41

Remove formating changes

047b2ce

rzikm requested a review from a team July 14, 2022 11:37

CarnaViire reviewed Jul 14, 2022

View reviewed changes

rzikm requested review from MichalStrehovsky, SamMonoRT, lambdageek, lewing, marek-safar, radical, thaystg and vargaz as code owners July 14, 2022 12:30

rzikm added 2 commits July 14, 2022 15:49

Merge remote-tracking branch 'upstream/main' into use-protocol-exception

14d6926

Code review feedback

9e4324b

rzikm force-pushed the use-protocol-exception branch from d467805 to 9e4324b Compare July 14, 2022 13:50

rzikm removed request for lewing and radical July 14, 2022 13:50

rzikm requested review from CarnaViire and removed request for MichalStrehovsky, SamMonoRT, lambdageek, marek-safar, thaystg and vargaz July 14, 2022 13:50

Fix build

030a14a

antonfirsov reviewed Jul 14, 2022

View reviewed changes

rzikm added 2 commits July 14, 2022 17:25

Throw HttpProtocolException from content stream as well

4caa3d2

Add test for throwing when reading the content stream

ace9da8

rzikm requested a review from antonfirsov July 14, 2022 16:00

antonfirsov approved these changes Jul 14, 2022

View reviewed changes

rzikm merged commit e2e9df7 into dotnet:main Jul 14, 2022

github-actions bot mentioned this pull request Jul 14, 2022

[release/7.0-preview7] Implement HttpProtocolException for HTTP/3 #72210

Merged

karelz added this to the 7.0.0 milestone Jul 19, 2022

ghost locked as resolved and limited conversation to collaborators Aug 18, 2022

		_output.WriteLine(outerEx.InnerException.Message);
		HttpProtocolException protocolEx = Assert.IsType<HttpProtocolException>(outerEx.InnerException);

		await new[] { clientTask, serverTask }.WhenAllOrAnyFailed(20_000);
		}

Implement HttpProtocolException for HTTP/3 #72095

Implement HttpProtocolException for HTTP/3 #72095

Uh oh!

Conversation

rzikm commented Jul 13, 2022

Uh oh!

ghost commented Jul 13, 2022

Uh oh!

antonfirsov commented Jul 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ManickaP commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

rzikm commented Jul 14, 2022

Uh oh!

github-actions bot commented Jul 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ManickaP commented Jul 13, 2022 •

edited

Loading

antonfirsov Jul 14, 2022 •

edited

Loading

antonfirsov Jul 14, 2022 •

edited

Loading