Fix memory leak with IOptionsMonitor.OnChange and non-singleton registered components (closes #20709 for 16/17) (#20723)

* Fix memory leak with IOptionsMonitor.OnChange and non-singleton registered components.

* Dispose disposable data editors in ValueEditorCache.

* Removed unnecessary refactoring and clarified code comments.

Author: AndyButland

src/Umbraco.Cms.Api.Management/Controllers/Help/GetHelpController.cs

@@ -17,7 +17,7 @@ public class GetHelpController : HelpControllerBase
 {
     private readonly ILogger<GetHelpController> _logger;
     private readonly IJsonSerializer _jsonSerializer;
-    private HelpPageSettings _helpPageSettings;
+    private readonly HelpPageSettings _helpPageSettings;
 
     public GetHelpController(
         IOptionsMonitor<HelpPageSettings> helpPageSettings,
@@ -27,11 +27,8 @@ public GetHelpController(
         _logger = logger;
         _jsonSerializer = jsonSerializer;
         _helpPageSettings = helpPageSettings.CurrentValue;
-        helpPageSettings.OnChange(UpdateHelpPageSettings);
     }
 
-    private void UpdateHelpPageSettings(HelpPageSettings settings) => _helpPageSettings = settings;
-
     [HttpGet]
     [MapToApiVersion("1.0")]
     [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)]

src/Umbraco.Cms.Api.Management/Controllers/ModelsBuilder/BuildModelsBuilderController.cs

@@ -1,22 +1,18 @@
-using System;
-using System.Threading.Tasks;
 using Asp.Versioning;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc;
-using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Options;
 using Umbraco.Cms.Core;
 using Umbraco.Cms.Core.Configuration.Models;
 using Umbraco.Cms.Infrastructure.ModelsBuilder;
 using Umbraco.Cms.Infrastructure.ModelsBuilder.Building;
-using Umbraco.Extensions;
 
 namespace Umbraco.Cms.Api.Management.Controllers.ModelsBuilder;
 
 [ApiVersion("1.0")]
 public class BuildModelsBuilderController : ModelsBuilderControllerBase
 {
-    private ModelsBuilderSettings _modelsBuilderSettings;
+    private readonly ModelsBuilderSettings _modelsBuilderSettings;
     private readonly ModelsGenerationError _mbErrors;
     private readonly IModelsGenerator _modelGenerator;
 
@@ -28,8 +24,6 @@ public BuildModelsBuilderController(
         _mbErrors = mbErrors;
         _modelGenerator = modelGenerator;
         _modelsBuilderSettings = modelsBuilderSettings.CurrentValue;
-
-        modelsBuilderSettings.OnChange(x => _modelsBuilderSettings = x);
     }
 
     [HttpPost("build")]

src/Umbraco.Cms.Api.Management/Factories/ModelsBuilderPresentationFactory.cs

@@ -1,30 +1,24 @@
-using System.Text;
 using Microsoft.Extensions.Options;
+using Umbraco.Cms.Api.Management.ViewModels.ModelsBuilderDashboard;
 using Umbraco.Cms.Core;
-using Umbraco.Cms.Core.Configuration;
 using Umbraco.Cms.Core.Configuration.Models;
 using Umbraco.Cms.Infrastructure.ModelsBuilder;
-using Umbraco.Cms.Api.Management.ViewModels.ModelsBuilderDashboard;
-using Umbraco.Extensions;
 
 namespace Umbraco.Cms.Api.Management.Factories;
 
 public class ModelsBuilderPresentationFactory : IModelsBuilderPresentationFactory
 {
-    private ModelsBuilderSettings _modelsBuilderSettings;
     private readonly ModelsGenerationError _mbErrors;
     private readonly OutOfDateModelsStatus _outOfDateModels;
+    private readonly ModelsBuilderSettings _modelsBuilderSettings;
 
     public ModelsBuilderPresentationFactory(IOptionsMonitor<ModelsBuilderSettings> modelsBuilderSettings, ModelsGenerationError mbErrors, OutOfDateModelsStatus outOfDateModels)
     {
         _mbErrors = mbErrors;
         _outOfDateModels = outOfDateModels;
         _modelsBuilderSettings = modelsBuilderSettings.CurrentValue;
-
-        modelsBuilderSettings.OnChange(x => _modelsBuilderSettings = x);
     }
 
-
     public ModelsBuilderResponseModel Create() =>
         new()
         {

src/Umbraco.Core/Cache/ValueEditorCache.cs

@@ -51,7 +51,15 @@ public void ClearCache(IEnumerable<int> dataTypeIds)
             {
                 foreach (Dictionary<int, IDataValueEditor> editors in _valueEditorCache.Values)
                 {
-                    editors.Remove(id);
+                    if (editors.TryGetValue(id, out IDataValueEditor? editor))
+                    {
+                        if (editor is IDisposable disposable)
+                        {
+                            disposable.Dispose();
+                        }
+
+                        editors.Remove(id);
+                    }
                 }
             }
         }

src/Umbraco.Core/DeliveryApi/ApiPublishedContentCache.cs

@@ -1,10 +1,7 @@
-using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Options;
 using Umbraco.Cms.Core.Configuration.Models;
-using Umbraco.Cms.Core.DependencyInjection;
 using Umbraco.Cms.Core.Models.PublishedContent;
 using Umbraco.Cms.Core.PublishedCache;
-using Umbraco.Cms.Core.Services;
 using Umbraco.Extensions;
 
 namespace Umbraco.Cms.Core.DeliveryApi;

src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs

@@ -2,6 +2,7 @@
 // See LICENSE for more details.
 
 using Microsoft.Extensions.Options;
+using Umbraco.Cms.Core.Cache;
 using Umbraco.Cms.Core.Configuration.Models;
 using Umbraco.Cms.Core.IO;
 using Umbraco.Cms.Core.Models;
@@ -22,14 +23,22 @@ namespace Umbraco.Cms.Core.PropertyEditors;
 /// <summary>
 ///     The value editor for the file upload property editor.
 /// </summary>
-internal sealed class FileUploadPropertyValueEditor : DataValueEditor
+/// <remarks>
+///     As this class is loaded into <see cref="ValueEditorCache"/> which can be cleared, it needs
+///     to be disposable in order to properly clean up resources such as
+///     the settings change subscription and avoid a memory leak.
+/// </remarks>
+internal sealed class FileUploadPropertyValueEditor : DataValueEditor, IDisposable
 {
     private readonly MediaFileManager _mediaFileManager;
     private readonly ITemporaryFileService _temporaryFileService;
     private readonly IScopeProvider _scopeProvider;
     private readonly IFileStreamSecurityValidator _fileStreamSecurityValidator;
     private readonly FileUploadValueParser _valueParser;
+
     private ContentSettings _contentSettings;
+    private readonly IDisposable? _contentSettingsChangeSubscription;
+
 
     /// <summary>
     /// Initializes a new instance of the <see cref="FileUploadPropertyValueEditor"/> class.
@@ -46,14 +55,14 @@ public FileUploadPropertyValueEditor(
         IFileStreamSecurityValidator fileStreamSecurityValidator)
         : base(shortStringHelper, jsonSerializer, ioHelper, attribute)
     {
-        _mediaFileManager = mediaFileManager ?? throw new ArgumentNullException(nameof(mediaFileManager));
+        _mediaFileManager = mediaFileManager;
         _temporaryFileService = temporaryFileService;
         _scopeProvider = scopeProvider;
         _fileStreamSecurityValidator = fileStreamSecurityValidator;
         _valueParser = new FileUploadValueParser(jsonSerializer);
 
-        _contentSettings = contentSettings.CurrentValue ?? throw new ArgumentNullException(nameof(contentSettings));
-        contentSettings.OnChange(x => _contentSettings = x);
+        _contentSettings = contentSettings.CurrentValue;
+        _contentSettingsChangeSubscription = contentSettings.OnChange(x => _contentSettings = x);
 
         Validators.Add(new TemporaryFileUploadValidator(
             () => _contentSettings,
@@ -216,4 +225,6 @@ private string GetMediaPath(TemporaryFileModel file, object? dataTypeConfigurati
         // in case we are using the old path scheme, try to re-use numbers (bah...)
         return _mediaFileManager.GetMediaPath(file.FileName, contentKey, propertyTypeKey);
     }
+
+    public void Dispose() => _contentSettingsChangeSubscription?.Dispose();
 }

src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyEditor.cs

@@ -1,11 +1,9 @@
 // Copyright (c) Umbraco.
 // See LICENSE for more details.
 
-using Microsoft.Extensions.DependencyInjection;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Umbraco.Cms.Core.Configuration.Models;
-using Umbraco.Cms.Core.DependencyInjection;
 using Umbraco.Cms.Core.Events;
 using Umbraco.Cms.Core.IO;
 using Umbraco.Cms.Core.Media;
@@ -26,7 +24,8 @@ namespace Umbraco.Cms.Core.PropertyEditors;
     Constants.PropertyEditors.Aliases.ImageCropper,
     ValueType = ValueTypes.Json,
     ValueEditorIsReusable = true)]
-public class ImageCropperPropertyEditor : DataEditor, IMediaUrlGenerator,
+public class ImageCropperPropertyEditor : DataEditor,
+    IMediaUrlGenerator,
     INotificationHandler<ContentCopiedNotification>,
     INotificationHandler<ContentDeletedNotification>,
     INotificationHandler<MediaDeletedNotification>,
@@ -40,9 +39,10 @@ public class ImageCropperPropertyEditor : DataEditor, IMediaUrlGenerator,
     private readonly IIOHelper _ioHelper;
     private readonly ILogger<ImageCropperPropertyEditor> _logger;
     private readonly MediaFileManager _mediaFileManager;
-    private ContentSettings _contentSettings;
     private readonly IJsonSerializer _jsonSerializer;
 
+    private ContentSettings _contentSettings;
+
     /// <summary>
     ///     Initializes a new instance of the <see cref="ImageCropperPropertyEditor" /> class.
     /// </summary>
@@ -57,15 +57,14 @@ public ImageCropperPropertyEditor(
         IJsonSerializer jsonSerializer)
         : base(dataValueEditorFactory)
     {
-        _mediaFileManager = mediaFileManager ?? throw new ArgumentNullException(nameof(mediaFileManager));
-        _contentSettings = contentSettings.CurrentValue ?? throw new ArgumentNullException(nameof(contentSettings));
-        _ioHelper = ioHelper ?? throw new ArgumentNullException(nameof(ioHelper));
-        _autoFillProperties =
-            uploadAutoFillProperties ?? throw new ArgumentNullException(nameof(uploadAutoFillProperties));
+        _mediaFileManager = mediaFileManager;
+        _ioHelper = ioHelper;
+        _autoFillProperties = uploadAutoFillProperties;
         _contentService = contentService;
         _jsonSerializer = jsonSerializer;
         _logger = loggerFactory.CreateLogger<ImageCropperPropertyEditor>();
 
+        _contentSettings = contentSettings.CurrentValue;
         contentSettings.OnChange(x => _contentSettings = x);
 
         SupportsReadOnly = true;

src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs

@@ -22,17 +22,24 @@ namespace Umbraco.Cms.Core.PropertyEditors;
 /// <summary>
 ///     The value editor for the image cropper property editor.
 /// </summary>
-internal sealed class ImageCropperPropertyValueEditor : DataValueEditor // TODO: core vs web?
+/// <remarks>
+///     As this class is loaded into <see cref="ValueEditorCache"/> which can be cleared, it needs
+///     to be disposable in order to properly clean up resources such as
+///     the settings change subscription and avoid a memory leak.
+/// </remarks>
+internal sealed class ImageCropperPropertyValueEditor : DataValueEditor, IDisposable
 {
     private readonly IDataTypeConfigurationCache _dataTypeConfigurationCache;
     private readonly IFileStreamSecurityValidator _fileStreamSecurityValidator;
     private readonly ILogger<ImageCropperPropertyValueEditor> _logger;
     private readonly MediaFileManager _mediaFileManager;
     private readonly IJsonSerializer _jsonSerializer;
-    private ContentSettings _contentSettings;
     private readonly ITemporaryFileService _temporaryFileService;
     private readonly IScopeProvider _scopeProvider;
 
+    private ContentSettings _contentSettings;
+    private readonly IDisposable? _contentSettingsChangeSubscription;
+
     public ImageCropperPropertyValueEditor(
         DataEditorAttribute attribute,
         ILogger<ImageCropperPropertyValueEditor> logger,
@@ -50,12 +57,13 @@ public ImageCropperPropertyValueEditor(
         _logger = logger ?? throw new ArgumentNullException(nameof(logger));
         _mediaFileManager = mediaFileSystem ?? throw new ArgumentNullException(nameof(mediaFileSystem));
         _jsonSerializer = jsonSerializer;
-        _contentSettings = contentSettings.CurrentValue;
         _temporaryFileService = temporaryFileService;
         _scopeProvider = scopeProvider;
         _fileStreamSecurityValidator = fileStreamSecurityValidator;
         _dataTypeConfigurationCache = dataTypeConfigurationCache;
-        contentSettings.OnChange(x => _contentSettings = x);
+
+        _contentSettings = contentSettings.CurrentValue;
+        _contentSettingsChangeSubscription = contentSettings.OnChange(x => _contentSettings = x);
 
         Validators.Add(new TemporaryFileUploadValidator(() => _contentSettings, TryParseTemporaryFileKey, TryGetTemporaryFile));
     }
@@ -267,4 +275,6 @@ public override string ConvertDbToString(IPropertyType propertyType, object? val
 
         return filepath;
     }
+
+    public void Dispose() => _contentSettingsChangeSubscription?.Dispose();
 }

src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/RteBlockRenderingValueConverter.cs

@@ -26,7 +26,7 @@ namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;
 ///     used dynamically.
 /// </summary>
 [DefaultPropertyValueConverter]
-public class RteBlockRenderingValueConverter : SimpleRichTextValueConverter, IDeliveryApiPropertyValueConverter
+public class RteBlockRenderingValueConverter : SimpleRichTextValueConverter, IDeliveryApiPropertyValueConverter, IDisposable
 {
     private readonly HtmlImageSourceParser _imageSourceParser;
     private readonly HtmlLocalLinkParser _linkParser;
@@ -41,7 +41,9 @@ public class RteBlockRenderingValueConverter : SimpleRichTextValueConverter, IDe
     private readonly RichTextBlockPropertyValueConstructorCache _constructorCache;
     private readonly IVariationContextAccessor _variationContextAccessor;
     private readonly BlockEditorVarianceHandler _blockEditorVarianceHandler;
+
     private DeliveryApiSettings _deliveryApiSettings;
+    private readonly IDisposable? _deliveryApiSettingsChangeSubscription;
 
     public RteBlockRenderingValueConverter(HtmlLocalLinkParser linkParser, HtmlUrlParser urlParser, HtmlImageSourceParser imageSourceParser,
         IApiRichTextElementParser apiRichTextElementParser, IApiRichTextMarkupParser apiRichTextMarkupParser,
@@ -62,8 +64,9 @@ public RteBlockRenderingValueConverter(HtmlLocalLinkParser linkParser, HtmlUrlPa
         _logger = logger;
         _variationContextAccessor = variationContextAccessor;
         _blockEditorVarianceHandler = blockEditorVarianceHandler;
+
         _deliveryApiSettings = deliveryApiSettingsMonitor.CurrentValue;
-        deliveryApiSettingsMonitor.OnChange(settings => _deliveryApiSettings = settings);
+        _deliveryApiSettingsChangeSubscription = deliveryApiSettingsMonitor.OnChange(settings => _deliveryApiSettings = settings);
     }
 
     public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType) =>
@@ -250,4 +253,6 @@ private sealed class RichTextEditorIntermediateValue : IRichTextEditorIntermedia
 
         public required RichTextBlockModel? RichTextBlockModel { get; set; }
     }
+
+    public void Dispose() => _deliveryApiSettingsChangeSubscription?.Dispose();
 }

src/Umbraco.Infrastructure/Search/IndexingNotificationHandler.DeliveryApi.cs

@@ -1,4 +1,4 @@
-using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using Umbraco.Cms.Core.Cache;
 using Umbraco.Cms.Core.Configuration.Models;
@@ -17,7 +17,7 @@ internal sealed class DeliveryApiContentIndexingNotificationHandler :
 {
     private readonly IDeliveryApiIndexingHandler _deliveryApiIndexingHandler;
     private readonly ILogger<DeliveryApiContentIndexingNotificationHandler> _logger;
-    private DeliveryApiSettings _deliveryApiSettings;
+    private readonly DeliveryApiSettings _deliveryApiSettings;
 
     public DeliveryApiContentIndexingNotificationHandler(
         IDeliveryApiIndexingHandler deliveryApiIndexingHandler,
@@ -27,7 +27,6 @@ public DeliveryApiContentIndexingNotificationHandler(
         _deliveryApiIndexingHandler = deliveryApiIndexingHandler;
         _logger = logger;
         _deliveryApiSettings = deliveryApiSettings.CurrentValue;
-        deliveryApiSettings.OnChange(settings => _deliveryApiSettings = settings);
     }
 
     public void Handle(ContentCacheRefresherNotification notification)