Running ASP.NET Core application with PublishTrimmed set in visual studio makes Microsoft.AspNetCore.Watch.BrowserRefresh fail to execute

davidfowl commented 3 years ago

Describe the bug

When <PublishTrimmed> is set in the csproj, causes Microsoft.AspNetCore.Watch.BrowserRefresh to fail. I'm guessing hosting startups will fail when trimmed but I'm surprised this affects the developer experience.

To Reproduce

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <Nullable>enable</Nullable>
    <PublishTrimmed>true</PublishTrimmed>
  </PropertyGroup>

</Project>

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.MapGet("/", () => "Hello World!");

app.Run();

Exceptions (if any)

crit: Microsoft.AspNetCore.Hosting.Diagnostics[11]
      Hosting startup assembly exception
      System.InvalidOperationException: Startup assembly Microsoft.AspNetCore.Watch.BrowserRefresh failed to execute. See the inner exception for more details.
       ---> System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.AspNetCore.Watch.BrowserRefresh, Culture=neutral, PublicKeyToken=null'. The system cannot find the file specified.
      File name: 'Microsoft.AspNetCore.Watch.BrowserRefresh, Culture=neutral, PublicKeyToken=null'
         at System.Reflection.RuntimeAssembly.InternalLoad(ObjectHandleOnStack assemblyName, ObjectHandleOnStack requestingAssembly, StackCrawlMarkHandle stackMark, Boolean throwOnFileNotFound, ObjectHandleOnStack assemblyLoadContext, ObjectHandleOnStack retAssembly)
         at System.Reflection.RuntimeAssembly.InternalLoad(AssemblyName assemblyName, RuntimeAssembly requestingAssembly, StackCrawlMark& stackMark, Boolean throwOnFileNotFound, AssemblyLoadContext assemblyLoadContext)
         at System.Reflection.Assembly.Load(AssemblyName assemblyRef)
         at Microsoft.AspNetCore.Hosting.GenericWebHostBuilder.ExecuteHostingStartups()
         --- End of inner exception stack trace ---

Further technical details

ASP.NET Core version: Microsoft.AspNetCore.App 6.0.0-rc.1.21406.15

cc @pranavkm @eerhardt

eerhardt commented 3 years ago

I debugged this a bit today. The reason this is broken is because we disable Startup Hooks when PublishTrimmed=true. See https://github.com/dotnet/runtime/issues/36526

When PublishTrimmed isn't set, the "Microsoft.AspNetCore.Watch.BrowserRefresh" assembly is already loaded in the process by the time the following code is invoked:

https://github.com/dotnet/aspnetcore/blob/545d4eece7fcc5a67b27f6ba96636d9f476741ae/src/Hosting/Hosting/src/GenericHost/GenericWebHostBuilder.cs#L139-L145

So loading the assembly works, because it is already loaded.

When PublishTrimmed=true is set, Startup Hooks are disabled, which means the assembly isn't already loaded. And trying to load the assembly fails with "FileNotFoundException".

I'm guessing hosting startups will fail when trimmed but I'm surprised this affects the developer experience.

This is an intentional decision with PublishTrimmed. The intention is that dotnet run and dotnet publish apps should execute the exact same. Since Startup Hooks can't work with trimmed apps, we disable the feature by default when trimming. There are a number of other features that are disabled by default, see:

https://github.com/dotnet/sdk/blob/c3850ddbd2c92702cf62868d0614d35a1278f7bb/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets#L31-L47

We recommend app devs set PublishTrimmed=true in their apps if they intend on trimming - see https://docs.microsoft.com/en-us/dotnet/core/deploying/trim-self-contained#trim-your-app---cli. This gives the most consistent behavior between F5/run and publish.

The ASP.NET SDK (or dotnet watch) can re-enable StartupHookSupport in MSBuild if it requires Startup Hooks to work at development time. Maybe based off Configuration=Debug?

cc @agocke @vitek-karas @sbomer @mateoatr

mkArtakMSFT commented 3 years ago

@davidfowl need your help to suggest what needs to happen and where. Thanks!

davidfowl commented 3 years ago

We probably need to enable this on launch from watch and from visual studio.

cc @vijayrkn @BillHiebert

pranavkm commented 3 years ago

The ASP.NET SDK (or dotnet watch) can re-enable StartupHookSupport in MSBuild if it requires Startup Hooks to work at development time. Maybe based off Configuration=Debug?

