fix error mapping in HttpWebRequest to handle NetworkException#40666
fix error mapping in HttpWebRequest to handle NetworkException#40666geoffkizer merged 2 commits intodotnet:masterfrom
Conversation
|
Tagging subscribers to this area: @dotnet/ncl |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@antonfirsov The sync Connect fix here will need to get ported to your ConnectionFactory change. |
scalablecory
left a comment
There was a problem hiding this comment.
lgtm. we might consider mapping the NoData error; I think it is reasonable to handle it as HostNotFound.
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| catch (SocketException se) | ||
| { | ||
| socket.Dispose(); | ||
|
|
||
| // SocketConnectionFactory wraps SocketException in NetworkException. Do the same here. | ||
| NetworkException ne = NetworkErrorHelper.MapSocketException(se); | ||
|
|
||
| throw CreateWrappedException(ne, host, port, cancellationToken); | ||
| } |
There was a problem hiding this comment.
Wouldn't it be wise to add a test for this path?
There was a problem hiding this comment.
Probably, yeah. I want to get this in so people aren't seeing this in CI.
| private static WebExceptionStatus GetStatusFromExceptionHelper(HttpRequestException ex) | ||
| { | ||
| SocketException? socketEx = ex.InnerException as SocketException; | ||
| NetworkException? networkException = ex.InnerException as NetworkException; |
There was a problem hiding this comment.
Any concerns about a breaking change here for other consumers of HttpClient? i.e. if WebRequest is fishing out the inner exception and expecting it to be a SocketException and now it's a NetworkException?
There was a problem hiding this comment.
It's a minor concern, but I'm not sure there's a better option.
Note that this only matters for exceptions that come during Connect, since those are the only ones that have SocketException as the inner exception today. Other exceptions are already IOExceptions.
The ConnectionFactory stuff is generalizing the Connect path so that you can plug in your own transport. In general these can't throw SocketException, and that's what we invented NetworkException for. We could special case SocketConnectionFactory in SocketsHttpHandler, pull out the inner SocketException, and discard the NetworkException, but that means we'll have different exception behavior based on whether you use SocketConnectionFactory or not, which seems bad.
Long-term, a better solution here would be to have distinct HttpRequestException errors like HostNotFound so that a user can inspect it directly and doesn't need to fish out the inner exception. WebRequestException already has this, as you can see here. We've discussed doing this in the past but haven't actually implemented it.
Fixes #40606
HttpWebRequest was specifically looking for a SocketException to detect and map HostNotFound. Change this to look for NetworkException.
Additionally, fix the sync Connect path in HttpClient to also throw NetworkException instead of SocketException.