dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.52k stars 10.04k forks source link

Hot Reload: unable to call added public API #36356

Open tmat opened 3 years ago

tmat commented 3 years ago

Description

Calling a public method added to a referenced library during Hot Reload may throw System.MissingMethodException.

Repro: Run App.exe, uncomment the commented code and apply changes. If both changes are applied at the same time the exception is thrown. If the method is added first and then the caller is updated it works. Seems like the changes should be applied in topological order based on assembly references.

App.exe:

class Program
{
    static void G()
    {
        var c = new LibClass();
        // Console.WriteLine(c.F()); // uncomment during Hot Reload session
    }

    static void Main(string[] args)
    {
        while (true)
        {
            Thread.Sleep(500);
            G();
        }
    }
}

Lib.dll referenced by App.exe

public class LibClass
{
     // uncomment during Hot Reload session
     // public int F() => 1;
}

Configuration

.NET 6.0.100-preview.6.21355.2 Windows 10

Regression?

No.

dotnet-issue-labeler[bot] commented 3 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

stephentoub commented 3 years ago

cc: @mikem8361, @tommcdon

ghost commented 3 years ago

Tagging subscribers to this area: @tommcdon See info in area-owners.md if you want to be subscribed.

Issue Details
### Description Calling a public method added to a referenced library during Hot Reload may throw `System.MissingMethodException`. Repro: Run App.exe, uncomment the commented code and apply changes. If both changes are applied at the same time the exception is thrown. If the method is added first and then the caller is updated it works. Seems like the changes should be applied in topological order based on assembly references. App.exe: ```C# class Program { static void G() { var c = new LibClass(); // Console.WriteLine(c.F()); // uncomment during Hot Reload session } static void Main(string[] args) { while (true) { Thread.Sleep(500); G(); } } } ``` Lib.dll referenced by App.exe ```C# public class LibClass { // uncomment during Hot Reload session // public int F() => 1; } ``` ### Configuration .NET 6.0.100-preview.6.21355.2 Windows 10 ### Regression? No.
Author: tmat
Assignees: mikem8361
Labels: `area-Diagnostics-coreclr`
Milestone: -
mikem8361 commented 3 years ago

@noahfalk, @jkotas could either of you point me in the general direction of where in the runtime to look?

jkotas commented 3 years ago

The hot reload agent should be responsible for applying the changes in topological order. It does not look like an issue in the runtime to me.

stephentoub commented 3 years ago

Presumably it would need to be done either in the agent, e.g. https://github.com/dotnet/aspnetcore/blob/18a926850f7374248e687ee64390e7f10514403f/src/Components/WebAssembly/WebAssembly/src/HotReload/HotReloadAgent.cs#L193-L207 or by Roslyn as part of generating the EnC diffs and then the agent just guaranteeing it'll apply them in the exact order provided to it.

cc: @pranavkm

mikem8361 commented 3 years ago

Yes, it does look like the assemblies are updated in the wrong order. My test app's main assembly is updated first adding the reference to the new function and then the test library that adds the function itself.

If I do the edits separately with the library first and then the main app, it works.

mikem8361 commented 3 years ago

@pranavkm can you look into this issue?

pranavkm commented 3 years ago

@tmat is this specific to watch / Control-F5 or does it also need to change under F5?

tmat commented 3 years ago

/cc @isadorasophia

F5 likely has a similar problem. EnC does not since the app is suspended when changes are applied.

I think the ordering needs to happen in the code that applies changes to the process. If it was done in Roslyn it wouldn't work for C# code calling F# (if in future F# supported hot reload), or other languages that might support Hot Reload.

isadorasophia commented 3 years ago

@tmat yeah, sounds like this issue would happen in both F5 and Ctrl-F5. Wouldn't it be sufficient to expect that each language service order its own updates? How would we know how to order it otherwise?

tmat commented 3 years ago

It wouldn't - imagine the above Lib.dll written in F# and the App.exe in C#. The ordering needs to consider contributions of all language services.

stephentoub commented 3 years ago

In such a scenario, who's coordinating getting all of the updates from each language service to the hot reload agent as part of the same batch?

tmat commented 3 years ago

The debugger receives the deltas from language services and then sends them over to all registered agents - it's M:N.

tmat commented 3 years ago

Another scenario that would be good to test as well:

  1. Add public method Foo() to a class in a.dll and a call to Foo() in b.dll when neither dll is loaded to the process
  2. b.dll gets loaded to the process - at this point the second change is applied
  3. The method that calls Foo() gets invoked - at this point Foo() MemberRef resolution starts, which loads a.dll, the first change is applied before the resolution continues so that the method is found.
mikem8361 commented 3 years ago

For the original scenario who is going to order the updates? The runtime can't. The hot reload agent would have to crack the delta from the language service(s) and order them topologically by assembly (which sounds like a lot of work this late in the release), but how would updates be "batched" from separate language services so agent knows what to order? A compromise (depending on how much work it is) is to have the Roslyn order them for now so at least C# to C# assembly work.

For tmat's new scenario, how do metadata changes get applied to assemblies that haven't been loaded yet? As far as I understand the runtime apply delta code, the assembly has to be loaded.

pranavkm commented 3 years ago

he hot reload agent would have to crack the delta from the language service(s) and order them topologically by assembly

The agent already has to do this because ApplyUpdate requires an Assembly instance. We have code to do topological sort the metadata updata handlers, so it shouldn't be to hard to also use it for this case.

how do metadata changes get applied to assemblies that haven't been loaded yet

HotReloadAgent stashes all of the batches away and applies them on AssemblyLoad.

Most of this is doable in a few days time, I'm less certain if it would be high on our priority list.

ghost commented 3 years ago

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.