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

Android / iOS / tvOS specific HttpClientHandler #47083

Merged
merged 16 commits into from
Mar 2, 2021

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jan 16, 2021

Implements #46646 and #47083 (comment)

Per https://github.com/dotnet/designs/blob/main/accepted/2020/mono-convergence/platform-specific-httpclient.md , the default HttpClientHandler will be the native handler specific to Android, iOS, and tvOS.

To fall back to SocketsHttpHandler, simply set the System.Net.Http.UseNativeHttpHandler AppContext switch.

AppContext.SetSwitch("System.Net.Http.UseNativeHttpHandler", true);

@ghost
Copy link

ghost commented Jan 16, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Implements #46646

Should make .NET6 compatible with what XA currently have:

  1. "ComboBox" with custom http handlers in VS and VSfM
    image
  2. <AndroidHttpClientHandlerType>AndroidClientHandler</AndroidHttpClientHandlerType> property in csproj
  3. Special ILLink sub-step (PreserveHttpAndroidClientHandler) to remove/preserve the native android handler if it's not used.
Author: EgorBo
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@stephentoub
Copy link
Member

will use the default SocketHttpHandler even if the native one was selected in VS

Did we decide that's not a scenario we care about?

@EgorBo
Copy link
Member Author

EgorBo commented Jan 17, 2021

will use the default SocketHttpHandler even if the native one was selected in VS

Did we decide that's not a scenario we care about?

It's open for discussion, I was just repeating the current XA behavior.

@steveisok
Copy link
Member

will use the default SocketHttpHandler even if the native one was selected in VS

Did we decide that's not a scenario we care about?

I don't see a reason not to preserve the existing behavior. But as Egor said, definitely up for discussion.

@steveisok
Copy link
Member

@EgorBo @akoeplinger Thoughts on how we can test this?

@stephentoub
Copy link
Member

I don't see a reason not to preserve the existing behavior

If the existing Xamarin behavior is that new HttpClientHandler() doesn't respect the user choice and may not work correctly, and we haven't heard much concern, ok.

@EgorBo
Copy link
Member Author

EgorBo commented Jan 19, 2021

@EgorBo @akoeplinger Thoughts on how we can test this?

Either leave it for XA repo or implement that Android Handler via JNI (we can't just re-write it to JNI and use it even for XA because it already exposes some XA APIs in public (to override if users want) so we'll have to maintain two implementation in this case 😐

@steveisok steveisok changed the title Android specific HttpClientHandler Android / iOS / tvOS specific HttpClientHandler Feb 8, 2021
@marek-safar marek-safar self-requested a review February 17, 2021 09:33
steveisok pushed a commit to steveisok/xamarin-android that referenced this pull request Feb 19, 2021
In dotnet/runtime#47083, we are introducing the UseNativeHttpHandler feature switch.  This change sets the
default to true
steveisok added a commit to dotnet/android that referenced this pull request Feb 22, 2021
In dotnet/runtime#47083, we are introducing the UseNativeHttpHandler feature switch. This change sets the default to true for Android projects.
steveisok pushed a commit to xamarin/xamarin-macios that referenced this pull request Feb 23, 2021
In dotnet/runtime#47083, we are introducing the UseNativeHttpHandler feature switch. This change sets the default to true for devices.
steveisok added a commit to xamarin/xamarin-macios that referenced this pull request Feb 23, 2021
…ault

In dotnet/runtime#47083, we are introducing the UseNativeHttpHandler feature switch. This change sets the default to true for devices.
Base automatically changed from master to main March 1, 2021 09:07
@steveisok steveisok merged commit 8df235a into dotnet:main Mar 2, 2021
steveisok pushed a commit to dotnet/sdk that referenced this pull request Mar 2, 2021
In dotnet/runtime#47083, we are introducing the UseNativeHttpHandler feature switch.  This
change makes sure it's defaulted to true for iOS,tvOS,maccatalyst, and android.
steveisok added a commit to dotnet/sdk that referenced this pull request Mar 2, 2021
In dotnet/runtime#47083, we are introducing the UseNativeHttpHandler feature switch. Adds support for it.
<argument>ILLink</argument>
<argument>IL2075</argument>
<property name="Scope">member</property>
<property name="Target">M:System.Net.Http.HttpClient.CreateDefaultHandler()</property>
Copy link
Member

@eerhardt eerhardt Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EgorBo @marek-safar - What is the plan to address this warning? We had this library ILLink "clean" already, and so it was no longer being tracked by #45623.

In the future, we should not be adding new ILLink warning suppression xmls to the libraries that are already clean. We need to address the ILLink warnings on any new features we add as we go.

Copy link
Contributor

@marek-safar marek-safar Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suppression is intentional and will stay there. We could ideally move it to Library.xml but I think we tried that and it had some problems.

Edit: just realized you were taking about XML file. Yes we should move the suppression to the code.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 19, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants