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

Microsoft.AspNetCore.Hosting.ListenOptionsHttpsExtensions without reflection ? #31561

Closed
LLT21 opened this issue Apr 6, 2021 · 56 comments
Closed
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-kestrel NativeAOT
Milestone

Comments

@LLT21
Copy link

LLT21 commented Apr 6, 2021

I have tried to compile my .NET 5 bare http server project with Native AOT without reflection. This is possible for a normal http connection, but not for a https connection because in Microsoft.AspNetCore.Hosting.ListenOptionsHttpsExtensions the line of code:

var loggerFactory = listenOptions.KestrelServerOptions?.ApplicationServices.GetRequiredService<ILoggerFactory>() ?? NullLoggerFactory.Instance;

that appears twice in this class requires reflection. Could it be changed to:

var loggerFactory = listenOptions.KestrelServerOptions?.ApplicationServices?.GetRequiredService<ILoggerFactory>() ?? NullLoggerFactory.Instance;

so that it has the ? after ApplicationServices. In this way I believe the reflection can be avoided.

@davidfowl
Copy link
Member

This must be the tip of the iceberg. We can change small things if people send pull requests but without a way to stop future regressions with tests it'll keep breaking in the future

@BrennanConroy
Copy link
Member

Like David said, until we have AOT testing we wont be able to make any significant improvements for AOT environments.

If you would like to send a PR we can accept that, otherwise we wont be fixing this issue specifically right now.

Also, what was the actual error you were getting? A nullref?

@BrennanConroy BrennanConroy added the help wanted Up for grabs. We would accept a PR to help resolve this issue label Apr 7, 2021
@BrennanConroy BrennanConroy added this to the Backlog milestone Apr 7, 2021
@ghost
Copy link

ghost commented Apr 7, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@LLT21
Copy link
Author

LLT21 commented Apr 8, 2021 via email

@davidfowl
Copy link
Member

I don't think it will.

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@davidfowl
Copy link
Member

Instead of doing this:

kestrelServerOptions.ApplicationServices = new ServiceCollection()
                                                         .AddLogging()
                                                         .BuildServiceProvider();

Implement a custom IServiceProvider that does this:

private class ServiceProvider : IServiceProvider
        {
            public object GetService(Type serviceType)
            {
                if (serviceType == typeof(ILoggerFactory))
                {
                    return NullLoggerFactory.Instance;
                }

                return null;
            }
        }

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@davidfowl
Copy link
Member

davidfowl commented Apr 11, 2021

Will this avoid then using the dependency injection ? Because that seems to be the problem to natively compile without reflection.

Yes, you're basically implementing your own IServiceProvider and it can do whatever you want. Like manually create objects.

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@davidfowl
Copy link
Member

You can use the HttpContext directly in your application as well. You might want to use something like https://github.com/benaadams/Ben.Http which wraps these Kestrel APIs and lets you create the server directly.

cc @benaadams you might need to test https with your server,

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@davidfowl
Copy link
Member

This solution reacts exactly the same with NativeAOT: it should be able to avoid the call in ListenOptionsHttpsExtensions after ApplicationServices var loggerFactory = listenOptions.KestrelServerOptions?.ApplicationServices?.GetRequiredService() ?? NullLoggerFactory.Instance;

I don't understand. There's no reflection here

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@davidfowl
Copy link
Member

@LLT21 Put the project on github with instructions and I'll try to get it working with no reflection model. I'm not understanding why the code I gave you doesn't work.

cc @MichalStrehovsky

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@davidfowl
Copy link
Member

Yes and just paste it here, but also with instructions on how you got those errors.

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@davidfowl
Copy link
Member

I'm going to assume windows vs OSX doesn't matter but we'll see.

@davidfowl
Copy link
Member

Yep, I can reproduce the issue on windows as well.

@LLT21
Copy link
Author

LLT21 commented Apr 11, 2021 via email

@MichalStrehovsky
Copy link
Member

Resource strings are used all over the code base that doesn't work at all.

I think this would be solvable - the framework also has resource strings all over the place, but it offers a mode where all of that can be stripped - ResourceManager gets out of the picture, along with all the resource strings. It's the documented UseSystemResourceKeys setting. ASP.NET could also add support for that. This is generally useful and not AOT specific.

I'm assuming resource strings in ASP.NET are used for the same reason as in the framework (exception messages) and they're not essential.

The bigger picture

