search

dotnet / android

.NET for Android provides open-source bindings of the Android SDK for use with .NET managed languages such as C#
MIT License
1.92k stars 526 forks source link

Some of the new wrapped Java exceptions use BCL exceptions that differ from the related BCL types #4127

Open brendanzagaeski opened 4 years ago

brendanzagaeski commented 4 years ago

Context

https://github.com/xamarin/xamarin-android/commit/60363ef45cd6994a322adac1b73d4271b0658c1e added an option to wrap Java exception types in .NET exception types for certain Android APIs.

JavaDictionary.Get() is an example:

https://github.com/xamarin/xamarin-android/blob/8d7557aad23a3c0724e1996aade3072b1a09f298/src/Mono.Android/Android.Runtime/JavaDictionary.cs#L72-L86

Expected behavior

JavaDictionary inherits from Java.Lang.Object and implements IDictionary:

https://github.com/xamarin/xamarin-android/blob/8d7557aad23a3c0724e1996aade3072b1a09f298/src/Mono.Android/Android.Runtime/JavaDictionary.cs#L12

The documentation for the IDictionary.Item[Object] indexer property mentions two exceptions:

Actual behavior

JavaDictionary.Get() can currently throw the following two different exceptions instead:

Depending on the goals of the Java exception wrapping, throwing different System exceptions from JavaDictionary.Get() than the ones documented for IDictionary.Item[Object] might be unintended. Maybe only the exception conditions documented for IDictionary.Item[Object] (like "key is null") need to be surfaced as System exceptions, and other exceptions, if any, can be left as Java exceptions?

Related scenarios

If this JavaDictionary.Get() scenario does look good to change, then there are probably a few additional similar cases to change too.

Another slightly different case is JavaCollection.Add(). I think it might have been wrapped because JavaCollection implements ICollection, but I just noticed today that the non-generic ICollection interface doesn't include an Add() method. Only the generic ICollection<T> includes an Add() method. So maybe JavaCollection.Add() doesn't need to wrap any exceptions?

brendanzagaeski commented 4 years ago

Additional background context: From a first quick chat with @jonpryor, it sounded like this might be unintentional, so I added an issue here to help with tracking potential changes to the behavior.

brendanzagaeski commented 4 years ago

Another related case is that with the new exception mode enabled Android.Runtime.InputStreamInvoker.Read() will at the moment throw things like:

brendanzagaeski commented 4 years ago

One more interesting case is AndroidClientHandler, which currently throws exceptions like Java.Net.UnknownHostException instead of System.Net.Http.HttpRequestException. I think that type might have been unintentionally overlooked in the first set of changes because it isn't a binding for an existing Android or Java type.

Adjusting AndroidClientHandler.SendAsync() to throw the documented excpetions for HttpClient.SendAsync() and HttpClientHandler.SendAsync() will fix https://github.com/xamarin/xamarin-android/issues/3216.