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

[4.0.0-pre5] Application size increase caused by SocketsHttpHandler support #772

Closed
tranb3r opened this issue Oct 23, 2023 · 12 comments
Closed

Comments

@tranb3r
Copy link
Contributor

tranb3r commented Oct 23, 2023

I'm using Flurl.Http in a dotnet maui application.
When updating Flurl.Http from 4.0.0-pre4 to 4.0.0-pre5, the app size increases by more than 800kB (which is a concern).
Here is the breakdown per assembly (only top increment, before compression):

Size difference in bytes ([*1] apk1 only, [*2] apk2 only):
  +     647,928 lib/arm64-v8a/libaot-System.Net.Http.dll.so
  +     322,656 lib/arm64-v8a/libaot-System.Security.Cryptography.dll.so
  +     233,944 lib/arm64-v8a/libaot-System.Net.Security.dll.so
  +     213,981 assemblies/assemblies.blob
  +     213,552 lib/arm64-v8a/libaot-System.Net.Sockets.dll.so *2
  +      40,000 lib/arm64-v8a/libaot-System.Threading.Channels.dll.so *2
  +      39,552 lib/arm64-v8a/libaot-System.Formats.Asn1.dll.so
  +      29,632 lib/arm64-v8a/libaot-System.Net.Primitives.dll.so
  +      20,528 lib/arm64-v8a/libaot-System.Private.CoreLib.dll.so
  +      14,496 lib/arm64-v8a/libaot-System.Runtime.Numerics.dll.so
  +      11,440 lib/arm64-v8a/libaot-System.Net.NameResolution.dll.so
  +       6,600 lib/arm64-v8a/libaot-Flurl.Http.dll.so

As you can see, two new assemblies are packed in my app (System.Net.Sockets.dll and System.Threading.Channels.dll).
However the app is not using the SocketsHttpHandler. So the dotnet trimmer should have gotten rid of it.
Unless it's somehow always used in Flurl, in some static or init code, that is called by my app, even if the functionality is not used.
Any idea?

@tranb3r tranb3r added the bug label Oct 23, 2023
@tranb3r
Copy link
Contributor Author

tranb3r commented Oct 23, 2023

Precision: I'm using the public FlurlClient(HttpClient httpClient, string baseUrl = null) constructor.

@tmenier
Copy link
Owner

tmenier commented Oct 23, 2023

Thank you for reporting this, I agree that's less than ideal and hopefully it's fixable. I'm not very familiar with Maui or the trimming system but I wonder if adding IsTrimmable to Flurl's project file is all that's necessary?

I'm willing to do a new prerelease if you're confident that that's all it'll take, but if you're not sure, I think I'd rather walk you through setting up a simple dummy library locally, have you do some testing until we figure out what works, then I'll take what we learn into a new prerelease. (What I want to avoid is iteratively pushing out prereleases in an effort to debug this. Doesn't seem very efficient. 😉)

Thoughts?

@tranb3r
Copy link
Contributor Author

tranb3r commented Oct 23, 2023

Thank you for reporting this, I agree that's less than ideal and hopefully it's fixable. I'm not very familiar with Maui or the trimming system but I wonder if adding IsTrimmable to Flurl's project file is all that's necessary?

I don't think so. When the IsTrimmable property isn't set, the library is actually Trimmable.

I'm willing to do a new prerelease if you're confident that that's all it'll take, but if you're not sure, I think I'd rather walk you through setting up a simple dummy library locally, have you do some testing until we figure out what works, then I'll take what we learn into a new prerelease. (What I want to avoid is iteratively pushing out prereleases in an effort to debug this. Doesn't seem very efficient. 😉)

I agree.
It shouldn't be difficult to prepare a repro project. I'll let you know when I have it ready. Then we can iterate on the code and figure out how to fix the issue.

@tranb3r
Copy link
Contributor Author

tranb3r commented Oct 24, 2023

Here is a simple repro.
The code is really simple.
To reproduce the issue, first make sure you clean everything (run the clean.bat script), then publish the app (run the publish_android_arm64.bat script). At the end of the build, the linked assemblies will be in the ..\MauiAppFlurlHttpTrimmerIssue\obj\Release\net7.0-android\android-arm64\linked folder.

I've investigated a bit more the code. I think the issue might be caused by combination of several things.

