tmenier / Flurl

Fluent URL builder and testable HTTP client for .NET
https://flurl.dev
MIT License
4.21k stars 388 forks source link

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

Closed tranb3r closed 10 months ago

tranb3r commented 1 year ago

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 commented 1 year ago

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

tmenier commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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. https://github.com/tmenier/Flurl/blob/8dbd2b933dbbd8999e92c29f0a859ae1712d7357/src/Flurl.Http/FlurlRequest.cs#L141

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? https://github.com/tmenier/Flurl/blob/8dbd2b933dbbd8999e92c29f0a859ae1712d7357/src/Flurl.Http/Configuration/FlurlClientFactory.cs#L54

tmenier commented 1 year ago

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 commented 1 year ago

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:

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

tmenier commented 1 year ago

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 commented 1 year ago

@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:

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 commented 12 months ago

@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 commented 12 months ago

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 commented 11 months ago

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.

tmenier commented 11 months ago

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.)