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

[Trimming] Put code for resolving HttpMessageHandler type name behind feature switch #10002

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

simonrozsival
Copy link
Member

We support passing the HTTP message handler type name via the AndroidHttpClientHandlerType or the XA_HTTP_CLIENT_HANDLER_TYPE environment variable. This is not compatible with trimming by default and while we can make sure this type is preserved by IL Link via custom trimmer steps, there is no reasonable way to do this for NativeAOT.

I suggest disabling the handler type lookup via XA_HTTP_CLIENT_HANDLER_TYPE env variable by default and introducing a feature switch to re-enable this feature. The GetHttpMessageHandler() method would simply return an instance of AndroidMessageHandler by default.

If this is deemed too drastic, we could only disable the feature switch for NativeAOT.

Open question and follow-up work:

  • is VS still showing the old dropdown which would set AndroidHttpClientHandlerType instead of a checkbox for UseNativeHttpHandler?
  • we have custom trimmer steps to preserve the type in AndroidHttpClientHandlerType somewhere, that should be skipped if the feature switch is not enabled

@simonrozsival simonrozsival marked this pull request as ready for review April 7, 2025 14:11
@jonpryor
Copy link
Member

jonpryor commented Apr 8, 2025

@simonrozsival wrote:

If this is deemed too drastic, we could only disable the feature switch for NativeAOT.

Where we want to eventually be is "no longer support $(XA_HTTP_CLIENT_HANDLER_TYPE) and $(AndroidHttpClientHandlerType )". The question is, how do we get there?

Platform-wise, I currently think it's fine if NativeAOT has no support for $(XA_HTTP_CLIENT_HANDLER_TYPE) and $(AndroidHttpClientHandlerType). Apps are likely to need lots of changes for NativeAOT support anyway; this will be Yet Another Thing, and I'm fine with that.

CoreCLR and MonoVM should support $(XA_HTTP_CLIENT_HANDLER_TYPE) and $(AndroidHttpClientHandlerType) for now.

Also for .NET 10, we should deprecate $(XA_HTTP_CLIENT_HANDLER_TYPE) and $(AndroidHttpClientHandlerType), as a build-time warnings when targeting CoreCLR and MonoVM, and a build-time error when targeting NativeAOT.

Update EnvironmentFileParser.cs to log a warning or error when XA_HTTP_CLIENT_HANDLER_TYPE is encountered:

if (lineToWrite.StartsWith ("XA_HTTP_CLIENT_HANDLER_TYPE=", StringComparison.Ordinal))

GeneratePackageManagerJava.cs will also require some form of update.

We would "similarly" want to default $(AndroidHttpClientHandlerType) to the empty string (unlike now) and have _CheckAndroidHttpClientHandlerType emit a warning or error if it isn't empty:

This will make the change more visible, as build warnings and errors are more visible than runtime messages to adb logcat.

@simonrozsival
Copy link
Member Author

@jonpryor thanks for the feedback. I will revisit the PR later this week and implement what you mentioned.

@simonrozsival simonrozsival marked this pull request as draft April 9, 2025 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants