Bug 47783 - AndroidClientHandler does not catch (and re-throw as managed) all possible web exception types.
Summary: AndroidClientHandler does not catch (and re-throw as managed) all possible we...
Status: NEEDINFO
Alias: None
Product: Android
Classification: Xamarin
Component: BCL Class Libraries ()
Version: 7.0 (C8)
Hardware: Macintosh Mac OS
: --- normal
Target Milestone: ---
Assignee: Marek Habersack
URL:
Depends on:
Blocks:
 
Reported: 2016-11-23 18:58 UTC by Jon Goldberger [MSFT]
Modified: 2017-11-27 15:31 UTC (History)
7 users (show)

Tags: XATriaged
Is this bug a regression?: ---
Last known good build:


Attachments
Test Project (22.98 KB, application/zip)
2016-11-23 18:58 UTC, Jon Goldberger [MSFT]
Details
test Project (25.75 KB, application/zip)
2016-11-24 00:27 UTC, Jon Goldberger [MSFT]
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report for Bug 47783 on Developer Community or GitHub if you have new information to add and do not yet see a matching new report.

If the latest results still closely match this report, you can use the original description:

  • Export the original title and description: Developer Community HTML or GitHub Markdown
  • Copy the title and description into the new report. Adjust them to be up-to-date if needed.
  • Add your new information.

In special cases on GitHub you might also want the comments: GitHub Markdown with public comments

Related Links:
Status:
NEEDINFO

Description Jon Goldberger [MSFT] 2016-11-23 18:58:02 UTC
## Description

AndroidClientHandler does not catch all possible web exception types. For instance, a Java.Net.UnknownHostException is not wrapped in the System.Net.WebException as a Java.Net.ConnectionException is. This creates some issues when using a Forms PCL project or other PCL project referenced by a XA app project as PCLs can not catch a Java exception type. Exceptions thrown, like the Java.Net.UnknownHostException, will get caught by a generic Exception catch block in a PCL, but this makes it unwieldy to determine the exact Java exception that is thrown, e.g you have to do something like:

> catch (Exception ex)
>{
> if (exc.GetType().ToString() == "Java.Net.UnknownHostException")
> Debug.WriteLine(ex);
>}

to act on a specific exception that is thrown by AndroidClientHandler other than the Java.Net.ConnectionException which seems to be the only one that is wrapped in a System.Net.WebException. See the code here:
https://github.com/xamarin/xamarin-android/blob/d198cfd5d5510bd83a25b132500843d4cca9b9e5/src/Mono.Android/Xamarin.Android.Net/AndroidClientHandler.cs#L251

It was requested by the customer who reported this issue that all possible exceptions that can occur from the AndroidClientHandler be wrapped in a System.Net.WebException so they can be caught in the catch block for a WebException

## Steps to reproduce

1. Open the attached test project

2. launch to an Android simulator or device. 

Expected result: Console output will show "WebException catch block" with the exception message and stack trace to follow. 

Actual result: Console output shows "Exception catch block" with the exception message and stack trace to follow. 


## Environment

=== Xamarin Studio Enterprise ===

Version 6.1.2 (build 44)
Installation UUID: ceaba76c-db06-4fbd-b326-f69ea53c3e01
Runtime:
	Mono 4.6.2 (mono-4.6.0-branch/08fd525) (64-bit)
	GTK+ 2.24.23 (Raleigh theme)

	Package version: 406020007

=== NuGet ===

Version: 3.4.3.0

=== Xamarin.Profiler ===

Version: 0.33.1
Location: /Applications/Xamarin Profiler.app/Contents/MacOS/Xamarin Profiler

=== Xamarin.Android ===

Version: 7.0.2.37 (Visual Studio Enterprise)
Android SDK: /Users/jongoldberger/Library/Developer/Xamarin/android-sdk-macosx
	Supported Android versions:
		4.0.3 (API level 15)
		4.1   (API level 16)
		4.2   (API level 17)
		4.3   (API level 18)
		4.4   (API level 19)
		5.0   (API level 21)
		5.1   (API level 22)
		6.0   (API level 23)
		7.0   (API level 24)

SDK Tools Version: 25.2.2
SDK Platform Tools Version: 24.0.3
SDK Build Tools Version: 24.0.3

Java SDK: /usr
java version "1.8.0_91"
Java(TM) SE Runtime Environment (build 1.8.0_91-b14)
Java HotSpot(TM) 64-Bit Server VM (build 25.91-b14, mixed mode)

Android Designer EPL code available here:
https://github.com/xamarin/AndroidDesigner.EPL

=== Xamarin Android Player ===

Version: 0.6.5
Location: /Applications/Xamarin Android Player.app

=== Apple Developer Tools ===

Xcode 8.1 (11544)
Build 8B62

=== Xamarin.Mac ===

Version: 2.10.0.113 (Visual Studio Enterprise)

