-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Change CurlHandler to use IOExceptions for Stream failures #6250
Change CurlHandler to use IOExceptions for Stream failures #6250
Conversation
As with WinHttpHandler, CurlHandler wraps exceptions in HttpRequestException. But WinHttpHandler switches to using IOException when sending exceptions out via Stream.Read/WriteAsync. This change does the same for CurlHandler.
| writer.Write("HTTP/1.1 200 OK\r\n"); | ||
| writer.Write($"Date: {DateTimeOffset.UtcNow:R}\r\n"); | ||
| writer.Write("Content-Type: text/plain\r\n"); | ||
| writer.Write($"Content-Length: {content.Length + 42}\r\n"); // incorrect length |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think we were able to use this construct yet? The TFS mirrored builds are still C# 5, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good to know.
|
As long as it's not going to break the mirror, structurally LGTM. |
|
Since this is part of unblocking an important scenario I am ok with it for now. I will try to add new capability to Azure within the next few days for this. In particular, we really need to test a few different "incomplete message read" scenarios. 'Content-Length' having length less than expected is one scenario. But there is also 'Transfer-Encoding: chunked' scenario where the connection is closed prior to the server sending the zero-length chunk. Finally, there is another response-test scenario where payload is sent without any Content-Length or Transfer-Encoding semantics. In that case, there is no "expected" playload length. |
|
|
||
| [ActiveIssue(6231, PlatformID.Windows)] | ||
| [Fact] | ||
| public async Task ReadAsStreamAsync_InvalidContentLengthResponse_ThrowsIOException() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change "InvalidContentLengthResponse" to "IncompleteContentLengthResponse"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
563ccc5 to
f9d4884
Compare
Change CurlHandler to use IOExceptions for Stream failures
…exception Change CurlHandler to use IOExceptions for Stream failures Commit migrated from dotnet/corefx@4375b15
Building on PR dotnet/corefx#6250, added more response stream tests dealing with invalid response body transfer semantics. Commit migrated from dotnet/corefx@42e5f46
As with WinHttpHandler, CurlHandler wraps exceptions in HttpRequestException. But WinHttpHandler switches to using IOException when sending exceptions out via Stream.Read/WriteAsync. This change does the same for CurlHandler.
This also adds a test for #6231, which was correctly failing on Unix but with the wrong exception type (which is now the correct type with the previous commit).
cc: @davidsh, @joelverhagen, @ericeil, @kapilash