[AndroidClientHandler] Wrap Java exceptions in .NET ones

[grendello]

Context: 60363ef

This commit is, in a way, a follow-up to 60363ef in that it causes less
Java exceptions to "leak" to applications which use code shared between
platforms that might not be able to check for Java exception types
directly (because they don't link against Mono.Android.dll). However,
instead of making the behavior optional, as implemented in 60363ef,
this commit unconditionally wraps Java.IO.Exception and some of its
derivatives in the standard .NET WebException. This makes the handler
to behave in a manner that's closer to how other HttpClient custom
handlers behave.

[brendanzagaeski]

• Since this changes behavior (e.g., if apps are currently catching the Java exception types, those catches would stop working after updating Xamarin.Android versions), maybe this new behavior can go under an MSBuild property too, like 60363ef?

• Depending on the intended scope of this PR, maybe it would be handy to cover the exceptions mentioned in Android HttpClient implementation in .NET Standard Project throwing Java Exception #3216 (cross-reference: Some of the new wrapped Java exceptions use BCL exceptions that differ from the related BCL types #4127 (comment)) so that item could be closed?

  For example, Java.Net.UnknownHostException can be wrapped in System.Net.Http.HttpRequestException for:

  try {
  	var client = new HttpClient ();
  	await client.GetStringAsync ("https://does.not.exist.contoso.com");
  } catch (System.Net.Http.HttpRequestException ex) {
  	Console.WriteLine (ex);
  }

  (This catches in a .NET Core app but not yet with AndroidClientHandler.)

  And Javax.Net.Ssl.SSLHandshakeException can be wrapped in System.Net.Http.HttpRequestException too:

  try {
  	var client = new HttpClient ();
  	await client.GetStringAsync ("https://has.an.expired.tls.cert.contoso.com");
  } catch (System.Net.Http.HttpRequestException ex) {
  	Console.WriteLine (ex);
  }

  (This also catches in a .NET Core app but not yet with AndroidClientHandler.)

• Maybe HttpRequestException fits well for the other wrappings added in this PR too instead of WebException? (Reference: HttpClient.SendAsync() , HttpClientHandler.SendAsync())

  For example, using 192.168.1.255 as a placeholder for an endpoint that times out, and assuming the app is configured to allow cleartext HTTP traffic to that address:

  try {
  	var client = new HttpClient(new AndroidClientHandler {
  		ConnectTimeout = TimeSpan.FromMilliseconds (10),
  		ReadTimeout = TimeSpan.FromMilliseconds(10)
  	});
  	await client.GetStringAsync ("http://192.168.1.255");
  } catch (System.Net.Http.HttpRequestException ex) {
  	Console.WriteLine (ex);
  }

  This catches in a .NET Core app (using the default SocketsHttpHandler) but not yet in a Xamarin.Android app with AndroidClientHandler. With the changes from this PR, the exception would be wrapped from Java.Net.SocketTimeoutException into a WebException instead of an HttpRequestException.

• Perhaps even the older WebException wrapping for Java.Net.ConnectException could be adjusted conditionally to HttpRequestException based on the new MSBuild property? For example, using 192.168.1.255:22 as a placeholder for an endpoint that refuses a connection:

  try {
  	var client = new HttpClient ();
  	});
  	await client.GetStringAsync ("http://192.168.1.255:22");
  } catch (System.Net.Http.HttpRequestException ex) {
  	Console.WriteLine (ex);
  }

  This catches in a .NET Core app but not yet in a Xamarin.Android app with AndroidClientHandler because it throws a WebException instead of an HttpRequestException.

• Maybe tests could also be added for some of the scenarios like the 4 examples in this comment? Maybe the existing test servers for other connection tests can produce these exceptions?

[brendanzagaeski]

Maybe the existing test servers for other connection tests can produce these exceptions?

Or maybe the BCL tests for Mono or .NET Core include some HttpClient exception scenarios that could help ensure matching behavior for AndroidClientHandler?

[jonpryor]

As @brendanzagaeski pointed out, these catch blocks should use when (JNIEnv.ShouldWrapJavaException (ex)), as was done in 60363ef.

[grendello]

@brendanzagaeski I'm in two minds regarding WebException vs HttpRequestException - the former is thrown by the default HttpClient handler implementation which uses the "old" .NET networking stack, the latter may be thrown by other handlers and is thrown by HttpClient itself but switching to it breaks backward compatibility of AndroidClientHandler. Mixing the two is, IMO, not advisable as it would introduce some confusion on which exception is thrown when - we should use one of them. I don't have an opinion on which is "better" - it's ultimately a strategic decision at this point. @jonpryor ?

Regarding tests... Yeah, it would be nice, but testing network behaviors is hard and in our case we can't mock the requests - we must issue actual requests to actual servers, so that the Java backend actually kicks in. Yes, we could make our code mockable by introducing a fake Java backend that would throw certain exceptions at certain points, but I honestly don't see much value in it. Regarding the test servers - we've been wanting to do it for quite a while now (to use https://httpbin.org/ on the test servers so we can perform all kinds of tests in controlled manner) but so far it hasn't happened. When we have a server we can rely on and fully control (in the httpbin sense), then sure.

[grendello]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jonpryor]

The underlying intent behind 60363ef was that ".NET methods" should throw only documented exception types.

HttpClient.GetAtringAsync(string) only documents two exception types:

• ArgumentNullException
• HttpRequestException

WebException is not on this list. Consequently, we should be throwing HttpRequestException.

[jonpryor]

…except:

1. HttpRequestException lacks additional useful information such as WebException.Status
2. The HttpClient docs are quasi-meaningless, because
3. We actually care about HttpClientHandler, not HttpClient, and
4. HttpClientHandler.SendAsync() is more useless, because it only documents ArgumentNullException.

Nor are the HttpClient docs complete; ObjectDisposedException, OperationCanceledException, and TimeoutException aren't documented either, and we see places where those can be thrown from HttpClient!

Thus, if everything is meaningless, go with the type that provides the most useful info: WebException.