Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clear some caches from MVC on hot reload event #33386

Merged
merged 1 commit into from
Jun 11, 2021
Merged

Clear some caches from MVC on hot reload event #33386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Components/Web.JS/dist/Release/blazor.server.js
View file
Open in desktop

Large diffs are not rendered by default.

14 changes: 4 additions & 10 deletions src/Mvc/Mvc.Core/src/Controllers/DefaultControllerPropertyActivator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,19 @@
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Threading;
using Microsoft.AspNetCore.Mvc.Core;
using Microsoft.Extensions.Internal;

namespace Microsoft.AspNetCore.Mvc.Controllers
{
internal class DefaultControllerPropertyActivator : IControllerPropertyActivator
internal sealed class DefaultControllerPropertyActivator : IControllerPropertyActivator
{
private static readonly Func<Type, PropertyActivator<ControllerContext>[]> _getPropertiesToActivate =
GetPropertiesToActivate;
private object _initializeLock = new object();
private bool _initialized;
private ConcurrentDictionary<Type, PropertyActivator<ControllerContext>[]>? _activateActions;
private readonly ConcurrentDictionary<Type, PropertyActivator<ControllerContext>[]> _activateActions = new();

public void Activate(ControllerContext context, object controller)
{
LazyInitializer.EnsureInitialized(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just silly.

ref _activateActions,
ref _initialized,
ref _initializeLock);

var controllerType = controller.GetType();
var propertiesToActivate = _activateActions!.GetOrAdd(
controllerType,
Expand All @@ -38,6 +30,8 @@ public void Activate(ControllerContext context, object controller)
}
}

public void ClearCache() => _activateActions.Clear();

public Action<ControllerContext, object> GetActivatorDelegate(ControllerActionDescriptor actionDescriptor)
{
if (actionDescriptor == null)
Expand Down
14 changes: 11 additions & 3 deletions src/Mvc/Mvc.Core/src/DependencyInjection/MvcCoreServiceCollectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@
using Microsoft.AspNetCore.Mvc.ApplicationParts;
using Microsoft.AspNetCore.Mvc.Controllers;
using Microsoft.AspNetCore.Mvc.Filters;
using Microsoft.AspNetCore.Mvc.HotReload;
using Microsoft.AspNetCore.Mvc.Infrastructure;
using Microsoft.AspNetCore.Mvc.ModelBinding;
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
using Microsoft.AspNetCore.Mvc.Routing;
using Microsoft.AspNetCore.Routing;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Options;

namespace Microsoft.Extensions.DependencyInjection
Expand Down Expand Up @@ -51,13 +53,20 @@ public static IMvcCoreBuilder AddMvcCore(this IServiceCollection services)
throw new ArgumentNullException(nameof(services));
}

var partManager = GetApplicationPartManager(services);
var environment = GetServiceFromCollection<IWebHostEnvironment>(services);
var partManager = GetApplicationPartManager(services, environment);
services.TryAddSingleton(partManager);

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

if (environment?.IsDevelopment() ?? false)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI @Tratcher. I hear this makes you sad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please. #28937

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@lambdageek lambdageek Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
services.TryAddEnumerable(
ServiceDescriptor.Singleton<IActionDescriptorChangeProvider, HotReloadService>());
}

var builder = new MvcCoreBuilder(services, partManager);

return builder;
Expand All @@ -71,14 +80,13 @@ private static void ConfigureDefaultFeatureProviders(ApplicationPartManager mana
}
}