Given hot reload is a .NET feature, and is enabled by default for all .NET apps running from VS which also relies on startup hooks, configuring it in Microsoft.NET.Sdk would be the correct place. But at that point, should we have the trimmer targets configure it conditionally - https://github.com/dotnet/sdk/blob/main/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets#L37

- <StartupHookSupport Condition="'$(StartupHookSupport)' == ''">false</StartupHookSupport>
+<StartupHookSupport Condition="'$(StartupHookSupport)' == '' AND '$(Configuration)' == 'Release'">false</StartupHookSupport>

eerhardt commented 3 years ago

Given hot reload is a .NET feature, and is enabled by default for all .NET apps running from VS which also relies on startup hooks

Can you point where hot reload depends on startup hooks (for non-ASP.NET projects)? I didn't know this, and would like to learn more.

cc @vitek-karas @agocke

pranavkm commented 3 years ago

The piece of code that receives hot reload deltas and calls MetadataUpdateHandlers is injected in to the app using a StartupHook. The code's similar to https://github.com/dotnet/sdk/blob/main/src/BuiltInTools/DotNetDeltaApplier/StartupHook.cs and used in F5 \ Ctrl - F5 and dotnet-watch.

eerhardt commented 3 years ago

FYI - the proposed fix above goes against the principle we were following when we defaulted StartupHookSupport=false when PublishTrimmed is set. We want the app to behave the exact same when the developer is debugging as it does when the app is published. Defaulting StartupHookSupport=false only in Release configuration breaks this principle. If an app is using StartupHooks for something else, the app will work for Debug, but then break once it is published for Release.

pranavkm commented 3 years ago

FYI - the proposed fix above goes against the principle we were following when we defaulted StartupHookSupport=false when PublishTrimmed is set.

I'm open to alternatives. We'd like startup hooks to work during F5 and Ctrl-F5 for all .NET 6 apps since they're now hot reloaded by default. We could have VS and dotnet-watch configure StartupHooksSupport=true when running, but I'm not sure what would be the point of the SDK's default would be at that point.

agocke commented 3 years ago

It would be best if startup hooks could only be enabled for the hot reload code. Then, if the user tries to use startup hooks for a different feature, their app would still fail.

vitek-karas commented 3 years ago

I'm going to make certain assumptions, if these are not true, then we might need to look into more complex solutions. So the assumptions:

Hot-Reload will only be applied to non-trimmed application code (this doesn't mean that the app is not enabled for trimming, nor that it won't eventually be published as trimmed, just that the build of the app to which we apply hot-reload has not been trimmed).
Our tooling fully controls the spawning of the application to be "hot-reloaded". Meaning our code creates the process which starts the app (and prepares it for hot-reload). Another way - we don't support "attaching hot-reload"

If this is true then I think we can make this work reasonably well:

We need to make sure the app is not trimmed, and fail otherwise. This would probably mean some SDK solution. Note that we want to keep PublishTrimmed untouched as it governs analysis as well. We just need to make sure that trimmer doesn't run. For console/ASP.NET apps this is already the case for dotnet build (since trimming only happens during dotnet publish), but other app models may run trimmer during build - for those it would have to be disabled.
- Side note: This is probably already necessary as I can't see how hot-reload would actually work with trimming. The runtime would not see the same picture as the compiler...
On non-trimmed app, startup hooks will always work, they just have to be enabled. This means that we need to modify the .runtimeconfig.json of the app we're starting to set System.StartupHookProvider.IsSupported runtime property to true (or not set it at all). The best way to achieve this would be in SDK as well, but it would mean that we know that we're building for hot-reload. This can be done after the fact by editing the .runtimeconfig.json, but that's a bit "hacky".
Since we control the start of the process we fully control the DOTNET_STARTUP_HOOKS environment variable. So we overwrite anything in it with just our hot-reload startup hook and nothing else.

With these there should be no observable behavioral difference between Debug/Hot-Reload/Release/Published app - even if the app has PublishTrimmed=true which will by default disable startup hooks in all configurations (and in hot-reload we would enable them), they would effectively not work since the env. variable which controls them would be overwritten.

Main downside is that this is rather fragile: Ideally we would produce slightly different build of the app for hot-reload (different runtime-config).

Alternative solutions (requires changed to runtime/framework):

We could introduce a new way to trigger the hot-reload code in the app (not startup hook), something built specifically for hot-reload.
Slightly less invasive change would be to introduce an environment variable to enable startup hooks even if they're disabled in the runtime config. Note that this would not be a new thing, we already have other feature switches (globalization for example) which are controlled both by runtime property and env. variable. This would mean that normal setup for startup hooks would still be "disabled", but hot-reload could enable it by simply setting a new env. variable, but still use the same startup hook mechanisms as everything else. (the env. variable would have no effect on trimmed apps, those would have startup hooks disabled always).

