Runtime: HTTP2: Incorrect exception thrown when attempting to resend non-rewindable content

[geoffkizer]

See SocketsHttpHandler_PostScenarioTest.PostNonRewindableContentUsingAuth_NoPreAuthenticate_ThrowsHttpRequestException with HTTP2

This is throwing an InvalidOperationException, when it should be throwing an HttpRequestException with an inner exception of type InvalidOperationException.

[karelz]

@eiriktsarpalis can you please fix this one as well? @davidsh can provide advice/pointers ...

[eiriktsarpalis]

@geoffkizer could you provide a bit more context? Test is passing locally and seems to be doing the same in outerloop test runs.

[davidsh]

@geoffkizer could you provide a bit more context? Test is passing locally and seems to be doing the same in outerloop test runs.

I suspect that this test is only failing due to @geoffkizer pending PR dotnet/corefx#39343. So, until that PR gets merged, you'l need to temporarily adjust the test's code to force it to use HTTP/2.0 protocol. Then I think you'll see the test failure.

[eiriktsarpalis]

I forced http/2 by changing this line, but the test is still passing for me locally.

[davidsh]

I don't think this test derives from that base class. Also, you need to make sure the test endpoint is the HTTP/2 remote server and not the HTTP/1 remote server.

If it still isn't repro'ing then @geoffkizer will need to comment.

[stephentoub]

I forced http/2 by changing this line, but the test is still passing for me locally.

I don't think that's sufficient, as in master the server it's talking to only speaks 1.1:
https://github.com/dotnet/corefx/blob/689e58ec123d8c730d48d1c35c8f7ecbba6c22cf/src/Common/tests/System/Net/Configuration.cs#L10
@geoffkizer's changes in https://github.com/dotnet/corefx/pull/39343 are fixing up these tests to actually talk to an appropriate remote server for its version:
https://github.com/dotnet/corefx/pull/39343/files#diff-94af524660d1444b9a137d3b0f637775R182

I suggest you pull down his PR and remove the if above to see if it then repros for you.

[eiriktsarpalis]

OK, I'm repro'ing now.

[eiriktsarpalis]

Comparing with the v1.1 implementation it seems like I need to adapt the logic found here to this handler.

As a side note, the exception emanates from HttpContent.SerializeToStreamAsync() which is override-able and could throw arbitrary exception types. Shouldn't those be getting wrapped as well?

[davidsh]

Shouldn't those be getting wrapped as well?

Probably. If you decide to fix that as part of this PR, please add more tests to validate that fix.

[eiriktsarpalis]

Probably. If you decide to fix that as part of this PR, please add more tests to validate that fix.

Is divergence of behaviour compared to the 1.1 implementation acceptable?

[davidsh]

Is divergence of behaviour compared to the 1.1 implementation acceptable?

The exception should only be wrapped as it emanates from the HttpClient APIs. It doesn't get wrapped right away from those HttpContent methods. Perhaps it would be best to focus on that change separately and with a specific test case so that we can better understand what behaviors might change.