private static ApplicationPartManager GetApplicationPartManager(IServiceCollection services)
private static ApplicationPartManager GetApplicationPartManager(IServiceCollection services, IWebHostEnvironment? environment)
{
var manager = GetServiceFromCollection<ApplicationPartManager>(services);
if (manager == null)
{
manager = new ApplicationPartManager();

var environment = GetServiceFromCollection<IWebHostEnvironment>(services);
var entryAssemblyName = environment?.ApplicationName;
if (string.IsNullOrEmpty(entryAssemblyName))
{
Expand Down
64 changes: 64 additions & 0 deletions src/Mvc/Mvc.Core/src/HotReload/HotReloadService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Reflection.Metadata;
using System.Threading;
using Microsoft.AspNetCore.Mvc.Controllers;
using Microsoft.AspNetCore.Mvc.Infrastructure;
using Microsoft.AspNetCore.Mvc.ModelBinding;
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
using Microsoft.Extensions.Primitives;

[assembly: MetadataUpdateHandler(typeof(Microsoft.AspNetCore.Mvc.HotReload.HotReloadService))]

namespace Microsoft.AspNetCore.Mvc.HotReload
{
internal sealed class HotReloadService : IActionDescriptorChangeProvider, IDisposable
{
private readonly DefaultModelMetadataProvider? _modelMetadataProvider;
private readonly DefaultControllerPropertyActivator? _controllerPropertyActivator;
Comment on lines +19 to +20
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private CancellationTokenSource _tokenSource = new();

public HotReloadService(
IModelMetadataProvider modelMetadataProvider,
IControllerPropertyActivator controllerPropertyActivator)
{
ClearCacheEvent += NotifyClearCache;
if (modelMetadataProvider.GetType() == typeof(DefaultModelMetadataProvider))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type isn't sealed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
_modelMetadataProvider = (DefaultModelMetadataProvider)modelMetadataProvider;
}

if (controllerPropertyActivator is DefaultControllerPropertyActivator defaultControllerPropertyActivator)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type is sealed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
_controllerPropertyActivator = defaultControllerPropertyActivator;
}
}

public static event Action? ClearCacheEvent;

public static void ClearCache(Type[]? _)
{
ClearCacheEvent?.Invoke();
}

IChangeToken IActionDescriptorChangeProvider.GetChangeToken() => new CancellationChangeToken(_tokenSource.Token);

private void NotifyClearCache()
{
// Trigger the ActionDescriptorChangeProvider
var current = Interlocked.Exchange(ref _tokenSource, new CancellationTokenSource());
current.Cancel();

// Clear individual caches
_modelMetadataProvider?.ClearCache();
_controllerPropertyActivator?.ClearCache();
}

public void Dispose()
{
ClearCacheEvent -= NotifyClearCache;
}
}
}
8 changes: 3 additions & 5 deletions src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadataProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata
/// </summary>
public class DefaultModelMetadataProvider : ModelMetadataProvider
{
private readonly ModelMetadataCache _modelMetadataCache = new ModelMetadataCache();
private readonly ConcurrentDictionary<ModelMetadataIdentity, ModelMetadataCacheEntry> _modelMetadataCache = new();
private readonly Func<ModelMetadataIdentity, ModelMetadataCacheEntry> _cacheEntryFactory;
private readonly ModelMetadataCacheEntry _metadataCacheEntryForObjectType;

Expand Down Expand Up @@ -71,6 +71,8 @@ private DefaultModelMetadataProvider(
/// <value>Same as <see cref="MvcOptions.ModelBindingMessageProvider"/> in all production scenarios.</value>
protected DefaultModelBindingMessageProvider ModelBindingMessageProvider { get; }

internal void ClearCache() => _modelMetadataCache.Clear();

/// <inheritdoc />
public override IEnumerable<ModelMetadata> GetMetadataForProperties(Type modelType)
{
Expand Down Expand Up @@ -442,10 +444,6 @@ protected virtual DefaultMetadataDetails CreateParameterDetails(ModelMetadataIde
ModelAttributes.GetAttributesForParameter(key.ParameterInfo!, key.ModelType));
}

private class ModelMetadataCache : ConcurrentDictionary<ModelMetadataIdentity, ModelMetadataCacheEntry>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
}

private readonly struct ModelMetadataCacheEntry
{
public ModelMetadataCacheEntry(ModelMetadata metadata, DefaultMetadataDetails details)
Expand Down
8 changes: 4 additions & 4 deletions src/Mvc/Mvc.Core/test/DependencyInjection/MvcCoreServiceCollectionExtensionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public void AddMvcCore_UsesOriginalHostingEnvironment()
{
// Arrange
var services = new ServiceCollection();
var environment = new Mock<IWebHostEnvironment>(MockBehavior.Strict);
var environment = new Mock<IWebHostEnvironment>();
environment.SetupGet(e => e.ApplicationName).Returns((string)null).Verifiable();
services.AddSingleton<IWebHostEnvironment>(environment.Object);

Expand All @@ -173,10 +173,10 @@ public void AddMvcCore_UsesLastHostingEnvironment()
{
// Arrange
var services = new ServiceCollection();
var environment = new Mock<IWebHostEnvironment>(MockBehavior.Strict);
var environment = new Mock<IWebHostEnvironment>();
services.AddSingleton<IWebHostEnvironment>(environment.Object);

environment = new Mock<IWebHostEnvironment>(MockBehavior.Strict);
environment = new Mock<IWebHostEnvironment>();
environment.SetupGet(e => e.ApplicationName).Returns((string)null).Verifiable();
services.AddSingleton<IWebHostEnvironment>(environment.Object);

Expand All @@ -196,7 +196,7 @@ public void AddMvcCore_GetsPartsForApplication()
{
// Arrange
var services = new ServiceCollection();
var environment = new Mock<IWebHostEnvironment>(MockBehavior.Strict);
var environment = new Mock<IWebHostEnvironment>();
var assemblyName = typeof(MvcCoreServiceCollectionExtensionsTest).Assembly.GetName();
var applicationName = assemblyName.FullName;
environment.SetupGet(e => e.ApplicationName).Returns(applicationName).Verifiable();
Expand Down
13 changes: 0 additions & 13 deletions src/Mvc/samples/MvcSandbox/Components/App.razor
View file
Open in desktop

This file was deleted.

2 changes: 0 additions & 2 deletions src/Mvc/samples/MvcSandbox/Components/Pages/Index.razor
View file
Open in desktop

This file was deleted.

15 changes: 0 additions & 15 deletions src/Mvc/samples/MvcSandbox/Components/Shared/MainLayout.razor
View file
Open in desktop

This file was deleted.

29 changes: 0 additions & 29 deletions src/Mvc/samples/MvcSandbox/Components/Shared/NavMenu.razor
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion src/Mvc/samples/MvcSandbox/Components/_Imports.razor
View file
Open in desktop

This file was deleted.

3 changes: 0 additions & 3 deletions src/Mvc/samples/MvcSandbox/MvcSandbox.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,9 @@

<ItemGroup>
<Reference Include="Microsoft.AspNetCore.Mvc" />
<Reference Include="Microsoft.AspNetCore.Components.Server" />
<Reference Include="Microsoft.AspNetCore.Diagnostics" />
<Reference Include="Microsoft.AspNetCore.Server.IISIntegration" />
<Reference Include="Microsoft.AspNetCore.Server.Kestrel" />
<Reference Include="Microsoft.AspNetCore.StaticFiles" />
<Reference Include="Microsoft.Extensions.Configuration.Json" />
<Reference Include="Microsoft.Extensions.Logging.Console" />
<Reference Include="Microsoft.Extensions.Logging.Debug" />
</ItemGroup>
Expand Down
20 changes: 0 additions & 20 deletions src/Mvc/samples/MvcSandbox/Pages/Components.cshtml
View file
Open in desktop

This file was deleted.

19 changes: 0 additions & 19 deletions src/Mvc/samples/MvcSandbox/SlugifyParameterTransformer.cs
View file
Open in desktop

This file was deleted.

Loading