feature: Refit should be linker-friendly and support trimming #1389
Comments
|
Hey @clairernovotny! 👋 I was thinking we could do pretty much what we did back in #836 and following PRs, where we added NOTE: this would only be a binary breaking change. Consumers bumping the NuGet reference and rebuilding from source should not actually encounter breaking changes, because the generators would just generate all updated code on the spot. Let us know how we can proceed with this! We really think adding proper trimming and reflection-free support for Refit would be a huge win for a ton of consumers. Especially now that starting from .NET 7 (see here), NativeAOT builds will try to trim everything by default, which means people publishing and referencing an assembly using Refit will start to see more linker warnings now, because the compiler will attempt to trim everything. And unless they changed that back, they'd now also have runtime issues, because Refit just breaks down with trimming enabled. Thank you! 🙂 |
|
This feature would be great when used in conjunction with a source generator based DI system like https://github.com/YairHalberstadt/stronginject. It would make Refit usable in embedded applications which benefit heavily from NativeAOT compilation.
|
|
Microsoft: "Please rewrite your entire library and spend hours of your limited free developer time, because we broke it. Again. Users will receive absolutely no new feature or benefit as a result of this work, they will simply have exactly what they did before. Oh, and we also plan to include breaking changes in your library." I recommend users disable NativeAOT. |
|
I think there's some misunderstanding here and some incorrect statements, and also some frustration towards Microsoft that I'm not really sure is warranted in this specific case. Let me try to clarify things here.
This was just me proposing an improvement. I wasn't speaking for Microsoft, and I don't think I ever said that that was the case here. This was just something I wanted to start a conversation about, as I'm personally a fan of refit 🙂
Nobody said anyone from the refit team should rewrite the entire library. In fact, @kant2002 already said he'd like to help work on this, and I also said I'd be happy to provide him with assistance.
Nobody broke anything. Refit still works just fine, it's just very inefficient and doesn't support trimming, but that has always been the case.
This is just clearly false and also very easy to disprove. I already outlined the improvements for end users that these changes would being. The library as a whole would be faster, it would give them the ability to manually inspect the code being used and improve their debugging experience, it would make the whole library trimmable and it would also make it much easier to use it on AOT scenarios. Those are valid benefits for end users. "Oh, and we also plan to include breaking changes in your library" I already talked with @clairernovotny internally and she said this would be fine, which is the reason why @kant2002 had already started some preliminary work. The breaking changes would go into a new major version, so customers could also choose to just remain on the previous one until they're ready to migrate. This is exactly what we did when I added the System.Text.Json support to refit and we later decided to make it the default and pull Newtonsof.Json out and in a separate package.
This is just poor advice, and it's unwarranted. Let me clarify things some more as well. I really like refit, and as I've mentioned I've also already contributed to it several time in the past. The reason why I'm proposing this is precisely because I like this project, and I'd love to see it improve, address the architectural issues it currently has, and become the best HTTP REST library it can possibly be. I've opened this proposal because I wanted to start a conversation about this and also allow @kant2002 to get in touch with the maintainers and get assurances that if he had done work it wouldn't have been blocked after the fact (which is why we reached out to Claire). This post wasn't meant as a critique on anyone nor as a demand on anyone from the team to do work for free 🙂 |
|
As person who want this feature. Is this would be accepted if I implement this? |
Do you have any data to prove that, in real-world scenarios involving actual network requests, that this makes any significant difference?
This is Curious because Refit actually uses source generators because of Xamarin AOT compilation requirements on iOS, and there it has worked for nearly a decade without issues. Why is this different? I think it is more appropriate to take a wait-and-see approach on this one, rather than radically rewriting how Refit works (for the third time!), in order to chase the latest Microsoft feature. If this is still causing users grief in a year, I think it's worth converting it. |
Yes, I'd you're doing few very long requests the relative impact would of course be minimal, but as I mentioned that wouldn't be the main point (though for apps doing tons of smaller requests it could still be noticeable). The main benefit would be supporting trimming, improving debugging, proper AOT support, and less impact on the IDE performance (this last one is a separate point, but it would be another benefit of a better source generation architecture).
Using source generators alone is not a magic wand that automatically makes code trimmer-safe or AOT-friendly. Refit uses source generators, yes, but the architecture is pretty suboptimal: the generated code is just a few very thin stubs that just forward all the logic back into the library, which then uses some admittedly pretty extreme reflection (and comments in the code do say as much) to make things work. As in, refit is not using source generators in a way that a truly source generator first architecture would. As a result, the code can work in some AOT scenarios, but it's still fundamentally inefficient, not AOT-friendly and doesn't support trimming at all (that's both due to the architecture, and the fact it's not even annotated). For instance, if you use refit on AOT and enable trimming, it will not work at all and it will just crash at runtime.
To clarify: this is not something new per se. As I mentioned, refit has been broken with trimming on UWP for years already. The idea is just that now that better source generator APIs are available, trimming is more widespread, trimming annotations are available, NativeAOT is officially supported, it seems like a good time to start looking into whether this would be doable 🙂 (Which it is, it just requires some work). |
....you know that I wrote this library right? Why are you explaining my own code to me |
I... Just answered your question 🥲 I'm aware you wrote most of this library, but I'm not sure how you expected me to be able to answer that without having to underline some bits of how the codebase is structured versus what is being proposed here, which is what I did. If you meant that you already knew all of this too, well then I'm afraid I don't understand why you even asked that question in the first place. |
|
My motivation to work on this issue is following. I would like to have better ecosystem which cares about NativeAOT + reflection-free mode. Reflection-free mode is experimental mode for NativeAOT, so you may think about it as niche within niche. Even if that's niche, whether possible I try to contribute back to existing libraries. Based on my observations, most code works just fine even in this mode, if augment things here and there with source generators (which I use like Reflection at compile time). Usually this results in the all dynamic code to be generated by sourcegen and visible to developers. That helps trimming technology within dotnet reason about things and end-user (developer) do not have to think about how to massage things to please trimming. I would say this also allows other developers to better reason about library, but that's maybe my opinion. Who cares about this subniche. I would say game developers, hot-heads like me, embedded developers and quite possible cloud developers. Game developers and embedded developers obviously like squeezing water from stone, so this case is definitely good for them. Cloud developers would have faster startup time so for some workflows that's allow them save money. I do not expect huge adoption of NativeAOT (default mode) in near future so if you drive this library purely based on demand, then I understand why you would like to wait and see. My experience is that unrestrained usage of reflection may cause issue for end-developers and complicate their lives. I do want that working with NativeAOT would be as pleasant as with CoreCLR, so that's why I'm going to libraries and try to fix things here and there. If you think that's not important for you, I can understand that. No skin of my back. Then I have to create something similar but without reflection myself. Either way, you may see that I do not super speedy in writing code for this issue. If there chance that I may persuade you extend support of your for these niches I prefer that way. |
Well, this is a surprisingly hostile response to a feature request... NativeAOT is an opt-in technology, but a valuable one in certain contexts where a JIT is just too slow or completely unavailable. For the moment, if an application requires NativeAOT, it simply cannot use Refit, which sucks once you're used to how nice Refit makes working with APIs. Going back to HttpClient boilerplate is a pill. The reason this issue exists at all is because we like Refit and we want to use it in even more scenarios. It's a request to collaborate, not to ask you to do all the work, but to focus community effort towards this feature because it is an open source project and it makes all of our development experiences better. I really hope you consider re-opening this issue because true reflection-less Refit would be a massive boon to many. |
|
In isolation, I would not be so hostile to this request - it is Annoying, but Reasonable. However, having authored many C# libraries and having things like this happen over and over and over, it is frustrating, both as a user of C# who Wants It To Be Good, and as a maintainer. .NET decides to break the world, and then eagerly rushes to convince everyone to get on board. The ecosystem gets left in a fractured mess over and over, as maintainers throw out all of their working, production-ready code and start again. Why is it okay to ship NativeAOT with such massive breaking changes? Xamarin shipped AOT almost a decade earlier and absolutely bent over backwards to still support existing code. Why? Because not breaking the ecosystem matters. And in fact, Refit works just fine with Xamarin AOT on iOS, arguably the most hostile AOT environment that exists! Even the suggestion of "Just major version it bro" like it magically waives away all compatibility issues is really frustrating to me - now anyone who has a shared library using Refit, who tries to integrate the latest version to their app, is completely stuck. This is not an edge case, this happens all the time in corporate environments. But instead, we are now going to break all of those people. That seems Bad! As a developer and maintainer, I am very much in the Raymond Chen camp of software design - Don't. Break. Software, unless it's really really really worth it. This is a part of why people, after all these years, still use things like Refit, and ReactiveUI, and Squirrel.Windows - because they don't constantly break people. |
|
Hello everyone! I wanted to inquire about the current status of this matter. Previously, there seemed to be a significant community interest, with several individuals expressing their willingness to contribute by submitting pull requests. However, it has come to my attention that the pull request from @kant2002 has been closed. Does this indicate a lack of interest from the library owners? Personally, I strongly believe that incorporating this feature and marking library with |
|
My use case is getting Refit into industrial .NET applications which are often targeted towards low power ARM SoCs, usually single core and around 1GHz in speed. .NET is surprisingly performant in steady state, but JIT pauses and deployment size are troublesome. ReadyToRun is an alright stopgap solution but NativeAOT performs excellently and is really the end goal. I'm also curious as to why #1414 was closed. If implementing full source generation is considered too disruptive to the project goals, would it be more feasible to fork/branch off into a separate package which is a ground up source generator first implementation? |
|
I'd also find AOT support extremely helpful. I'm deploying several web apis to Azure which use Refit to communicate with each other and other external apis. Since this is a private project and I need to keep my Azure bill low I've set the web apps to shut off when they're inactive. It would be really awesome if Refit as such an integral tool for so many .Net projects would support this new feature. |
I have develop new Refit that support AOT. Nuget package name is HttpClientFiller https://www.nuget.org/packages/HttpClientFiller
|
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@ghostnguyen @bugproof This conversation has nothing to do with Refit, please take it to a more appropriate venue. |
|
Lack of trimmer support is the reason I stopped using Refit after my first .NET project years ago. For anything with a UI, my platform of choice is UWP/Uno Platform, and they both make good use of AoT:
I'd like to use Refit again, but I can't until refit is trimmer-friendly. |
You can try this library, this Refit like and AoT support.
|
|
I would like to remind you that using [ModuleInitializer] can prevent the proxy class from being trimming, see HttpApiProxyClassInitializer.WebApiClient now has better performance in design and performance. |
|
Hey, would you still be open to me doing source generator rewrite? I feel like there are some major performance and usability gains to be made with generated code. While it should be possible to remove all reflection from the internal logic, it looks like several configuration interfaces rely on reflection types ie Aside from that the only public changes would be the removal of public interface IGithubApi
{
[Get("/users/{user}")]
Task<User> GetUser(string user, [Header("Authorization")] string authorization, [Query(CollectionFormat.Multi)]int[] ages);
}
// Generated code
public Task<User> GetUser(string user, string authorization, int[] ages)
{
var req = new HttpRequestMessage() { Method = HttpMethod.Get };
req.Headers.Add("Authorization", $"Authorization: {authorization}");
// could use `ValueStringBuilder` here with a little trickery
var sb = new StringBuilder();
sb.Append("/users/");
sb.Append(settings.UrlParameterFormatter.Format(user, null, typeof(string)));
sb.Append('?');
var key = settings.UrlParameterKeyFormatter.Format("ages");
Helpers.AddQueryCollection(sb, key, ages, settings);
req.RequestUri = new Uri(sb.ToString());
// send, handle errors and serialize etc
} |
Of course, the PR you did (#1710) is better than nothing, but it adds the entire generated code as an exception. This means that if there is an API call that you don't use, the generated code for it will still be preserved rather than trimmed. The author suggested using the source code generator, not just to generate a thin reflection proxy, but to generate the code that is currently done at the runtime level. This would have a very positive effect on the library. |
You can check the generate code in this library to see its working
|
Yes, this library does fully support 'generate the code that is currently done at the runtime level'
|
|
Born for AOT, but abandoned the support of many lower versions of .NET, and the scalability is almost zero, why bother? |
|
I don't understand why this issue has been closed. Refit is, as of 7.1.1, still completely not trim-safe nor AOT-safe. Just try it yourself by creating this minimal project: using Refit;
using WebApis;
var gitHubApi = RestService.For<IGitHubApi>("https://api.github.com");
Console.WriteLine(await gitHubApi.GetUser("sergio"));
Console.WriteLine(await gitHubApi.GetUsers());
namespace WebApis
{
public interface IGitHubApi
{
[Get("/users/{user}")]
Task<User> GetUser(string user);
[Get("/users/")]
Task<User[]> GetUsers();
}
public sealed record User(string Name, string Id, string Email);
}Then publish with AOT. You'll get 0 warnings in the IDE, but all the following warnings at publish time, from ILC: @glennawatson to clarify: Refit is not at the time being trim-safe nor AOT-safe whatsoever. |
|
I believe Github automatically closed this issue so I have reopened it. Let's come up with a solution for this, although the request is valid, the solution is not so simplistic. |
It depends:
This is what I've mentioned before as well. If maintaining complete backwards compatibility is a primary concern and we don't want to introduce any breaking changes even in a new major upgrade, that's fine, however we should at least add the necessary annotations to the library so that consumers would get the right warnings right in the IDE, instead of only seeing them at publish time. That will result in a much better developer experience. Put simply, we need to add a .NET 8 TFM to Refit, toggle To be clear, this would not make the library trim/AOT-safe. As I said, I don't think that's doable without some breaking changes. But it would, however, make it clearly incompatible with trimming and AOT, which at least gives you well defined behavior. |
|
Thank you @Sergio0694 the Net 8.0 TFM is already in place, next step AOT Annotation. Once this is in place an AOT Safe version could be produced for Net 8, depending on how wildly different it would need to become we may need to consider a separate Refit.AOT package to ensure the existing features are maintained too; there's too many people using Refit to make a big breaking change that results in everyone having to rewire and retest their applications. |
|
That sounds like a very reasonable plan to me 🙂
|
|
I have created a fully support AOT, no reflection and almost identical to Refit's usage. |
|
When I last time look at it make AOt compatible require a bit of refactoring. #1414 That was first direction. Basically you should extract AOT compatible subset of API or create new helpers in Refit so source generators can leverage that. I would start with having model completely independent from Relection |
Is your feature request related to a problem? Please describe.
Yes. The issue is that despite Refit uses source generators, the generated code is just a thin proxy back to the reflection-powered functionality in the main library. This means that using Refit with trimming enabled completely breaks it. As it is, there's not much value in having source generators. They should be updated so that all the reflection-powered logic is executed at compile time, and the generated stubs will just have all the logic that is today done at runtime by the method builder APIs. This library should effectively require no runtime reflection at all (or, virtually none, but especially none that can break trimming).
Describe the solution you'd like
As mentioned, the source generator should be updated so that instead of just calling back into the method builder and invoking that reflection-based callback to perform the request, it'd instead emit all the same logic that's today done at runtime. That is, all the various marshalling of parameters, request building etc. should just be done in a statically-analyzeable way and generated at compile time. The library should just expose helper APIs for the generator to use to construct the individual bits of a request/response.
All the information that the method builder is using at runtime is there at compile time too anyway. That logic should just essentially be inverted, and the generator APIs should be used to achieve static reflection, instead of runtime reflection like today.
If there's any reflection left, additionally, all those APIs should be properly annotated.
Refit as a whole should have the trimming analyzers enabled when on the .NET 6+ target.
Describe alternatives you've considered
Runtime directives can be used to patch the problem. This is extremely cumbersome, hard to do especially for beginners, and causes an unnecessary binary size increased, given you'd be forced to preserve way more stuff than strictly needed, to be safe.
Describe suggestions on how to achieve the feature
The text was updated successfully, but these errors were encountered: