Add a notification mechanism for EnC and HotReload

[StephaneDelcroix]

Background and Motivation

One of the key work stream for the success of HotReload is a feedback mechanism so frameworks and 3rd party libraries knows that a type was hot reloaded and can decide to replace a live instance, invalidate a cache, refresh a binding, etc...

The proposed API isn't prescriptive, and other feedback mechanisms can be discussed, but it's quite important that the API is part of the BCL and the feature is supported by the runtime, so all frameworks will benefits from it, and it'll work in all HotReload scenarios (debugger and botnet watch).

This proposal was first mentioned in #45689, 2nd bullet of the 'Related API changes' section

Proposed API

namespace System.Reflection.Metadata
{
    [System.AttributeUsage(System.AttributeTargets.Assembly, AllowMultiple = true)]
    public sealed class  ApplyUpdateNotificationAttribute : Attribute
    {
        /// <summary>
        /// HotReload notification API
        ///
        /// Declares which static method to execute when a HotReload delta is applied. The hot reload agent is responsible
        /// for scanning assemblies at load, and register the notifiers.
        ///
        /// notifier signature should look like: internal static void BeforeUpdate(Type type) and/or AfterUpdate(Type type).
        /// </summary>
        /// <param name="type">The Type on which the notifier is defined</param>
        /// <exception cref="ArgumentNullException">type or notifier are null</exception>
        /// <exception cref="...">type or notifier does not exist, or notifier isn't static, or doesn't have the right signature</exception>
        public ApplyUpdateNotificationAttribute (Type type)   
    }
}

Usage Examples

This is how a UI Framework could reload a view when an HotReload change is applied