The reflection free mode in NativeAOT is very aggressive - I wouldn't really expect it to be useful for much more than simple command line apps.

It's more of a tech demo of how the NativeAOT runtime can operate without any of the traditional .NET metadata and still run C# code.

With the trimming annotations we're adding in .NET 5/6, NativeAOT compiler is going to be in a better position to reason about what needs to be reflectable within the app. The compiler currently assumes that whatever was compiled, also needs to be reflectable - that's why there's a pretty big size/memory difference in a Hello World compiled with default settings vs in the reflection-free mode.

We already run the equivalent of the IL Linker trimming analysis in the Native AOT compiler, but it currently only makes sure we include extra code/data on top of what was discovered statically. I'm hoping in the coming weeks I'll get a chance to sit down and tweak this so that we have a mode where only the results of the trimming analysis are reflectable. I think this mode will get us pretty close to the reflection-free numbers, but will be immensely more useful for workloads like ASP.NET.

@LLT21
Copy link
Author

LLT21 commented Apr 14, 2021

I would like to do some further testing with my custom build, but have problems with the nuget configuration. The steps I took on OSX:

  1. Clear all previous .NET SDK's and runtimes

  2. Install .NET 6 preview 3: SDK 6.0.100-preview.3.21202.5 and runtimes 6.0.0-preview.3.21201.13 (AspNETCore) and 6.0.0-preview.3.21201.4 (NETCore)

  3. I cloned the git repository release/6.0-preview3 and built that one in debug on mac - worked fine; release had some issues with large files or so, but continued with debug

  4. The problem where I am stuck is the nuget reference. Tried 3 options:

4a. <FrameworkReference Include="Microsoft.AspNetCore.App" Version="6.0.0-dev" /> in project file + I replaced the
usr/local/share/dotnet/
content with the
.../artifacts/installers/debug/aspnetcore-runtime-6.0.0-dev-osx-x64/
content; this gives as a message when starting in VSCode:

It was not possible to find any compatible framework version
The framework 'Microsoft.AspNetCore.App', version '6.0.0-preview.3.21201.13' was not found.

  • The following frameworks were found:
    6.0.0-dev at [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]

4b. Added the dll's generated in .../artifacts/installers/debug/packs/Microsoft.AspNetCore.App.Ref/6.0.0-dev/ref/net6.0 to a subfolder net60 of my project and then tried to link the dll's in the project file with

<Reference Include="Microsoft.AspNetCore.Server.Kestrel.Core">
  <HintPath>net60/Microsoft.AspNetCore.Server.Kestrel.Core.dll</HintPath>
</Reference>

This gives a BadImageFormatException: "Cannot load a reference assembly for execution."

4c. Tried to add the compiled AspNetCore via nuget:

LLT@lmp tinykestrel6 % dotnet nuget list source
Registered Sources:

  1. aspnet [Enabled]
    /Users/LLT/Projects/microsoft/aspnetcore6p3/aspnetcore/artifacts/packages/debug/shipping/package
  2. aspnet6 [Enabled] /Users/LLT/Projects/microsoft/aspnetcore6p3/aspnetcore/artifacts/installers/debug/packs/Microsoft.AspNetCore.App.Ref/6.0.0-dev/ref/net6.0

In combination with for the involved dll's:

<PackageReference Include="Microsoft.AspNetCore.Server.Kestrel.Core" Version="6.0.0-dev">

This gives me an error:
error NU1101: Unable to find package Microsoft.AspNetCore.Server.Kestrel.Core. No packages exist with this id in source(s): aspnet, aspnet6

I am missing some info here how exactly to include the compiled AspNetCore libraries in my Visual Studio Code project. Probably because working with nuget on local projects is the first time I try.

@davidfowl davidfowl removed the help wanted Up for grabs. We would accept a PR to help resolve this issue label Apr 24, 2021
@davidfowl
Copy link
Member

I would just use the DLL and do <Reference Include="PathToAssembly" />

@LLT21
Copy link
Author

LLT21 commented Apr 25, 2021 via email

@davidfowl
Copy link
Member

@LLT21 Excellent work!

For the CoreStrings I tried with the parameterized compilation proposal of Michal, but that didn't work on my side. Maybe because sometimes there are values inserted in the strings ?

@MichalStrehovsky are there any guides on how to add support for "System.Globalization.Invariant" mode? Are you saying we would need to have a switch in our code to check for the AppContext setting and skip resource manager when that was set? Seems like something we could just bake into Resgen. How does the runtime implement this? Are there any pointers?

