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

[Mono.Android] Improve the GetHttpMessageHandler function #7214

Conversation

simonrozsival
Copy link
Member

@simonrozsival simonrozsival commented Jul 28, 2022

The constructor of System.Net.Http.HttpClientHandler calls AndroidEnvironment.GetHttpMessageHandler() via reflection to obtain the native HTTP handler (when $(UseNativeHttpHandler) == true). The object that is returned depends on the value of the $(AndroidHttpClientHandlerType) property.

Incorrect settings of $(AndroidHttpClientHandlerType) causes two problems in the .NET 6 builds:

  • When the type cannot be resolved or the instance of the type cannot be created, the GetHttpMessageHandler function returns null. This isn't immediately caught by the HttpClientHandler and it leads to a confusing NullReferenceException later on.
  • When the type is set to anything that derives from HttpClientHandler (including AndroidClientHandler), the app enters infinite indirect recursion because the HttpClientHandler's constructor calls the GetHttpMessageHandler function.

I base this PR on the implementation of Xamarin.iOS where we never return null and when the value is invalid, we use the default handler type as a fallback. Whenever the type is invalid (null or derived from HttpClientHandler), the AndroidMessageHandler class is used instead + a warning is logged.

The docs of $(AndroidHttpClientHandlerType) are not applicable for the .NET build as they suggest setting the property to AndroidClientHandler or HttpClientHandler. I think the best options would be to either mark the property as deprecated (and update code to completely ignore it in .NET) or to require the type to extend AndroidMessageHandler. Suggestions are welcome.

Fixes #6961

@simonrozsival simonrozsival marked this pull request as ready for review August 1, 2022 16:51
@jonpryor
Copy link
Member

The docs of $(AndroidHttpClientHandlerType) are not applicable for the .NET build as they suggest setting the property to AndroidClientHandler or HttpClientHandler. I think the best options would be to either mark the property as deprecated (and update code to completely ignore it in .NET) or to require the type to extend AndroidMessageHandler.

I think we should update the docs to note differences between Classic and .NET.

As far as requiring that the type extend AndroidMessageHandler, couldn't it just require that the type extend HttpMessageHandler? Why would extending AndroidMessageHandler be required?

We should likely also check this type during the build process, so that it's a build-time failure instead of a runtime exception. That can be done separately, by someone else. ;-)

@jonpryor
Copy link
Member

Related? #6949

@simonrozsival
Copy link
Member Author

simonrozsival commented Aug 16, 2022

@jonpryor I'll put some more thought into the docs and update it tomorrow.

The type check doesn't require extending AndroidMessageHandler but HttpMessageHandler. AndroidMessageHandler is used only as the fallback type for incorrect configurations.

EDIT: I didn't realize you were referring to my comment, not the actual implementation. I don't think my previous suggestion was sound, we shouldn't require extending that specific handler.

#6949 is most likely related

@simonrozsival
Copy link
Member Author

I believe the remaining failing tests are unrelated to this PR.

@jonpryor
Copy link
Member

jonpryor commented Sep 1, 2022

Commit message for review.

Fixes: https://github.com/xamarin/xamarin-android/issues/6961
Fixes: https://github.com/xamarin/xamarin-android/issues/6949

If a .NET SDK for Android app is built with the
`$(AndroidHttpClientHandlerType)` property set to a type which does
not exist, such as the value `Xamarin.Android.Net.AndroidClientHandler`
used by *Classic* Xamarin.Android apps (!), then the app may crash
during startup:

	I MonoDroid: UNHANDLED EXCEPTION:
	I MonoDroid: Android.Runtime.JavaProxyThrowable: Exception of type 'Android.Runtime.JavaProxyThrowable' was thrown.
	I MonoDroid: 
	I MonoDroid:   --- End of managed Android.Runtime.JavaProxyThrowable stack trace ---
	I MonoDroid: android.runtime.JavaProxyThrowable: System.NullReferenceException: Object reference not set to an instance of an object.
	I MonoDroid:    at System.Net.Http.HttpClientHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
	I MonoDroid:    at System.Net.Http.HttpMessageInvoker.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
	I MonoDroid:    at System.Net.Http.HttpClient.<>n__0(HttpRequestMessage request, CancellationToken cancellationToken)
	I MonoDroid:    at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
	I MonoDroid:    at NetworkTest.MainPage.MakeRequest()
	I MonoDroid:    at NetworkTest.MainPage.OnCounterClicked(Object sender, EventArgs e)
	I MonoDroid:    at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state)
	I MonoDroid:    at Android.App.SyncContext.<>c__DisplayClass2_0.<Post>b__0()
	I MonoDroid:    at Java.Lang.Thread.RunnableImplementor.Run()
	I MonoDroid:    at Java.Lang.IRunnableInvoker.n_Run(IntPtr jnienv, IntPtr native__this)
	I MonoDroid:    at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PP_V(_JniMarshal_PP_V callback, IntPtr jnienv, IntPtr klazz)
	I MonoDroid:     at mono.java.lang.RunnableImplementor.n_run(Native Method)
	I MonoDroid:     at mono.java.lang.RunnableImplementor.run(RunnableImplementor.java:30)
	I MonoDroid:     at android.os.Handler.handleCallback(Handler.java:938)
	I MonoDroid:     at android.os.Handler.dispatchMessage(Handler.java:99)
	I MonoDroid:     at android.os.Looper.loopOnce(Looper.java:201)
	I MonoDroid:     at android.os.Looper.loop(Looper.java:288)
	I MonoDroid:     at android.app.ActivityThread.main(ActivityThread.java:7870)
	I MonoDroid:     at java.lang.reflect.Method.invoke(Native Method)
	I MonoDroid:     at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
	I MonoDroid:     at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
	I MonoDroid: 
	I MonoDroid:   --- End of managed Android.Runtime.JavaProxyThrowable stack trace ---
	I MonoDroid: android.runtime.JavaProxyThrowable: System.NullReferenceException: Object reference not set to an instance of an object.
	I MonoDroid:    at System.Net.Http.HttpClientHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
	I MonoDroid:    at System.Net.Http.HttpMessageInvoker.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
	I MonoDroid:    at System.Net.Http.HttpClient.<>n__0(HttpRequestMessage request, CancellationToken cancellationToken)
	I MonoDroid:    at System.Net.Http.HttpClient.<SendAsync>g__Core|83_0(HttpRequestMessage request, HttpCompletionOption completionOption, CancellationTokenSource cts, Boolean disposeCts, CancellationTokenSource pendingRequestsCts, CancellationToken originalCancellationToken)
	I MonoDroid:    at NetworkTest.MainPage.MakeRequest()
	I MonoDroid:    at NetworkTest.MainPage.OnCounterClicked(Object sender, EventArgs e)
	I MonoDroid:    at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state)
	I MonoDroid:    at Android.App.SyncContext.<>c__DisplayClass2_0.<Post>b__0()
	I MonoDroid:    at Java.Lang.Thread.RunnableImplementor.Run()
	I MonoDroid:    at Java.Lang.IRunnableInvoker.n_Run(IntPtr jnienv, IntPtr native__this)
	I MonoDroid:    at Android.Runtime.JNINativeWrapper.Wrap_JniMarshal_PP_V(_JniMarshal_PP_V callback, IntPtr jnienv, IntPtr klazz)
	I MonoDroid:     at mono.java.lang.RunnableImplementor.n_run(Native Method)
	I MonoDroid:     at mono.java.lang.RunnableImplementor.run(RunnableImplementor.java:30)
	I MonoDroid:     at android.os.Handler.handleCallback(Handler.java:938)
	I MonoDroid:     at android.os.Handler.dispatchMessage(Handler.java:99)
	I MonoDroid:     at android.os.Looper.loopOnce(Looper.java:201)
	I MonoDroid:     at android.os.Looper.loop(Looper.java:288)
	I MonoDroid:     at android.app.ActivityThread.main(ActivityThread.java:7870)
	I MonoDroid:     at java.lang.reflect.Method.invoke(Native Method)
	I MonoDroid:     at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
	I MonoDroid:     at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)