[assembly:ApplyUpdateNotification(typeof(HotReloadExtensions)]

namespace MyUIFramework
{
    public class View {
        public View Parent {get; set;} // setting the Parent sets the Child on the Parent view

        public View ()
        {
            HotReloadExtensions.RegisterInstance(this);
        }
    }

    internal static class HotReloadExtensions
    {
        // Keep weakRef of created views in a cache
        public static void RegisterInstance(View view) {...}

        // retrieve values from the cache
        static IEnumerable<View> GetViewsOfType(Type type) {...}

        // the notifier, declared by the attribute
        public static void AfterUpdate(Type type)
        {
            foreach (var oldinstance in GetViewsOfType(type)
            {
            }
        }
    }
}

Alternative Designs

1. event: creating an event has the problem of registration, mainly for 3rd party libraries
2. implement this capability at the framework level. repetitive work, and the runtime would still have to implement it's own mechanism to invalide its caches (I'm thinking ReflectionCache, ....)
3. whatever applies the change could also trigger the notification, but that would require injecting some shared code in all platforms supporting EnC/HR

Risks

Unknown at this time

[dotnet-issue-labeler]

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.

[StephaneDelcroix]

/cc @tommcdon @LyalinDotCom @tmat @joj

[stephentoub]

The runtime is responsible for scanning assemblies at load, and register the notifiers

If it's the runtime (i.e. corelib) that's responsible for notifying, then I think it should just be an event code can subscribe to rather than a dynamic reflection-based mechanism, likely right next to ApplyUpdate, e.g. UpdateApplied. A component can register with the event from a ModuleInitializer if desired.

If we want to use a reflection-based scheme looking for attributes, it doesn't need to be the runtime that does this: it can just be the hot reload component injected into the process.

EnCNotificationAttribute

EnC is an implementation detail on coreclr, e.g. mono uses MBR. We should tie the naming here to the same naming used for the other public surface area, e.g. ApplyUpdate.

Guid notifierId, IEnumerable<Guid> runBefore

Why is this necessary at this level? If there are multiple components that have a relationship, why can't just one of them register and then be responsible for invoking whatever else needs to be invoked in whatever order is important?

that would require injecting some shared code in all platforms supporting EnC/HR

Isn't this already the plan?

[stephentoub]

and the runtime would still have to implement it's own mechanism to invalide its caches

This part still needs to be debated. @jkotas has expressed strong concerns against doing this.

[jkotas]

attributes

Enumerating all types in all assemblies to find items marked with a specific attribute is well know performance anti pattern. What about having assembly-level attribute that specifies the EnC manager(s) in the given assembly? The EnC manager can then just look for the assembly-level attributes to find all eligible participants that is quite cheap.

EDIT: I have misread the proposal. I see that the proposal is assembly level attribute. That's good.

A component can register with the event from a ModuleInitializer if desired.

I like the declarative scheme better than registering upfront. The declarative scheme is more pay-for-play (e.g. wrt linkability) without additional work.

Also, the declarative scheme allows assemblies compiled against netstadard to participate, without re-targeting for .NET 6+. I am not sure whether this is important.

If there are multiple components that have a relationship, why can't just one of them register and then be responsible for invoking whatever else needs to be invoked

How does the lower-level component know whether there is going to be some higher-level component to flush it, or whether it is on its own and it should be responsible for itself? I think there will need to be some sort of ordering applied by the EnC manager.

runtime invalide its caches

I would be fine with CoreLib reflection participating in the updating scheme as long as it is pay-for-play, like the assembly-level attribute that specifies the EnC manager(s) in the given assembly that I have suggested.

Also, do we need an API that returns true when code updates are allowed? This API would return true when the environment variable that enables code updatability is set. I expect that some components will want to switch to a different slower mode of operation to support updateability.

[jkotas]

IEnumerable runBefore

I do not think you can have IEnumerable<Guid> in an attribute.

I think the attribute should just have the local update manager type name. The types of notifications and other details can be inferred from the methods implemented on the update manager.

public static void OnChange(Type oldType, Type newType)

Why does this have oldType and newType? I thought that we are focused on updating types in-place.

[stephentoub]

I would be fine with CoreLib reflection participating in the updating scheme as long as it is pay-for-play, like the assembly-level attribute that specifies the EnC manager(s) in the given assembly that I have suggested.

To make sure I understand, who is doing the clearing/reset here? And your previously expressed concerns about reflection becoming a mutable model are no longer a concern because the assembly is opting in to hot reload via the attribute?

The declarative scheme is more pay-for-play (e.g. wrt linkability) without additional work.

You mean because the linker could be told whether or not to remove the attribute based on build flavor vs needing to be taught if/when it was ok to remove the module initializer? Fair enough.

How does the lower-level component know whether there is going to be some higher-level component to flush it, or whether it is on its own and it should be responsible for itself?

It's a question of scenarios. If it's really the case that you might be using a lower-level component that needs notification and be doing so separately from a higher-level component (that may or may not be used) that also needs notifications and they need to be executed in a particular order, then yeah, that would need to be applied by the invoker. Is that the scenario? To make it concrete, how does that come into play here, e.g. with Xamarin?

the EnC manager

When you say "EnC manager", which component specifically are you referring to? I just want to make sure we're talking about the same thing. Do you mean something in the runtime itself (e.g. code invoked as part of ApplyUpdate), or do you mean the code injected into the process (e.g. the same code that calls ApplyUpdate), or something else?

[jkotas]

To make sure I understand, who is doing the clearing/reset here?

The global update manager (not in CoreLib, comes with SDK) would call local update handler in CoreLib. The local update handler in CoreLib would simply clear the weak handle that points to the reflection cache.

And your previously expressed concerns about reflection becoming a mutable model are no longer a concern because the assembly is opting in to hot reload via the attribute?

I have internalized the fact that all this is best effort, the feature will not work as expected in number of cases and that it is by design.

Is that the scenario? To make it concrete, how does that come into play here, e.g. with Xamarin?

Reflection and Json serializer are example of lower-level / higher-level components with relationship.

EnC manager

I meant the code injected into the process. We need better names for this - what about:

• Update Manager: Global component injected into the process
• Update Handler: Local assembly-specific code that applies the updates

[stephentoub]

The global update manager (not in CoreLib, comes with SDK) would call local update handler in CoreLib

Meaning corelib itself would have an attribute on it like:

[assembly: UpdateAppliedHandler("System.Reflection.Type", "ClearReflectionCaches", "D02B1C93-A6E5-4F07-A45D-2CD02675CF77")] //with whatever attribute name and arguments we decided on

yes?

I have internalized the fact that all this is best effort

Ok 😄

And, yes, there are plenty of things that won't work, automatically. For example, I don't think JsonSerializer will be in a good position to automatically dump all reflection-emitted code it spit out previously, given that it doesn't reference all JsonSerializerOptions instances ever used, and such options objects hold on to reflection-related state. Libraries and app code that may have used JsonSerializer and are holding on to the options bag that references the serialized code will need to (and know to) drop it and create a new one. You could imagine a complicated versioning scheme where every cache is tagged with a version, JsonSerializer updates its notion of valid version on every update, and it only respects caches created with that same version number, or something like that.

Reflection and Json serializer are example of lower-level / higher-level components with relationship.

I presume you mean because, for example, JsonSerializerOptions will need to ditch its s_defaultOptions instance in order to drop any reflection caches it contains? Are all executing threads going to be paused during the invocation of these handlers? If not, I wonder how much ordering really matters, as problems can arise regardless of which cache is cleared first. It's obviously better for the underlying reflection cache to be cleared so that all subsequent accesses to it get up-to-date information, but if JsonSerializer could be executing concurrently with an update being applied, it's going to be operating on stale state until its cache is cleared and executing threads with a reference to the cached data decide to go back to the cache to update.

Regardless, your point is that such orderings may be needed. That suggests we're going to need to document the IDs of anything that anyone might want to rely on for ordering, so that they can include those dependencies.

This example is also one where just ordering the invocations from lower in the assembly reference graph to higher in the graph would suffice. Are there examples where it'd be important for the higher-level component to update first (and where the lower-level component is used with and without the higher-level one such that it needs its own update handler)?

We need better names for this

@tommcdon has been using:

• Hot Reload Agent: the code injected into the process (what you refer to as "update manager")
• Hot Reload Manager: the external launcher component (e.g. dotnet watch) that injects the agent

and we could add to that:

• Hot Reload Update Handler: code invoked by the manager in response to an update being applied

[ghost]

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

Issue Details

---

Background and Motivation

One of the key work stream for the success of HotReload is a feedback mechanism so frameworks and 3rd party libraries knows that a type was hot reloaded and can decide to replace a live instance, invalidate a cache, refresh a binding, etc...

The proposed API isn't prescriptive, and other feedback mechanisms can be discussed, but it's quite important that the API is part of the BCL and the feature is supported by the runtime, so all frameworks will benefits from it, and it'll work in all HotReload scenarios (debugger and botnet watch).

This proposal was first mentioned in #45689, 2nd bullet of the 'Related API changes' section

Proposed API

namespace System.Runtime.???
{
[System.AttributeUsage(System.AttributeTargets.Assembly)]
    public sealed class EnCNotificationAttribute : Attribute
    {
        /// <summary>
        /// HotReload notification API
        ///
        /// Declares which static method to execute when a HotReload delta is applied. The runtime is responsible
        /// for scanning assemblies at load, and register the notifiers
        ///
        /// notifier signature should look like: internal static void OnChange(Type oldType, Type newType);
        /// </summary>
        /// <param name="type">The Type on which the notifier is defined</param>
        /// <param name="notifier">The name of the static method to invoke when a delta is applied</param>
        /// <exception cref="ArgumentNullException">type or notifier are null</exception>
        /// <exception cref="...">type or notifier does not exist, or notifier isn't static, or doesn't have the right signature</exception>
        public EnCNotificationAttribute (string type, string notifier)

        /// <summary>
        /// ...
        /// when the runtime loads the containing assembly, it can build a topological graph and execute the notifiers in the right order
        /// </summary>
        /// ...
        /// <param name="notifierId">the Id of the notifier</param>
        /// <param name="runBefore">this notifier should be executed before notifier in runBefore</param>
        /// ...
        /// <exception cref="...">loop detected in the topological map of notifiers</exception>
        public EnCNotificationAttribute (string type, string notifier, Guid notifierId, IEnumerable<Guid> runBefore)
        {
        }
    }
}

Usage Examples

This is how a UI Framework could reload a view when an HotReload change is applied

namespace MyUIFramework
{
    [assembly:EnCNotification("HotReloadExtensions", "OnChange"]

    public class View {
        public View Parent {get; set;} // setting the Parent sets the Child on the Parent view

        public View ()
        {
            HotReloadExtensions.RegisterInstance(this);
        }
    }

    internal static class HotReloadExtensions
    {
        // Keep weakRef of created views in a cache
        public static void RegisterInstance(View view) {...}

        // retrieve values from the cache
        static IEnumerable<View> GetViewsOfType(Type type) {...}

        // the notifier, declared by the attribute
        public static void OnChange(Type oldType, Type newType)
        {
            foreach (var oldinstance in GetViewsOfType(oldType)
            {
                var newInstance = System.Activator.CreateInstance(newType);
                newInstance.Parent = oldInstance.Parent; //a serialisation/deserialisation of the instance is better, Parent is given as an example
            }
        }
    }
}

Alternative Designs

1. event: creating an event has the problem of registration, mainly for 3rd party libraries
2. implement this capability at the framework level. repetitive work, and the runtime would still have to implement it's own mechanism to invalide its caches (I'm thinking ReflectionCache, ....)
3. whatever applies the change could also trigger the notification, but that would require injecting some shared code in all platforms supporting EnC/HR

Risks

Unknown at this time

Author:	StephaneDelcroix
Assignees:	-
Labels:	api-suggestion, area-Diagnostics-coreclr, untriaged
Milestone:	-

[jkotas]

Meaning corelib itself would have an attribute on it like:
[assembly: UpdateAppliedHandler("System.Reflection.Type", "ClearReflectionCaches", "D02B1C93-A6E5-4F07-A45D-2CD02675CF77")]

Yep.

with whatever attribute name and arguments we decided on

I think the attribute should have just the type name. No method name or Guid. All those details can be attached to the update handler type.

You could imagine a complicated versioning scheme where every cache is tagged with a version, JsonSerializer updates its notion of valid version on every update, and it only respects caches created with that same version number, or something like that.

Yep, there is a lot of complicated schemes we can invent here to push this further. Another option can be to have BeforeUpdate and AfterUpdate methods on the update handler, and require the update handler takes a lock in BeforeUpdate and release the lock in AfterUpdate.

ordering the invocations from lower in the assembly reference graph to higher in the graph would suffice.

Yes, this works for assemblies with static dependencies. It does not work for cases where the graph is composed dynamically, via DI or similar architectures. Again, whether this is important is a question for how far we want to go.

[mikem8361]

Couldn't AppDomain.AssemblyLoad be used for the dynamic cases by the Hot Reload Agent?

All those details can be attached to the update handler type.

If the attribute only has the type name, how does the agent know what to call on that type? Is the agent going to make assumptions on the name and parameters of the method or methods? Would defining an interface that the type implements make sense?

[jkotas]

how does the agent know what to call on that type? Is the agent going to make assumptions on the name and parameters of the method or methods?

Yes, the idea is to define a convention that the update handler methods need to follow, and that the agent will use to find methods to call.

Would defining an interface that the type implements make sense?

I expect that we will see a need to evolve the convention a lot over time as the feature evolves and handling of more complex cases is added. Fixed interface would get in the way.

Also, the convention based scheme makes it straightforward for the application-framework specific types to be part of the convention and be special cased by the agent as necessary.

[ericstj]

Small comment on the functionality. It should be located by name and not strong-type reference so that components which target older frameworks like .NETStandard2.0 can make use of it without taking a new binary dependency nor retargeting.

[mikem8361]

I would like to propose:

1. Rename to ApplyUpdateNotificationAttribute.
2. Have one constructor with with Type type as the only parameter.
3. Define the Update Handler convention as two static functions "BeforeUpdate()" and "AfterUpdate()" with no parameters.

Any feedback?

[stephentoub]

Any feedback?

1. Is the intent that BeforeUpdate/AfterUpdate are statics or that the specified type will be instantiated and then those would be instance members?
2. Will any additional information be passed, e.g. the Types being updated?

[jkotas]

Have one constructor with with "string type" as the only parameter.

The parameter should be Type handler. Types are fine as attribute parameters.

[mikem8361]

1. Is the intent that BeforeUpdate/AfterUpdate are statics or that the specified type will be instantiated and then those would be instance members?

I think they should be static.

2. Will any additional information be passed, e.g. the Types being updated?

The Hot Reload Manager would have to crack the metadata delta format to pass any information about the change. To keep this simple for now, no information is passed.

[stephentoub]

To keep this simple for now

FWIW, it keeps this simpler, but it likely makes some handlers more complicated. For example, the handler for clearing reflection type caches needs to null out a field in the relevant type. If it's given the type, it can set the field. If it's not and needs to assume that any type could have been updated, it'll need to enumerate every Type in the system that's been reified and clear it on that, just in case it was one that was updated.

[mikem8361]

Yes, I realized this as I was editing the "Usage Example" in the header that without the info on the type being updated it will make flushing the reflection caches and probably the JsonSerializer-like cases way more difficult. If we pass the type in the *Update handlers, are we ok with putting this burden of writing a parser in C# for the deltas on the Hot Reload Agent or Manager? I may be making a big deal of the work involved since the format isn't well documented.

What does everyone else think about adding the type being updated to the handler?

[jkotas]

I believe that @tmat said at one point that Roslyn has the information about edited types available and it should not be hard to pass it through the pipeline. @tmat Do I remember it correctly?

Also, there is a distinction between directly edited types and types affected by the edit. For example, when you edit a base type, reflection caches or cached serializers for inherited types may need to get invalidated too. How are we going to handle that?

[stephentoub]

For example, when you edit a base type, reflection caches or cached serializers for inherited types may need to get invalidated too.

That's a good point. For reflection caches, if the type provided is unsealed, we'd probably need to walk whatever cache we have of all created Types to look for ones deriving from the one specified. For cached serializers, at least for JsonSerializer, I expect the most expedient thing to do (with or without derived types) is to invalidate all serializers, whether or not they were affected... but maybe we can be more surgical.

[lambdageek]

are we ok with putting this burden of writing a parser in C# for the deltas on the Hot Reload Agent or Manager?

Technically this exists MetadataReaderExtensions.GetEditAndContinueLogEntries but the devil is in the details: to resolve the tokens to type names (or some other identity) you will need not only the delta blob but also a view of the updated metadata heaps. A good comparison is what metadata-tools has to do to get a value from a heap in the presence of multiple generations.

It's probably better to get the data over the wire from the Hot Reload Manager and Roslyn.

There's also the asymmetry in computing resources: the Agent is running on a mobile device (or an emulator), the Manager on a beefy dev machine. Most computation should stay on the Manager side, IMO.

[mikem8361]

Even though we haven't worked out all the details of the Hot Reload Handler convention, I think the attribute itself is ready for review. The convention can be refined over time, but getting the attribute reviewed and into the framework soon is important schedule wise.

[mikem8361]

/cc: @davidwrighton

[mikem8361]

It sounds like the ordering and the details of the hot reload handler convention are close. Do we want a separate Reflection cache update handler? Since it and other caches can be just marked as "cleared" and not repopulated in the handler, the order isn't important other than being before the UI refresh handlers. Is BeforeUpdate (caches)/AfterUpdate (UI) handlers enough?

Can we agree that the attribute itself is ready for review? I would like to drive that to closure if possible.

[jkotas]

The attribute looks good to me. I am not sure whether System.Reflection.Metadata namespace make sense - something to discuss during API review.

[jkotas]

The runtime is responsible for scanning assemblies at load, and register the notifiers

This should be updated to say that the hot reload agent is reponsible for this.

[stephentoub]

Can we agree that the attribute itself is ready for review?

As specified, it'll prohibit multiple such attributes on an assembly. Is that the desired semantic? That might make it harder for source generators, as a source generator that wanted to receive a notification would then need to coordinate with any other usage of the attribute in the assembly. So, maybe it should be AllowMultiple=true, and the hot reload agent is responsible for calling all defined handlers in an assembly rather than just one per.

I am not sure whether System.Reflection.Metadata namespace make sense - something to discuss during API review.

I think it makes sense for ApplyUpdateNotificationAttribute to be in the same namespace as ApplyUpdate, which leads to System.Reflection.Metadata. But, I'm fine with whatever we decide in API review, as well.

Do we want a separate Reflection cache update handler?

We should use the same mechanism as everyone else, which would suggest Corelib will need its own handler for the agent to invoke, and that handler will clear the relevant state. Presumably that'll be some static that QCalls into the runtime to clear whatever caches it needs to clear, if it can't just do it by looking at the one type it's handed (e.g. if it's given a non-sealed type and needs to find derived types that might need to be cleared as well).

[mikem8361]

Do we want a separate Reflection cache update handler?

What I meant was should there be a different method on the handler class (i.e. ReflectionUpdate(Type type)) just for reflection cache clearing in corelib or will BeforeUpdate(Type type) be sufficient.

[stephentoub]

or will BeforeUpdate(Type type) be sufficient

BeforeUpdate should be sufficient. If we find it's not, we'll need to revisit the general approach and discuss exactly what support for different levels/ordering are required for the target scenarios. We should not add anything reflection-specific to the pattern.

[tmat]

BeforeUpdate? I thought we are invoking AfterUpdate so that one can inspect the new version of the type.
Reflection's handler only needs to be special cased so that it's invoked first (so that AfterUpdate(Type) can inspect the new version of the type. But it can follow the same API pattern.

[stephentoub]

The pattern would have both before and after; the agent would invoke whichever exist at the appropriate time. The reflection clearing support in corelib would only need to implement the before.

[tmat]

I see. OK.

[terrajobst]

• Looks good as proposed

namespace System.Reflection.Metadata
{
    [AttributeUsage(AttributeTargets.Assembly, AllowMultiple=True)]
    public sealed class MetadataUpdateHandlerAttribute : Attribute
    {
        public MetadataUpdateHandlerAttribute(Type type)   
    }
}

[danmoseley]

@mikem8361 @stephentoub who is doing the work here to (1) add this attribute type (2) make it work?

[danmoseley]

cc @ericstj

[stephentoub]

Adding the attribute is trivial. I can do that. "Making it work", meaning invoking it, is part of the hot reload agent and is not part of dotnet/runtime. Then there are the various places we ourselves want to subscribe to a notification. I believe Mike is signed up to clear the runtime's reflection cache, but he can correct me if that's evolved. And @eerhardt has been auditing where else we may want to take action.

[mikem8361]

I'm planning to add the attribute and to clear the runtime's reflection caches. I'm currently working on something else at this point, but I'll get to this hot reload work in a few weeks.

[stephentoub]

Thanks, Mike. I'll just add the attribute now, as it's quick to do, to unblock anyone else upstream. I'll open an issue and leave a TODO in the code for you to fill in the cache clearing stuff later.