lambdageek commented 3 years ago

We need to make sure the app is not trimmed, and fail otherwise. This would probably mean some SDK solution. Note that we want to keep PublishTrimmed untouched as it governs analysis as well. We just need to make sure that trimmer doesn't run. For console/ASP.NET apps this is already the case for dotnet build (since trimming only happens during dotnet publish), but other app models may run trimmer during build - for those it would have to be disabled.

This is true for .NET for iOS - they use custom trimmer passes during the build even for running the app without trimming. Any solution should not ultimately affect whether the trimmer runs at all.

Side note: This is probably already necessary as I can't see how hot-reload would actually work with trimming. The runtime would not see the same picture as the compiler...

Yep, no IL or metadata rewriting should be happening in hot reload scenarios - otherwise the deltas will not match the loaded assemblies. Neither CoreCLR nor Mono do a lot of validation before applying the delta, so the results will be unpredicatable.

vitek-karas commented 3 years ago

This is true for .NET for iOS - they use custom trimmer passes during the build even for running the app without trimming. Any solution should not ultimately affect whether the trimmer runs at all.

If we do need to run the trimmer in this case (I assume this is the run which is meant to just run custom steps), we should run it with all assemblies set to "copy" action, which guarantees the trimmer does not modify the files at all. In which case it would have no effect on ability to run startup hooks either (in fact, such run should basically throw away the output of the trimming anyway - it should be identical to input).

lambdageek commented 3 years ago

If we do need to run the trimmer in this case (I assume this is the run which is meant to just run custom steps), we should run it with all assemblies set to "copy" action, which guarantees the trimmer does not modify the files at all.

Yes, sorry, that's what I meant. They use the trimmer for analysis (and the custom steps produce some artifacts used later in their build), but they do not rewrite the assemblies.

My point was just that for an iOS projects are an example where PublishTrimmed shouldn't be used (during the build, or at runtime) to make decisions about the ability to use hot reload, run startup hooks, etc.

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

MarkusRodler commented 2 years ago

~"Workaround" for other folks with this problem:~

<PublishTrimmed Condition="'$(DotNetWatchBuild)'!='true'">true</PublishTrimmed>

Edit: Forget what I said. It does not work unfortunately 😢

mkArtakMSFT commented 1 year ago

@vitek-karas, @lambdageek, @davidfowl, @eerhardt can somebody please summarize what is the current state of this issue and what is pending ? Thanks!

ghost commented 1 year ago

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

fabiano commented 5 months ago

The same happens when you add <PublishAot> to the csproj file:

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
    <InvariantGlobalization>true</InvariantGlobalization>
    <PublishAot>true</PublishAot>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Google.Cloud.Diagnostics.AspNetCore3" Version="5.1.0" />
    <PackageReference Include="Microsoft.Data.Sqlite" Version="8.0.6" />
  </ItemGroup>

</Project>

The exception:

crit: Microsoft.AspNetCore.Hosting.Diagnostics[11]
      Hosting startup assembly exception
      System.InvalidOperationException: Startup assembly Microsoft.AspNetCore.Watch.BrowserRefresh failed to execute. See the inner exception for more details.
       ---> System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.AspNetCore.Watch.BrowserRefresh, Culture=neutral, PublicKeyToken=null'. The system cannot find the file specified.

      File name: 'Microsoft.AspNetCore.Watch.BrowserRefresh, Culture=neutral, PublicKeyToken=null'
         at System.Reflection.RuntimeAssembly.InternalLoad(AssemblyName assemblyName, StackCrawlMark& stackMark, AssemblyLoadContext assemblyLoadContext, RuntimeAssembly requestingAssembly, Boolean throwOnFileNotFound)
         at System.Reflection.Assembly.Load(AssemblyName assemblyRef)
         at Microsoft.AspNetCore.Hosting.GenericWebHostBuilder.ExecuteHostingStartups()
         --- End of inner exception stack trace ---

SDK:

.NET SDK:
 Version:           8.0.104
 Commit:            034f91fcc0
 Workload version:  8.0.100-manifests.cd97f1c9

Runtime Environment:
 OS Name:     fedora
 OS Version:  40
 OS Platform: Linux
 RID:         fedora.40-x64
 Base Path:   /usr/lib64/dotnet/sdk/8.0.104/

.NET workloads installed:
 Workload version: 8.0.100-manifests.cd97f1c9

dotnet / aspnetcore