Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unix HttpClient doesn't throw exceptions consistent with Windows/NetNative #15567

Open
mconnew opened this issue Oct 26, 2015 · 33 comments
Open
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http os-linux Linux OS (any supported distro)
Milestone

Comments

@mconnew
Copy link
Member

mconnew commented Oct 26, 2015

WCF throws specific exceptions consistent across all transports and platforms that allows developers using WCF to have unified exception handling. For HttpClient, because of lack of an explicit mechanism to indicate the error condition, we have been using HttpRequestException.InnerException.HResult as a consistent way to know what the error was. This was after a discussion with @davidsh and @CIPop about how to achieve this. For the error condition of the specified hostname in the url not resolving, we are expecting the HResult to have a value of 0x80072ee7 (ERROR_INTERNET_NAME_NOT_RESOLVED). With the Unix version of HttpClient, the value being returned is 0x00000006 (ERROR_INVALID_HANDLE). This is causing WCF to not be able to tell what the error condition is and we can't throw the correct exception. Currently we have one other error condition mapped, which is the case where the server sends a reset on the socket while we're receiving data. In this case, we expect an HResult of 0x80072eff (ERROR_INTERNET_CONNECTION_RESET).

@davidsh
Copy link
Contributor

davidsh commented Oct 26, 2015

This won't get solved completely until I finish the work on #14857.

@davidsh
Copy link
Contributor

davidsh commented Oct 26, 2015

cc: @stephentoub

@davidsh
Copy link
Contributor

davidsh commented Oct 26, 2015

My recommendation for now is for the Unix HttpClientHandler (CurlHandler) to mimic the current behavior of Windows HttpClientHandler/WinHttpHandler.

@stephentoub
Copy link
Member

My recommendation for now is for the Unix HttpClientHandler (CurlHandler) to mimic the current behavior of Windows HttpClientHandler/WinHttpHandler.

Meaning you're recomming having CurlHandler generate Win32 HRESULT values? That doesn't seem like a path to success. Is there even a complete list of all possible error codes generated? I'm hesitant to do this, even as a stop-gap until a real solution is in place, as I don't want anyone else to start depending on such semantics. When is your real solution going to be available, David?

@davidsh
Copy link
Contributor

davidsh commented Oct 26, 2015

The real solution won't be available until at least RTM because it involves an API change (addition) to the System.Net.Http contract. It's possible it won't even happen until post-RTM

cc: @SidharthNabar

@davidsh
Copy link
Contributor

davidsh commented Oct 27, 2015

Meaning you're recommending having CurlHandler generate Win32 HRESULT values? That doesn't seem like a path to success. Is there even a complete list of all possible error codes generated?

Also, it is not necessary to try to get all the possible HRESULT values to match. WCF (and also System.Net.Requests library) only needs about 2 or 3 values to unblock almost all of their scenarios.

@mconnew
Copy link
Member Author

mconnew commented Oct 27, 2015

@stephentoub, WCF and presumably other users of HttpClient need a cross platform way to categorize certain failure modes when HttpClient throws an exception. I'm really not strongly in favor of any particular solution, as long as we have one. We haven't added code to map all possible failure modes yet as we would need to create a reliable test case for each one. The one's I've listed above are the only one's we currently map (as we wrote tests for them) and it would likely be post-RTM before we add any more.
As it is, an HResult is currently being set to something, it's just the value 0x6 which means invalid handle. So somewhere an error condition is already being mapped to a Win32 HResult, just a different one than we're expecting.

@stephentoub
Copy link
Member

WCF and presumably other users of HttpClient need a cross platform way to categorize certain failure modes when HttpClient throws an exception

Then the API should be updated to include that in a meaningful way, and it sounds like that's the plan. Poorly named, but the HResult field of the exception exposes the underlying OS' error value, and that's the case across corefx. We do not want to be in the business of proclaiming one OS as the king and then trying to map all error values from other OSes to its same numbering scheme.

@stephentoub
Copy link
Member

cc: @nguerrera