First, even if we provide a client, the code for the clientless pattern is in the path.
This is probably the root cause for the trimmer not removing the code, however the same pattern is used in pre4.

Client ??= FlurlHttp.GetClientForRequest(this);

Second, the DefaultFlurlClientFactory is now creating a different type of HttpMessageHandler depending on the framework. And when building for net7.0-android it will now create a SocketsHttpHandler. I'm not sure if this is what we expect, or even if it's supported, but it's not my point (the goal is still to trim this piece of code).
If the first issue cannot be solved, could we have different implementations of this factory and a way to select the one we want, that would allow the others to be trimmed?

#if NETCOREAPP2_1_OR_GREATER

@tmenier
Copy link
Owner

tmenier commented Oct 25, 2023

Thanks for investigating. I'm open to changing the APIs around this ahead of the final release - I now realize simple platform sniffing isn't sufficient to know whether SocketsHttpHandler can or should be used. I have ideas for making it more opt-in, and hopefully in the process I'll be able to get it out of the code path that's preventing the trimming in your case.

As a side note, can I ask why you're providing your own HttpClient? For 4.0 I'm attempting to minimize the cases where that's needed so I like to have an understanding of cases where it's useful.

@tranb3r
Copy link
Contributor Author

tranb3r commented Oct 26, 2023

As a side note, can I ask why you're providing your own HttpClient? For 4.0 I'm attempting to minimize the cases where that's needed so I like to have an understanding of cases where it's useful.

We have:

  • several HttpClient (the app is targeting several APIs). They are actually registered in DI as named HttpClient.
  • a custom HttpClientHandler (adding certificate pinning)
  • several HttpMessageHandler (for logging and debugging).

We get an HttpClient via DI, and then we create a FlurlClient using this HttpClient.

@tmenier
Copy link
Owner

tmenier commented Oct 27, 2023

Cool, those all sound like things 4.0 will have first-class support for (see #770). What you're doing will continue to work so there might not be a compelling reason to change it, but new projects should find those sorts of things easier to do in 4.0. Thanks for sharing!

tmenier pushed a commit that referenced this issue Oct 27, 2023
@tmenier
Copy link
Owner

tmenier commented Oct 27, 2023

@tranb3r

SocketsHttpHandler is now an explicit opt-in, which should address some of these concerns. But I don't know if it's enough to prevent the trimmer from trimming it. Here's some relevant details:

  • This line hasn't been changed or removed: Client ??= FlurlHttp.GetClientForRequest(this);
  • GetClientForRequest eventually references IFlurlClientBuilder.
  • IFlurlClientBuilder.UseSocketsHttpHandler references SocketsHttpHandler.
  • The default IFlurlClientFactory now never creates a SocketsHttpHandler - that has been broken into a separate factory that is only instantiated if IFlurlClientBuilder.UseSocketsHttpHandler is called.
  • In your case, IFlurlClientBuilder.UseSocketsHttpHandler will never be called in any code path.

Do you think this is enough to solve the original trimmer issue? If so, I can publish a new prerelease. If you're not sure, I would request that you clone or download the repo (dev branch) in its current state and experiment with it locally. Thanks!

@tranb3r
Copy link
Contributor Author

tranb3r commented Oct 29, 2023

@tmenier
I've tested your dev branch with my repro project, and the trimmer issue is gone.
So, I guess you can now publish a new prerelease.
Thanks a lot!

@tmenier
Copy link
Owner

tmenier commented Oct 29, 2023

Excellent news! thank you so much for your help with this. I try not to over-use the "help-wanted" label but it's definitely nice to lean on folks in cases where I'd need a significant change to my local dev environment (MAUI in this case). Thanks again!

@tranb3r
Copy link
Contributor Author

tranb3r commented Nov 5, 2023

I've tested pre6 on my app, the issue I was intially reporting is now gone.

One more word.
I've been using Flurl for many years now. It's always been a pleasure to work with you.
Thanks a lot for bringing lots of improvements to this great lib while taking care of users' feedback.

@tranb3r tranb3r closed this as completed Nov 5, 2023
@tmenier
Copy link
Owner

tmenier commented Nov 5, 2023

Thanks for the kind words. It's users like you who not only report issues (which is always better than nothing!), but also actively help figure out a solution, that makes this project so great to work on.

(Reopening until the final release, which might be an unusual flow but fits how I have my project board organized.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Released
Development

No branches or pull requests

2 participants