=== Xamarin.iOS ===

Version: 10.2.1.5 (Visual Studio Enterprise)
Hash: 44931ae
Branch: xcode8.1
Build date: 2016-11-01 20:52:28-0400

=== Xamarin Inspector ===

Version: 0.10.0.0
Hash: e931a52
Branch: master
Build date: Thu, 18 Aug 2016 17:46:46 GMT

=== Build Information ===

Release ID: 601020044
Git revision: 0ccfcd52b95305ebd5b7eca0d88c1017035910ae
Build date: 2016-10-28 15:12:43-04
Xamarin addins: a39a869d8a78d87bdc6775f696c13a4cc9024501
Build lane: monodevelop-lion-cycle8

=== Operating System ===

Mac OS X 10.12.1
Darwin Jons-MacBook-Pro.local 16.1.0 Darwin Kernel Version 16.1.0
    Thu Oct 13 21:26:57 PDT 2016
    root:xnu-3789.21.3~60/RELEASE_X86_64 x86_64

=== Enabled user installed addins ===

Xamarin Inspector 0.10.0.0
Comment 1 Jon Goldberger [MSFT] 2016-11-23 18:58:51 UTC
Created attachment 18626 [details]
Test Project
Comment 2 Jon Goldberger [MSFT] 2016-11-24 00:27:03 UTC
Created attachment 18631 [details]
test Project