@vijaykota vijaykota assigned kapilash and unassigned vijaykota Oct 27, 2015
@vijaykota
Copy link
Contributor

cc: @kapilash to confirm

So somewhere an error condition is already being mapped to a Win32 HResult

There is no mapping currently. The error code from libcurl is being exposed via innerexception.HResult field.

@kapilash
Copy link
Contributor

HttpWebRequest has some what similar requirement - to compute the appropriate value of WebExceptionStatus depending on Error. We solved it this way

There is a list of wininet errors that can get set to HResult.

@vijaykota
Copy link
Contributor

OK.. one correction for this

error code from libcurl is being exposed via innerexception.HResult

From WebRequest code, it seems the libcurl code is exposed via HttpRequestExceptin.HResult.

@stephentoub
Copy link
Member

There is no mapping currently. The error code from libcurl is being exposed via innerexception.HResult field.

Correct. The outer exception is the same one that's thrown by any handler implementation. The inner exception is specific to the handler, e.g. WinHttpHandler throws a WinHttpException, CurlHandler throws a CurlException, etc., and the HResult of that inner exception is the underlying system's error code. The outer exception I believe bubbles up the inner exception's error code; that's just how the outer exception type is implemented in the shared HttpClient implementation.

HttpWebRequest has some what similar requirement

That's different, though. That's about taking the underlying platform implementation and mapping it to the documented .NET abstraction, e.g. mapping the OS error code to the .NET error code. That's fine. That's not what's being asked for here, though, which is mapping the Linux error code to the Win32 error code, effectively trying to emulate Windows. We do not want to be in that business.

If HttpClient were documented that the HResults for any handler must be XYZ for ABC error condition, then that would be documented .NET behavior that all handlers would need to follow, and while unfortunate, we would. But that's not the case.

I doubt folks would be very pleased if we said that we should actually standardize on libcurl's error codes (after all, libcurl is more portable) and WinHttpHandler must map all of the Win32 HResults it surfaces to those libcurl error codes. This is no different.

@davidsh
Copy link
Contributor

davidsh commented Oct 27, 2015

I'm working on the design for what the solution for this will look like long term. As I said, it involves a change to the System.Net.Http contract (and we'll need to make it work on Desktop via extension methods) since we'll be adding a property to the HttpRequestException class and a new enumeration type similar to the WebExceptionStatus enum.

For now, you'll need to decide how much of a hack you want to accept for the short term to unblock WCF scenarios.

@mconnew
Copy link
Member Author

mconnew commented Oct 27, 2015

The entire idea of an HResult comes from COM, which is a Windows technology. You are already exposing an error code via a Windows-ism as it is. There are a couple of thoughts on how this could be solved in the short term.

  1. HttpRequestException have a "mapped" HResult and leave the inner exception with the original HResult. This would mean the handler specific exception has the handler specific error code, so you don't lose any fidelity. The HttpClient generic exception then optionally has a generic mapping of the problem.
  2. Use the Data property of HttpRequestException to hold a mapped value. This could even be the numeric value of a planned or existing enum. For example, the integer value of an equivalent WebExceptionStatus. The key name could even be made something to make it clear not to rely on it. E.g. "IPromiseNotToComplainWhenThisValueIsGoneInFutureReleaseErrorCode"
  3. Have WCF and every other consumer of HttpClient need to embed in it the possible HResult values of every platform that has an implementation of HttpClient. This would cause WCF to need to build a unix specific build of our assembly. Right now, our windows assembly is the same as for Linux.

Right now, no. 3 is our only option and I think that is the only one of my ideas that I would say is not viable.

@stephentoub
Copy link
Member

The entire idea of an HResult comes from COM, which is a Windows technology

This is why i said it's poorly named. If we could change the name of the Exception property, we would. That ship has sailed unfortunately. It should really be named NativeErrorCode, or something like that. We have a similar naming issue with Win32Exception; some .NET APIs are documented to throw this, so we do on Unix as well, because it's really NativeException, just poorly named.

There are a couple of thoughts on how this could be solved in the short term