@LLT21

The logger changes look easy to fix here without NativeLogger, so that's not a big deal but I fear that pattern is used pervasively and I would prefer if it were possible to fix the Logger<T> implementation. Though I'm not sure if that's possible (any ideas here @MichalStrehovsky ?) without runtime or compiler changes. The I could see an intrinsic like nameof(T) that would allow resolving the full name of the T at AOT/JIT time.

That said, I don't have a problem making this change in Kestrel with a comment specifying that there's a narrow "no reflection" scenario for AOT here that this enables to keep it from regressing.

As for JSON, we have a new source generator that you'll want to try out here that will allow you to keep using the JSON serializer but doesn't depend on reflection based metadata. (cc @layomia)

I don't have any comments about SQL or ODBC but I'll cc @roji as an FYI.

Finally, we don't have any tests for this scenario so it'll likely regress in strange ways until we have some basic scenario checked in that we run (this is similar to how we do handling linking managed code today).

In the end, this is a very cool experiment and one that I'm happy to make changes (within reason) to keep it working for your scenarios. It'll also teach us more about AOT 😄

PS: There's some code clean up you can do, I'll send a pull request.

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Apr 26, 2021

The libraries implementation of UseSystemResourceKeys property you can enable in the CSPROJ boils down to basically this:

  1. The class that handles resource reading in all framework assemblies has a UsingResourceKeys method that is implemented like this:

https://github.com/dotnet/runtime/blob/207b03a073107e63df0b67056dd596226065651e/src/libraries/Common/src/System/SR.cs#L11-L46

The "normal" implementation keys off the System.Resources.UseSystemResourceKeys AppContext switch.

  1. There's an IL Linker descriptor defining a feature named System.Resources.UseSystemResourceKeys. This file gets injected as an embedded resource into all framework assemblies.

https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/eng/ILLink.Substitutions.Resources.template#L1-L10

What this descriptor does is that when IL Linker runs and this feature was specified as "enabled", illink will replace the method body of UsingResourceKeys to just return true. It will also enable dead code elimination at the callsites of UsingResourceKeys. Linker will also eliminate the managed resource that has the actual resource strings in it. Note that the linked file is a template. We have msbuild magic that replaces the tokens in this file with actual assembly names.

  1. The SDK has code that translates the UseSystemResourceKeys property in the csproj into two things:
  • Lines get injected into runtimesettings.json to set the appcontext switch.
  • ILLink gets the extra argument to consider UseSystemResourceKeys as an enabled feature.