The cause of the crash is that the
[`System.Net.Http.HttpClientHandler` default constructor][0] calls
[`CreateNativeHandler()`][1], which uses Reflection to call
`AndroidEnvironment.GetHttpMessageHandler()`, in order to obtain the
native HTTP handler (when `$(UseNativeHttpHandler)`=true).

The object that is returned depends on the value of the
`$(AndroidHttpClientHandlerType)` property.

When the type mentioned by `$(AndroidHttpClientHandlerType)` can't be
found, then `GetHttpMessageHandler()` will return `null`.  This isn't
immediately caught by `HttpClientHandler`, leading to a confusing
`NullReferenceException` later on.

When the type mentioned by `$(AndroidHttpClientHandlerType)` is a
type which inherits `HttpClientHandler` -- which includes the Classic
default value of `AndroidClientHandler`! -- then the app enters
infinite indirect recursion because the `HttpClientHandler`'s
constructor calls the `GetHttpMessageHandler` function which calls
the `HttpClientHandler` constructor which calls…

I base this PR on the implementation of Xamarin.iOS where we never
return `null`, and when the value is invalid we use the default handler
type as a fallback.  Whenever the type is invalid (`null` or derived
from `HttpClientHandler`), the `AndroidMessageHandler` class is used
instead and a warning is logged.

The existing docs for `$(AndroidHttpClientHandlerType)` are not
applicable for .NET SDK for Android, as they suggest setting the
property to `AndroidClientHandler` or `HttpClientHandler`.

Update the documentation for `$(AndroidHttpClientHandlerType)` to
explicitly call out .NET 6+ behavior, and that
`Xamarin.Android.Net.AndroidMessageHandler` (1e5bfa3f) or
`System.Net.Http.SocketsHttpHandler, System.Net.Http` should be  used
instead.

TODO: [Emit a build-time warning][2] if
`$(AndroidHttpClientHandlerType)` is set to an invalid value

[0]: https://github.com/dotnet/runtime/blob/7a45201181d37329b7ab0bee541ed7c42b5d94b6/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.cs#L41
[1]: https://github.com/dotnet/runtime/blob/7a45201181d37329b7ab0bee541ed7c42b5d94b6/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs#L149-L158
[2]: https://github.com/xamarin/xamarin-android/issues/7326

@jonpryor jonpryor merged commit d3e8767 into dotnet:main Sep 1, 2022
@simonrozsival simonrozsival deleted the get-http-message-handler-fallback-to-android-message-handler branch September 1, 2022 20:19
grendello added a commit to grendello/xamarin-android that referenced this pull request Sep 2, 2022
* main:
  Bump to dotnet/installer@2d1a4de 7.0.100-rc.2.22426.5 (dotnet#7314)
  [FabricBot] Apply 'needs-triage' label to new issues. (dotnet#7327)
  [Mono.Android] .NET 6+ & GetHttpMessageHandler & null, oh my! (dotnet#7214)
grendello added a commit to grendello/xamarin-android that referenced this pull request Sep 2, 2022
* mm-runtime:
  Bump to dotnet/installer@2d1a4de 7.0.100-rc.2.22426.5 (dotnet#7314)
  [FabricBot] Apply 'needs-triage' label to new issues. (dotnet#7327)
  [Mono.Android] .NET 6+ & GetHttpMessageHandler & null, oh my! (dotnet#7214)
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.NET 6 app crashes on start in Debug mode
2 participants