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

The constructor of AndroidHttpHandler should be preserved together with AndroidMessageHandler #6949

Closed
huoyaoyuan opened this issue Apr 20, 2022 · 5 comments
Assignees
Labels
Area: Linker Issues when linking assemblies.

Comments

@huoyaoyuan
Copy link
Member

Android application type

Android for .NET (net6.0-android, etc.)

Affected platform version

.NET SDK 6.0.202, Runtime 6.0.4

Description

Xamarin.Android.Net.AndroidHttpHandler was used for Xamarin android applications, and is set when using Visual Studio project setting UI.
However, only the new AndroidMessageHandler is preserved:

https://github.com/xamarin/xamarin-android/blob/bacc3f0282b05978f1cde13f0ca49ecc58048efe/src/Mono.Android/Android.Runtime/AndroidEnvironment.cs#L342-L348

When using HttpClientHandler with a migrated application, it crashes hardly in Debug configuration, and throws NullReferenceException in Release configuration. It's quite hard to debug for people unfamiliar with how http client handler works.

Steps to Reproduce

Use <AndroidHttpClientHandlerType>Xamarin.Android.Net.AndroidHttpHandler</AndroidHttpClientHandlerType> in any minimal application, and use HttpClientHandler.

Did you find any workaround?

I browsed the code path setting up native handler, and realized only AndroidMessageHandler would work.

Relevant log output

No response

@huoyaoyuan huoyaoyuan added Area: App Runtime Issues in `libmonodroid.so`. needs-triage Issues that need to be assigned. labels Apr 20, 2022
@grendello grendello added Area: Linker Issues when linking assemblies. and removed Area: App Runtime Issues in `libmonodroid.so`. needs-triage Issues that need to be assigned. labels Apr 20, 2022
@jonathanpeppers
Copy link
Member

jonathanpeppers commented Apr 20, 2022

@huoyaoyuan AndroidClientHandler is marked obsolete:
https://github.com/xamarin/xamarin-android/blob/bacc3f0282b05978f1cde13f0ca49ecc58048efe/src/Mono.Android/Xamarin.Android.Net/AndroidClientHandler.cs#L60-L61

Is there a reason you need to use it? Just so we understand the use-case.

For now, I'd workaround with a custom linker configuration:

https://docs.microsoft.com/en-us/xamarin/cross-platform/deploy-test/linker

<assembly fullname="Mono.Android">
    <type fullname="Xamarin.Android.Net.AndroidClientHandler">
        <method signature="System.Void .ctor()" />
    </type>
</assembly>

We don't want to put DynamicDependency, because that would preserve it in every net6.0-android app using HttpClient.

@jonathanpeppers jonathanpeppers self-assigned this Apr 20, 2022
@jonathanpeppers jonathanpeppers added this to the .NET 7 milestone Apr 20, 2022
@huoyaoyuan
Copy link
Member Author

Is there a reason you need to use it? Just so we understand the use-case.

Converted legacy project specifies this handler. Supporting it would make migration easier.
It's also nice to include a sdk warning when the handler is specified, and update Visual Studio project UI to select the new one.

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Apr 20, 2022

@huoyaoyuan were you using the .NET upgrade assistant?

I'd recommend removing almost every MSBuild property in the project file, and just use the defaults.

We have some of this documented here:

https://github.com/xamarin/xamarin-android/blob/main/Documentation/guides/OneDotNet.md#changes-to-msbuild-properties

Or you can see the template as what we pretty much recommend:

https://github.com/xamarin/xamarin-android/blob/bacc3f0282b05978f1cde13f0ca49ecc58048efe/src/Microsoft.Android.Templates/android/AndroidApp1.csproj#L1-L13

@akoeplinger
Copy link
Member

Just to make sure: you used Xamarin.Android.Net.AndroidClientHandler not Xamarin.Android.Net.AndroidHttpHandler in the AndroidHttpClientHandlerType property right?

@huoyaoyuan
Copy link
Member Author

Just to make sure: you used Xamarin.Android.Net.AndroidClientHandler not Xamarin.Android.Net.AndroidHttpHandler in the AndroidHttpClientHandlerType property right?

Confirmed in commit history that it was <AndroidHttpClientHandlerType>Xamarin.Android.Net.AndroidClientHandler</AndroidHttpClientHandlerType>.

were you using the .NET upgrade assistant?

No. I manually converted it based on my knowledge of MSBuild. The project file was somehow customized with .props.

The VS project setting UI (in VS 17.1) sets AndroidClientHandler when selecting in http handler. This should be probably updated to the recommended value too.

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Apr 26, 2022
Fixes: dotnet#6949
Fixes: dotnet/maui#6174

The linker feature switch `$(UseNativeHttpHandler)` defaults to:

    <UseNativeHttpHandler Condition="'$(UseNativeHttpHandler)' == ''">true</UseNativeHttpHandler

If you set `$(AndroidHttpClientHandlerType)` to something else other
than the default, like `System.Net.Http.HttpClientHandler`, you would
actually need to toggle this switch yourself for things to work
properly.

We should only default `$(UseNativeHttpHandler)` to `true` if:

* `$(AndroidHttpClientHandlerType)` is blank
* `$(AndroidHttpClientHandlerType)` is `Xamarin.Android.Net.AndroidMessageHandler`
* `$(AndroidHttpClientHandlerType)` is `Xamarin.Android.Net.AndroidClientHandler`

Otherwise, `$(UseNativeHttpHandler)` defaults to `false`.

I updated a test around this scenario, to verify the feature flag is
set in `runtimeconfig.json`.
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Apr 26, 2022
Fixes: dotnet#6949
Fixes: dotnet/maui#6174

The linker feature switch `$(UseNativeHttpHandler)` defaults to:

    <UseNativeHttpHandler Condition="'$(UseNativeHttpHandler)' == ''">true</UseNativeHttpHandler

If you set `$(AndroidHttpClientHandlerType)` to something else other
than the default, like `System.Net.Http.HttpClientHandler`, you would
actually need to toggle this switch yourself for things to work
properly.

We should only default `$(UseNativeHttpHandler)` to `true` if:

* `$(AndroidHttpClientHandlerType)` is blank
* `$(AndroidHttpClientHandlerType)` is `Xamarin.Android.Net.AndroidMessageHandler`
* `$(AndroidHttpClientHandlerType)` is `Xamarin.Android.Net.AndroidClientHandler`

Otherwise, `$(UseNativeHttpHandler)` defaults to `false`.

I updated a test around this scenario, to verify the feature flag is
set in `runtimeconfig.json`.
@ghost ghost locked as resolved and limited conversation to collaborators Oct 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: Linker Issues when linking assemblies.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants