Clean up HTTP/2 protocol exception handling by stephentoub · Pull Request #37223 · dotnet/corefx

stephentoub · 2019-04-26T15:13:14Z

Propagate the GOAWAY and RST_STREAM error codes to the calling code via an exception.
Propagate to the Http2Stream exceptions incurred in the Http2Connection and that result in an Abort.
Don't Dispose of the Http2Stream before we try to Cancel it.
Make Http2ProtocolException serializable, as we generally do that for all of our exception types.
Add tests that verify sufficient HTTP/2 protocol details are in the exception messages.
Clean up the error message for the exception.
Separate the error code and exception into their own files.
Leave Http2ProtocolException internal for now; we can expose it publicly when we design the all-up public error model for HttpClient.

cc: @geoffkizer, @davidsh, @wfurt
Fixes https://github.com/dotnet/corefx/issues/31315 (exposing error codes publicly should be handled as part of https://github.com/dotnet/corefx/issues/26477)

davidsh

Left a few comments. LGTM

wfurt

LGTM.

stephentoub · 2019-04-27T19:29:29Z

/azp run corefx-outerloop-linux

stephentoub · 2019-04-27T19:29:36Z

/azp run corefx-outerloop-windows

azure-pipelines · 2019-04-27T19:29:41Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2019-04-27T19:29:48Z

Azure Pipelines successfully started running 1 pipeline(s).

JamesNK · 2019-04-28T02:58:11Z

This may be useful to gRPC. Some questions:

Http2ProtocolException is still internal in this PR. Is the barrier to making it public going through an API review process?
Where could this exception be thrown from? SendAsync, Write/Read on body streams, etc
In the 3.0 timeframe could we expose the error code value via Exception.Data? Parsing it out of the exception message is an option but one I'd like to avoid.

stephentoub · 2019-04-28T10:34:00Z

Is the barrier to making it public going through an API review process?

No, it's designing an error model that makes holistic sense, across both HTTP/1.1 and HTTP/2, and that serves the needs of everyone who's been asking for it. That's what #26477 tracks doing. I was hopeful it would happen for 3.0, but it currently doesn't look like it. I don't want to expose something just for HTTP/2 if it's not going to fit in well with that, and I don't think exposing another exception type is how we'd actually want this surfaced, more likely enums and properties.

stephentoub · 2019-04-28T10:39:17Z

In the 3.0 timeframe could we expose the error code value via Exception.Data? Parsing it out of the exception message is an option but one I'd like to avoid.

You said this wasn't blocking. Please avoid doing anything like parsing exception text, which isn't stable and isn't guaranteed from release to release, could be localized and change even in a single release, could change even in a patch, could be stripped out of binaries in tooling for size, etc. Exception.Data is similar.

JamesNK · 2019-04-28T20:16:07Z

Sure. I was thinking of it only as a contingency. The spec says the code is only used to provide more information to the user, but our gRPC interop tests focus on successful scenarios so I'm not completely certain.

- Propagate the GOAWAY and RST_STREAM error codes to the calling code via an exception. - Propagate to the Http2Stream exceptions incurred in the Http2Connection and that result in an Abort. - Don't Dispose of the Http2Stream before we try to Cancel it. - Make Http2ProtocolException serializable, as we generally do that for all of our exception types. - Add tests that verify sufficient HTTP/2 protocol details are in the exception messages. - Clean up the error message for the exception. - Separate the error code and exception into their own files. - Leave Http2ProtocolException internal for now; we can expose it publicly when we design the all-up public error model for HttpClient.

* improve error handling on failed Http/2 handshake * remove extra space * add test for HTTP2 client talking to HTTP1 server. * fix broken tests * improve exception handling * update exception handling * ws update * fix bad merge * update test and changes from #37223 * use _abortException only if the stream is not aborted already * fix IsAborted check * updates to syncup with recent changes

* Clean up HTTP/2 protocol exception handling - Propagate the GOAWAY and RST_STREAM error codes to the calling code via an exception. - Propagate to the Http2Stream exceptions incurred in the Http2Connection and that result in an Abort. - Don't Dispose of the Http2Stream before we try to Cancel it. - Make Http2ProtocolException serializable, as we generally do that for all of our exception types. - Add tests that verify sufficient HTTP/2 protocol details are in the exception messages. - Clean up the error message for the exception. - Separate the error code and exception into their own files. - Leave Http2ProtocolException internal for now; we can expose it publicly when we design the all-up public error model for HttpClient. * Address PR feedback Commit migrated from dotnet/corefx@7237da0

* improve error handling on failed Http/2 handshake * remove extra space * add test for HTTP2 client talking to HTTP1 server. * fix broken tests * improve exception handling * update exception handling * ws update * fix bad merge * update test and changes from dotnet/corefx#37223 * use _abortException only if the stream is not aborted already * fix IsAborted check * updates to syncup with recent changes Commit migrated from dotnet/corefx@c412fa7

stephentoub commented Apr 26, 2019

View reviewed changes

Comment thread src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs

davidsh added the area-System.Net.Http.SocketsHttpHandler label Apr 26, 2019

davidsh added this to the 3.0 milestone Apr 26, 2019