This way one gets consistent experience whether illink runs or not (the appcontext switch disables the resources support if we didn't run linker, e.g. for dotnet run).

If ASP.NET adds 1 and 2 and uses the same names for the AppContext switch and illink feature name, the 3 should just fall into place nicely.

@MichalStrehovsky
Copy link
Member

Forgot to respond to the other parts.

First of all, @LLT21 - this is great work - I'm pleasantly surprised this works with Kestrel and DB and JSON with so little changes!

I have no plans to remove the reflection-free mode - at minimum it's great for experimenting with the compiler and I want to keep it.

In chatting with David about this, I realized this is still keeping some unnecessary metadata around - to get rid of the last bits of metadata, add <IlcGenerateStackTraceData>false</IlcGenerateStackTraceData> to a propertygroup in your CSPROJ. This will remove the metadata used to generate textual stack traces when an exception is thrown. You'll get offset instead of method names in exception stack traces (e.g. Exception.ToString). You can use the PDB to decode the offsets into names. This tool should probably work: https://github.com/dotnet/corefx-tools/tree/master/src/StackParser

@LLT21
Copy link
Author

LLT21 commented Apr 26, 2021 via email

@kant2002
Copy link
Contributor

@LL21 it was not obvious from your example initially. But if you do not use HTTPS then seems to be everything would be working without any patches. So if SSL would be handled by IIS or Nginx then you can use just previews.

I think this is exciting. Technically from your starting point, you can have small MVC framework utilizing source generators for routing and for model binding.

@LLT21
Copy link
Author

LLT21 commented Jun 1, 2021

@kant2002 Sorry for not replying earlier, but I have been extremely busy with implementing a whole TechEmpower benchmark framework using the NativeAOT in a reflection free mode, with ODBC as an interface to PostgreSQL.
While implementing this, I noticed that you can just use the previews for non https (as I do in the TechEmpower benchmark framework), except if you need kestrelServerOptions.Listen(IPAddress.Any, 8080); instead of kestrelServerOptions.ListenLocalhost(8080); then at the exit of the program you will also get a Disabled Reflection error; it is not a real showstopper, but could be nicer.

Now regarding the TechEmpower benchmark, as I am a first time contributer, I am looking for someone who could approve my pull request which you can find here: TechEmpower/FrameworkBenchmarks#6614. Don't know what the normal process is for this, so if anyone of you would have the necessary rights, I would appreciate the review.

Last question @davidfowl : do you expect me to make the changes in a pull request for this open issue or is it already planned ? I am not fully comfortable with this, but I can try if you like.

@roji
Copy link
Member

roji commented Jun 3, 2021

@LLT21 thanks for this super interesting work.

with ODBC as an interface to PostgreSQL

Am curious... Have you given Npgsql a try instead of ODBC? There's definitely some reflection and runtime codegen in current released versions which could be problematic, but I've worked on removing those for 6.0 (see npgsql/npgsql#3300); Npgsql 6.0.0-preview4 already includes these changes.

@LLT21
Copy link
Author

LLT21 commented Jun 3, 2021

@roji Thanks for the feedback. I was not aware of the work in progress on Npgsql regarding this topic. Otherwise it could have saved me some sleepless odbc nights. Now that the implementation is there, I would like to first let the odbc test run and once these results are available, then compare with Npgsql. If performance is similar or better with Npgsql, I would certiainly favor the latter as it is much easier to implement. However, at the moment I am a bit confused how to proceed with the TechEmpower Benchmarks. According to this link TechEmpower/FrameworkBenchmarks#6614, a maintainer has to approve the running of workflows for first-time contributors (which I am). I cannot find a list of maintainers or how to get in contact. Do you have any idea ?

@MichalStrehovsky
Copy link
Member

The TechEmpower people are maintainers. I don't have a separate channel to them - not sure if anyone on this issue has. They look at these things but sometimes it takes weeks to add a new thing (clicking through other PR history). Updates get merged faster.

@LLT21
Copy link
Author

LLT21 commented Jun 4, 2021

@MichalStrehovsky The flow has started, so I am happy it goes this fast. Thanks for your feedback.

@LLT21
Copy link
Author

LLT21 commented Jun 4, 2021

@MichalStrehovsky One more question Michal: could you have a quick look at the project settings in https://github.com/LLT21/FrameworkBenchmarks/blob/appmpowerv1/frameworks/CSharp/appmpower/src/appMpower.csproj whether these are all optimal for fastest execution ?
@roji Next week I will try whether I can convert and compile with disabled reflection and the version 6 npgsql

@roji
Copy link
Member

roji commented Jun 4, 2021

Next week I will try whether I can convert and compile with disabled reflection and the version 6 npgsql

@LLT21 great. If you still run into Npgsql issues, we can try to iterate quickly to get to a point where it works.

@MichalStrehovsky
Copy link
Member

whether these are all optimal for fastest execution ?

Yup, these all look good! Looking forward to see the results!

@roji
Copy link
Member

roji commented Jun 6, 2021

@LLT21 if and when you get around to looking at Npgsql, check out npgsql/npgsql#3813 (comment). We've also added some tasks to npgsql/npgsql#3300, and there's a pretty good idea of what needs to be done (thanks to @NinoFloris).

@LLT21
Copy link
Author

LLT21 commented Jun 8, 2021

@roji I added today a repository https://github.com/LLT21/appmpower-npgsql with therein a class RawDbNpgsql. In the HttpApplication class there are conditional compilation statements for Npgsql or Odbc. In the RawDbNpgsql class I have also foreseen some Console.WriteLine statements to see up to where the native compilation goes. At the moment it ends before the creation of the new NpgsqlConnection, but I noticed in your repository you still did some recent changes I have not integrated yet. Feel free to test with the https://github.com/LLT21/appmpower-npgsql repository if you want.

@roji
Copy link
Member

roji commented Jun 8, 2021

Yeah, we're working on it :) I'll ping you once we have something which should work.

@davidfowl
Copy link
Member

We have no plans to make no reflection mode work.

@davidfowl davidfowl closed this as not planned Won't fix, can't repro, duplicate, stale Oct 20, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 19, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-kestrel NativeAOT
Projects
None yet
Development

No branches or pull requests

9 participants