Clear some caches from MVC on hot reload event #33386

pranavkm · 2021-06-08T23:22:21Z

Trigger action descriptor provider on hot reload.
Clear other caches that are keyed off of Type
Clean up MvcSandbox

Contributes to #31806

* Trigger action descriptor provider on hot reload. * Clear other caches that are keyed off of Type * Clean up MvcSandbox

pranavkm · 2021-06-08T23:22:45Z

src/Mvc/Mvc.Core/src/Controllers/DefaultControllerPropertyActivator.cs


 public void Activate(ControllerContext context, object controller)
 {
- LazyInitializer.EnsureInitialized(


This is just silly.

pranavkm · 2021-06-08T23:23:13Z

src/Mvc/Mvc.Core/src/DependencyInjection/MvcCoreServiceCollectionExtensions.cs

 services.TryAddSingleton(partManager);

 ConfigureDefaultFeatureProviders(partManager);
 ConfigureDefaultServices(services);
 AddMvcCoreServices(services);

+ if (environment?.IsDevelopment() ?? false)


FYI @Tratcher. I hear this makes you sad.

What's the workaround for someone using a custom environment?

Should we investigate using some other marker here, e.g. a specific env var that's set by the Hot Reload manager?

Yes please. #28937

Blazor WASM currently uses Debugging.IsSupported to determine this - https://github.com/dotnet/aspnetcore/blob/main/src/Components/Shared/src/HotReloadFeature.cs#L6-L16. But the experience is nicer there. The trimmer yanks out all of that code during publish, so it entirely no-ops. We don't have the benefit here. Also, aspnetcore's deployment is slightly centered around environments rather than build / publish.

@stephentoub, do you have suggestions? Would dotnet/runtime#51159 solve this problem?

Does "Hot Reload" work in apps that didn't have a debugger attached when the process was started?

With dotnet-watch and the Ctrl-F5 scenarios in VS, a debugger isn't attached (and once a hot reload delta is applied, the runtime prevents attaching a debugger).

The runtime currently requires setting DOTNET_MODIFIABLE_ASSEMBLIES=Debug for the ApplyDelta API to be called. I'm not a 100% sure if that env is also required with the debugger (probably not), but perhaps we could HotReload.IsSupported => Debugger.IsAttached || Env["DOTNET_MODIFIABLE_ASSEMBLIES"] == "Debug"?

Yes, I think this would be the best way though I would say something HotReload.IsSupported => Debugger.IsAttached || Env["DOTNET_MODIFIABLE_ASSEMBLIES"] != "" || Env["COMPlus_ForceEnc"] == "1". There is one problem with this logic: when the env vars are NOT set and a debugger attaches (a typical scenario) this proposed property would incorrectly return true.

ENC for a module is enabled on launch by the debugger on module load callbacks. These env vars are only needed if you want ENC when the debugger attaches.

If we want this global property part of the runtime, it probably should be renamed to something without "HotReload". We have been avoiding that term in the public APIs (see AssemblyExtensions.ApplyUpdate or AssemblyExtensions.GetApplyUpdateCapabilities).

The runtime itself couldn't do any better with some kind of QCall because it doesn't have a concept of a global "is ENC'able or hotreloadable" flag. It uses these env vars (or a call to a debugging API) to create special ENC per-module data structures. The runtime could return that an assembly/module is ENC'able/hotreloadable but something global is needed.

Just a minor difference in behavior with Mono: right now we always require Env["DOTNET_MODIFIABLE_ASSEMBLIES"]!="" to be set at startup (possibly it's enough for the variable to be set just before any assembly we intend to modify executes any code, but I haven't tested this). The mono debugger engine doesn't have any mechanism to say "enable EnC for a module" at the moment.

But actually, checking the environment variable is a necessary but not sufficient condition for whether we actually have hot reload at runtime - with the ongoing componentization of Mono, workloads can decide whether to bundle hot reload support in some configurations or builds of the user's app.

So framework or library authors really need a separate feature switch if they want to affect runtime behavior. (I think we're all aware of the open issue for that, but just in case it's dotnet/runtime#51159)

I filed #33477 to track updating this code once we've had a chance to figure out what the right semantics should be for this flag. Given solving this is largely a hygiene issue, I'm not going to block on resolving if we're able to get support for custom attributes working end-to-end in preview6.

pranavkm · 2021-06-08T23:25:29Z

src/Mvc/Mvc.Core/src/HotReload/HotReloadService.cs

+ private readonly DefaultModelMetadataProvider? _modelMetadataProvider;
+ private readonly DefaultControllerPropertyActivator? _controllerPropertyActivator;


These two were the only two I found that were keyed off Type (aside from PropertyHelper). The rest are keyed off things like action descriptors or ModelMetadata - which should auto-invalidate. There is the slight issue that those caches will grow indefinitely, but we could take them on if there's a legitimate issue.

davidfowl · 2021-06-11T22:17:12Z

src/Mvc/Mvc.Core/src/HotReload/HotReloadService.cs

+ IControllerPropertyActivator controllerPropertyActivator)
+ {
+ ClearCacheEvent += NotifyClearCache;
+ if (modelMetadataProvider.GetType() == typeof(DefaultModelMetadataProvider))


This type isn't sealed?

It's not. I've seen a couple users implement it. It feels weird to delete the caches under them.

davidfowl · 2021-06-11T22:17:21Z

src/Mvc/Mvc.Core/src/HotReload/HotReloadService.cs

+ _modelMetadataProvider = (DefaultModelMetadataProvider)modelMetadataProvider;
+ }
+
+ if (controllerPropertyActivator is DefaultControllerPropertyActivator defaultControllerPropertyActivator)


This type is sealed?

Yup. internal sealed. An alternative to doing this is to put the handler in the type itself, but I wanted to try and contain all of the clearing bits in one place particularly because there's indirection involved turning a static method call in to clearing per-instance caches.

davidfowl · 2021-06-11T22:19:19Z

src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadataProvider.cs

@@ -442,10 +444,6 @@ protected virtual DefaultMetadataDetails CreateParameterDetails(ModelMetadataIde
 ModelAttributes.GetAttributesForParameter(key.ParameterInfo!, key.ModelType));
 }

- private class ModelMetadataCache : ConcurrentDictionary<ModelMetadataIdentity, ModelMetadataCacheEntry>


I don't even want to ask why this was there

davidfowl

Is this exhaustive?

pranavkm · 2021-06-11T22:38:30Z

This doesn't clean up SignalR as yet. That would be next in the list. But at least from trying this out this did enough to rebuild ActionDescriptors and ApiDescriptors for MVC and Razor Pages.

Clear some caches from MVC on hot reload event

b545cdd

* Trigger action descriptor provider on hot reload. * Clear other caches that are keyed off of Type * Clean up MvcSandbox

pranavkm requested review from javiercn, Pilchie and a team as code owners June 8, 2021 23:22

pranavkm requested a review from davidfowl June 8, 2021 23:22

pranavkm commented Jun 8, 2021

View reviewed changes

mkArtakMSFT added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jun 9, 2021

pranavkm mentioned this pull request Jun 11, 2021

Update code in ASP.NET Core that detect if the app is being hot reloaded #33477

Closed

davidfowl reviewed Jun 11, 2021

View reviewed changes

davidfowl approved these changes Jun 11, 2021

View reviewed changes

pranavkm merged commit 63fb269 into main Jun 11, 2021

pranavkm deleted the prkrishn/more-cache-clearing branch June 11, 2021 23:01

ghost added this to the 6.0-preview6 milestone Jun 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear some caches from MVC on hot reload event #33386

Clear some caches from MVC on hot reload event #33386

pranavkm commented Jun 8, 2021

pranavkm Jun 8, 2021

pranavkm Jun 8, 2021

Tratcher Jun 8, 2021

DamianEdwards Jun 8, 2021

Tratcher Jun 8, 2021

pranavkm Jun 8, 2021

pranavkm Jun 9, 2021

mikem8361 Jun 9, 2021

lambdageek Jun 10, 2021

lambdageek Jun 10, 2021 •

edited

Loading

pranavkm Jun 11, 2021

pranavkm Jun 8, 2021

davidfowl Jun 11, 2021

pranavkm Jun 11, 2021

davidfowl Jun 11, 2021

pranavkm Jun 11, 2021

davidfowl Jun 11, 2021

davidfowl left a comment

pranavkm commented Jun 11, 2021

		private readonly DefaultModelMetadataProvider? _modelMetadataProvider;
		private readonly DefaultControllerPropertyActivator? _controllerPropertyActivator;

Clear some caches from MVC on hot reload event #33386

Clear some caches from MVC on hot reload event #33386

Conversation

pranavkm commented Jun 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl left a comment

Choose a reason for hiding this comment

pranavkm commented Jun 11, 2021

lambdageek Jun 10, 2021 •

edited

Loading