Repository navigation
Add a notification mechanism for EnC and HotReload #49361
Description
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
- event: creating an event has the problem of registration, mainly for 3rd party libraries
- 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, ....)
- 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
Activity
StephaneDelcroix commentedon Mar 9, 2021
/cc @tommcdon @LyalinDotCom @tmat @joj
stephentoub commentedon Mar 9, 2021
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.
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.
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?
Isn't this already the plan?
stephentoub commentedon Mar 9, 2021
This part still needs to be debated. @jkotas has expressed strong concerns against doing this.
jkotas commentedon Mar 9, 2021
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.
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.
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.
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 commentedon Mar 9, 2021
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.
Why does this have
oldTypeandnewType? I thought that we are focused on updating types in-place.stephentoub commentedon Mar 9, 2021
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?
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.
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?
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 commentedon Mar 9, 2021
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.
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.
Reflection and Json serializer are example of lower-level / higher-level components with relationship.
I meant the code injected into the process. We need better names for this - what about:
stephentoub commentedon Mar 9, 2021
Meaning corelib itself would have an attribute on it like:
yes?
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.
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)?
@tommcdon has been using:
and we could add to that:
jkotas commentedon Mar 9, 2021
Yep.
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.
Yep, there is a lot of complicated schemes we can invent here to push this further. Another option can be to have
BeforeUpdateandAfterUpdatemethods on the update handler, and require the update handler takes a lock inBeforeUpdateand release the lock inAfterUpdate.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 commentedon Mar 9, 2021
Couldn't AppDomain.AssemblyLoad be used for the dynamic cases by the Hot Reload Agent?
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?
45 remaining items
terrajobst commentedon Apr 1, 2021
danmoseley commentedon Apr 8, 2021
@mikem8361 @stephentoub who is doing the work here to (1) add this attribute type (2) make it work?
danmoseley commentedon Apr 8, 2021
cc @ericstj
stephentoub commentedon Apr 8, 2021
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 commentedon Apr 8, 2021
stephentoub commentedon Apr 8, 2021
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.