Ooops, uploaded wrong test project, This is the correct one (correct messages written to console as in steps to reproduce)
Comment 3 Marek Habersack 2016-11-24 11:23:51 UTC
@Jon, wrapping the Java exception in WebException won't change the way you determine what *Java* exception caused it. You'd still need to use .ToString() to determine the type. And if the idea is to catch every Java I/O exception and map into WebException, I don't think it's practical as this is an open set with possibly many conditions that won't directly map.
I could map some the `Java.Net` exceptions to WebExceptionStatus (https://msdn.microsoft.com/en-us/library/system.net.webexceptionstatus(v=vs.110).aspx) but the rest would just be placed in WebException.InnerException and would still require the .ToString() hack to check its type should they want to handle any other exception. The Java.Net exceptions are:

  Java.Net.BindException
  Java.Net.HttpRetryException
  Java.Net.NoRouteToHostException
  Java.Net.ProtocolException
  Java.Net.SocketTimeoutException
  Java.Net.UnknownServiceException
  Java.Net.ConnectException
  Java.Net.MalformedURLException
  Java.Net.PortUnreachableException
  Java.Net.SocketException
  Java.Net.UnknownHostException
  Java.Net.URISyntaxException

And some of them don't have a direct mapping to WebExceptionStatus.

@jonp, thoughts?
Comment 4 Ruben Buniatyan 2016-11-24 18:16:45 UTC
@Marek
In case there's no exact mapping, WebExceptionStatus.UnknownError should be used. It leads to inconsistencies if some Java network errors are wrapped in WebException while others not. For instance, in my PCL I catch WebException to handle connection errors. And that works fine on iOS. Switching to Android causes headaches and code changes, as now I have to have another catch for Exception and check a bunch of Exception.GetType().ToString() cases.
Distinguishing exceptions by their type string is not the best practice, isn't it? I do understand that there are network errors in both iOS and Android that cannot be mapped to WebExceptionStatus and in that case WebExceptionStatus.UnknownError should be used. In other words, Java.IO.IOException (network ones) should be wrapped in WebException and, if possible, be mapped.
Comment 5 Marek Habersack 2016-11-28 17:22:44 UTC
@Ruben
The code works on iOS because, IIRC, it uses the actual HttpClientHandler from BCL (or a thin wrapper around it) while on Android we use a custom implementation of the handler - there are no documented guarantees that the exceptions thrown will be the same on both platforms.

Furthermore, I chose to handle the minimum necessary set of exceptions so that we don't reference Java types we don't use - that makes linker not being able to link them out if the code doesn't use them. This creates bloat for applications that don't need to have those types in the binary.

Handling of platform-specific exceptions should be done in the OS-specific part of your application, not in the PCL code. You can delegate handling of unknown exceptions to the native part of the code easily, without having to restort to the .ToString() hack. Wrapping the exceptions in WebException will not help you in determining which Java exception caused it (which might be necessary to correctly handle the exception for the UnknownError case) and you will still need to use the .ToString() hack.

You're right that checking the exception type by their type string is bad practice, but the proposed solution doesn't fix the practice - delegating does that, IMO.

@jonp, your take?
Comment 6 Jonathan Pryor 2016-11-28 17:36:57 UTC
While "mapping" Java types to .NET types sounds reasonable, it often isn't:

1. The semantics may not fully match.

2. The mapping can be gotten wrong or out of date, increasing the likelihood of semantic "breaks" between product versions. ("It used to work in version X, but broke in version Y, because a different exception type was thrown!")

3. Linker interactions.

(3) is subtle. The linker (mostly) works on static type information; if a type is statically referenced in linker-preserved code, then the type will be preserved, otherwise it can be removed.

So let's consider a type like `Java.Net.BindException`: if *no* linker-preserved code references that type, then that type *need not exist* in a Release build of the app.

Consider Attachment #18631 [details]: when building a Release configuration, `Java.Net.BindException` doesn't exist within `Mono.Android.dll`. Ditto most of the types in Comment #3:

> $ monodis --typedef android/assets/shrunk/Mono.Android.dll | grep -i java.Net
> 247: Java.Net.InetSocketAddress (flist=1690, mlist=3451, flags=0x100001, extends=0x3ec)
> 248: Java.Net.Proxy (flist=1691, mlist=3460, flags=0x100001, extends=0x220)
> 249: Java.Net.ProxySelector (flist=1694, mlist=3471, flags=0x100081, extends=0x220)
> 250: Java.Net.ProxySelectorInvoker (flist=1697, mlist=3480, flags=0x100000, extends=0x3e4)
> 251: Java.Net.SocketAddress (flist=1698, mlist=3485, flags=0x100081, extends=0x220)
> 252: Java.Net.SocketAddressInvoker (flist=1699, mlist=3490, flags=0x100000, extends=0x3ec)
> 253: Java.Net.URI (flist=1700, mlist=3494, flags=0x100101, extends=0x220)

This in turn means that this code is *broken*:

>  if (ex.GetType().ToString() == "Java.Net.UnknownHostException")

In a Release app, the runtime type *will not be* `Java.Net.UnknownHostException`. Nor will it be `Java.IO.IOException` (which is also linked away). It will instead be `Java.Lang.Exception`.

To obtain the underlying Java class type, you would need to use the `Java.Lang.Exception.Class` property, e.g.:

>  if ((ex as Java.Lang.Exception)?.Class?.Name == "java.net.UnknownHostException")

...which you unfortunately can't do from a PCL.

If we were to add an exception mapping function as suggested, suddenly all of the types in Comment #3 would be preserved in Mono.Android.dll, increasing it's size. (For the current Release build, Mono.Android.dll is 597KB; do you *really* want that bigger? Especially considering that assemblies are stored *uncompressed* by default in .apks?)

~~~

Ultimately, all that is required is that we conform to the HttpClientHandler contract, which doesn't make any mention of "acceptable" exception types:

https://msdn.microsoft.com/en-us/library/system.net.http.httpclienthandler.sendasync(v=vs.110).aspx

This is unfortunate; there is *no* mention of what exception types the returned `Task<HttpResponseMessage>` may contain, and since an `await` will raise those exceptions, developers are left with *nothing* exception System.Exception.

Given that reading, I don't think that Mono.Android.dll itself needs to do anything more here. It should be possible to alter the PCL `MyClass` type so that it could e.g. call a delegate/raise an event when an exception is handled to determine whether the URL should be retried/etc., which would allow the app to make appropriate determinations (and "eat" the corresponding post-linked size increase).
Comment 7 Ruben Buniatyan 2016-11-28 22:30:43 UTC
@jonp
I bet no one wants Mono.Android.dll getting bigger, but consistent behavior is equally or, in some cases, more important. Well, at least for me. For instance, NSUrlSessionHandler throws WebExceptions for me so far. And that's cool.

I do understand your concerns regarding mappings. Again, what I suggest is in case you don't want to do "mapping" because of the abovementioned reasons, you can always use WebException with status of UnknownError. So the generic solution would be wrapping all Java.IO.IOExceptions in WebException. That's what I suggest. At least in PCL you will know it's a network error and not something else. Of course, you can always find a workaround, but standard catch block is the best option here and I really don't want to overcomplicate the code for a single network error detection for Android. PCL must handle this with standard catch block. Again, all I need is to know whether an error is a network error (in PCL) without any "abracadabra".

So, what if to add a new catch block for Java.IO.IOException (maybe a few more specific exceptions too from Java.Net, not all) here:

https://github.com/xamarin/xamarin-android/blob/d198cfd5d5510bd83a25b132500843d4cca9b9e5/src/Mono.Android/Xamarin.Android.Net/AndroidClientHandler.cs#L251

or replace the current catch to handle more generic Java.IO.IOException and change WebExceptionStatus to UnknownError?

I think in that case Mono.Android.dll size should remain the same or increase just a little bit. Correct?
Comment 8 jacanosalado 2017-11-24 08:40:40 UTC
Hi guys, I'm hitting this issue.
Any workaround?
Comment 9 Ruben Buniatyan 2017-11-27 15:31:20 UTC
@jacanosalado
You can use Exception.GetType().FullName. If it starts with Java.IO or Java.Net, then it is a network error in this context. That works for me for release builds in .NET Standard shared assembly.