If this is a critical issue to be addressed, then we should address it the right way, by bumping the priority of augmenting the HttpClient APIs so that the .NET APIs abstract away the underlying OS.

how this could be solved in the short term

If short-term were a month or two, I'd be amenable to our doing this as a temporary fix. But it sounds like with current prioritization, the real solution may not be available until RTM or beyond. By that point this behavior is locked, changing it would be a breaking change, and it's no longer "short term".

This would cause WCF to need to build a unix specific build of our assembly.

Why?

if (RuntimeInformation.IsOSPlatform(PlatformID.Windows)
{
    ... // use the error codes for Windows
}
else
{
    ... // use the error codes for another platform
}

HttpRequestException have a "mapped" HResult and leave the inner exception with the original HResult.

If HttpClient were to publically document the values expected from HttpRequestException.HResult in each of the relevant cases, and require that any conforming handler provide those values, then CurlHandler would need to comply, as these would no longer be OS-specific codes and would now be the (poor) .NET abstraction for those error cases. But this is just a poorer version of the real fix it sounds like David has in mind.

@davidsh
Copy link
Contributor

davidsh commented Oct 27, 2015

This seems like the best short-term solution since you don't need to build a separate binary.

if (RuntimeInformation.IsOSPlatform(PlatformID.Windows)
{
    ... // use the error codes for Windows
}
else
{
    ... // use the error codes for another platform
}

@mconnew
Copy link
Member Author

mconnew commented Oct 27, 2015

Unless the api catalog tool isn't valid anymore, RuntimeInformation isn't available on NetNative.
We can't access Interop.libcurl.CURLcode.* in a cross platform manner. Either we match on numeric values with comments, or we need to have our own enum with these values.

@stephentoub
Copy link
Member

RuntimeInformation isn't available on NetNative.

I expect it will be eventually even if it's not currently. @Priya91 can comment on the status of that. In the meantime, there are other ways to determine if you're on Windows or not, e.g.
static bool IsWindows() {
return Environment.NewLine == "\r\n";
}

we need to have our own enum with these values

Yes, why is that a problem? We don't have most of the error codes in an enum internally anyway. And presumably you do something like that for Windows, since they're not exposed from an framework API by name. Here are the libcurl error codes:
http://curl.haxx.se/libcurl/c/libcurl-errors.html#CURLEOK

@Priya91
Copy link
Contributor

Priya91 commented Oct 27, 2015

RuntimeInformation isn't available on NetNative.

@mconnew RuntimeInformation is available for .NET Native, it's on Nuget, so you need to install using Nuget Package Manager.

@vijaykota vijaykota assigned joshfree and unassigned kapilash Nov 19, 2015
@vijaykota vijaykota assigned kapilash and unassigned joshfree Dec 9, 2015
@vijaykota
Copy link
Contributor

Changing milestone to Future till we have resolution for #14857

@CIPop
Copy link
Member

CIPop commented Dec 12, 2016

@karelz @davidsh I'm tentatively marking this as netstandard2.0 as we're missing a good way to identify what caused the exception across all platforms.

@karelz
Copy link
Member

karelz commented Feb 2, 2017

Given WCF is not blocked and it is the only customer asking for this, we do not believe we should do anything tactical for .NET Standard 2.0.
What we should do long-term is to provide a new API which provides platform-independent way to expose lower-level stack errors.

@karelz
Copy link
Member

karelz commented Nov 7, 2019

Triage: We still agree that it would be nice to have platform independent way to find out root cause of exceptions.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@karelz karelz modified the milestones: 5.0, Future May 7, 2020
@geoffkizer
Copy link
Contributor

Triage: We still agree that it would be nice to have platform independent way to find out root cause of exceptions.

BTW, it's not just platform-independent. It's also to avoid having to fish around with inner exceptions etc to programmatically determine what happened, which is really ugly and fragile. Customers shouldn't have to do this.

I think what this boils down to is having HttpRequestErrorCode on HttpRequestException. Maybe it's worth filing a separate issue to track this specifically, to raise the visibility.

@antonfirsov
Copy link
Member

@mconnew sorry for reviving this old discussion, but I wonder if you consider this solved in the era of the cross-platform SocketsHttpHandler? If yes, how? Is there is anything else we could improve in HttpClient error handling for WCF in future releases of .NET?

@mconnew
Copy link
Member Author

mconnew commented Feb 8, 2023

Are the exceptions that SocketHttpHandler can throw consistent across all OS's? If so, are they documented anywhere?

@antonfirsov
Copy link
Member

antonfirsov commented Feb 8, 2023

Are the exceptions that SocketHttpHandler can throw consistent across all OS's?

For the platforms that support SocketsHttpHandler, yes. WASM is an exception with it's own handler, so on that platform default HttpClient behavior might be different.

are they documented anywhere?

Partially:
https://learn.microsoft.com/en-us/dotnet/fundamentals/networking/http/httpclient#http-error-handling

In other important undocumented cases it's usually HttpRequestException's InnerException that tells the actual reason of the failure:

  • SocketException indicates DNS or TCP-level errors. The exact error might be deduced by observing it's SocketErrorCode.
  • AuthenticationException indicates SSL/TLS errors
  • HttpProtocolException indicates HTTP/2 or HTTP/3 protocol errors
  • IOException covers a bunch of other uncategorized IO errors eg. server sending premature EOF. Currently it's not possible to distinguish between them.

/cc @ManickaP in case I missed something.

@mconnew if we would extend the documentation to the cases above, would be the current status good enough for WCF?

@mconnew
Copy link
Member Author

mconnew commented Feb 10, 2023

I believe that would be sufficient. WCF can't run on WASM yet because of some issues with other apis throwing PlatformNotSupportedException inappropriately so WASM behavior isn't even a concern for me yet. Having to special case one platform isn't as bad as having to platform detect 6 different platforms with logic for each.
Do you know if the. NET MAUI HttpClientHandler native wrappers match this behavior too?

@antonfirsov
Copy link
Member

antonfirsov commented Feb 11, 2023

Do you know if the. NET MAUI HttpClientHandler native wrappers match this behavior too?

You mean the platform-native HttpClientHandler implementations? I don't think so. Whether native handlers are used depends on whether the System.Net.Http.UseNativeHttpHandler AppContext switch is enabled. It is false by default, but there might be workloads which prefer turn it on, which is a fact I missed in my previous reply meaning it's inaccurate.

@simonrozsival do you know what are the default handlers on MAUI?

@simonrozsival
Copy link
Member

@antonfirsov The native handlers are turned on by default on iOS and Android:

The default native handlers are AndroidMessageHandler and NSUrlSessionHandler.

It's possible to switch to SocketsHttpHandler by adding <UseNativeHttpHandler>false</UseNativeHttpHandler> into the app's csproj file.

@mconnew
Copy link
Member Author

mconnew commented Feb 13, 2023

If MAUI's HttpClientHandler wrapping AndroidMessageHandler and NSUrlSessionHandler don't make any effort to match exception behavior, then we still can't have consistent error handling code. I suspect it's too late now, but I think wrapping the native implementation by default is a mistake. It's a pit of failure for libraries which haven't explicitly targeted running on MAUI which use HttpClientHandler. It encourages try { /*...*/ } catch(Exception ex) { /* Generic exception handling */ } which is usually a code smell and bad practice.

@antonfirsov
Copy link
Member

antonfirsov commented Feb 13, 2023

The reason why I was asking questions in old issues is that I'm doing analysis around #76644. If it turns out to be a feasible approach, we may end up defining a set of generic error codes, similar to ones in WinHttp or WebExceptionStatus eg. NameResolutionError, SslError, InvalidResponse, InvalidHeaders etc.

If native mobile implementations can deduce these cases from the native errors/exceptions, it should be possible to deliver better compatibility in this space. It would need work from MAUI side though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Net.Http os-linux Linux OS (any supported distro)
Projects
None yet
Development

No branches or pull requests