Fix memory leak with IOptionsMonitor.OnChange and non-singleton registered components (closes #20709 for 13) (#20722)

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

* Added XML docs.

* Apply fix to DeliveryApiContentIndexingNotificationHandler.

* Dispose disposable data editors in ValueEditorCache.

* Removed unnecessary refactoring and clarified code comments.

Author: AndyButland

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.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.Editors;
@@ -16,11 +17,18 @@ namespace Umbraco.Cms.Core.PropertyEditors;
 /// <summary>
 ///     The value editor for the file upload property editor.
 /// </summary>
-internal 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 class FileUploadPropertyValueEditor : DataValueEditor, IDisposable
 {
     private readonly MediaFileManager _mediaFileManager;
     private readonly IFileStreamSecurityValidator _fileStreamSecurityValidator;
+
     private ContentSettings _contentSettings;
+    private readonly IDisposable? _contentSettingsChangeSubscription;
 
     public FileUploadPropertyValueEditor(
         DataEditorAttribute attribute,
@@ -36,7 +44,7 @@ public FileUploadPropertyValueEditor(
         _mediaFileManager = mediaFileManager ?? throw new ArgumentNullException(nameof(mediaFileManager));
         _fileStreamSecurityValidator = fileStreamSecurityValidator;
         _contentSettings = contentSettings.CurrentValue ?? throw new ArgumentNullException(nameof(contentSettings));
-        contentSettings.OnChange(x => _contentSettings = x);
+        _contentSettingsChangeSubscription = contentSettings.OnChange(x => _contentSettings = x);
     }
 
     /// <summary>
@@ -163,4 +171,6 @@ public FileUploadPropertyValueEditor(
 
         return filepath;
     }
+
+    public void Dispose() => _contentSettingsChangeSubscription?.Dispose();
 }

src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyEditor.cs

@@ -21,6 +21,10 @@ namespace Umbraco.Cms.Core.PropertyEditors;
 /// <summary>
 ///     Represents an image cropper property editor.
 /// </summary>
+/// <remarks>
+///     As this class is not registered with DI as a singleton, it must be disposed to release
+///     the settings change subscription and avoid a memory leak.
+/// </remarks>
 [DataEditor(
     Constants.PropertyEditors.Aliases.ImageCropper,
     "Image Cropper",
@@ -42,6 +46,7 @@ public class ImageCropperPropertyEditor : DataEditor, IMediaUrlGenerator,
     private readonly IIOHelper _ioHelper;
     private readonly ILogger<ImageCropperPropertyEditor> _logger;
     private readonly MediaFileManager _mediaFileManager;
+
     private ContentSettings _contentSettings;
 
     // Scheduled for removal in v12

src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs

@@ -23,13 +23,20 @@ namespace Umbraco.Cms.Core.PropertyEditors;
 /// <summary>
 ///     The value editor for the image cropper property editor.
 /// </summary>
-internal 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 class ImageCropperPropertyValueEditor : DataValueEditor, IDisposable
 {
     private readonly IDataTypeConfigurationCache _dataTypeConfigurationCache;
     private readonly IFileStreamSecurityValidator _fileStreamSecurityValidator;
     private readonly ILogger<ImageCropperPropertyValueEditor> _logger;
     private readonly MediaFileManager _mediaFileManager;
+
     private ContentSettings _contentSettings;
+    private readonly IDisposable? _contentSettingsChangeSubscription;
 
     public ImageCropperPropertyValueEditor(
         DataEditorAttribute attribute,
@@ -49,7 +56,7 @@ public ImageCropperPropertyValueEditor(
         _contentSettings = contentSettings.CurrentValue;
         _dataTypeConfigurationCache = dataTypeConfigurationCache;
         _fileStreamSecurityValidator = fileStreamSecurityValidator;
-        contentSettings.OnChange(x => _contentSettings = x);
+        _contentSettingsChangeSubscription = contentSettings.OnChange(x => _contentSettings = x);
     }
 
     /// <summary>
@@ -252,4 +259,6 @@ public override string ConvertDbToString(IPropertyType propertyType, object? val
 
         return filepath;
     }
+
+    public void Dispose() => _contentSettingsChangeSubscription?.Dispose();
 }

src/Umbraco.Infrastructure/PropertyEditors/UploadFileTypeValidator.cs

@@ -11,10 +11,19 @@
 
 namespace Umbraco.Cms.Core.PropertyEditors;
 
-internal class UploadFileTypeValidator : IValueValidator
+/// <summary>
+///     The value editor for file upload property editors.
+/// </summary>
+/// <remarks>
+///     As this class is not registered with DI as a singleton, it must be disposed to release
+///     the settings change subscription and avoid a memory leak.
+/// </remarks>
+internal class UploadFileTypeValidator : IValueValidator, IDisposable
 {
     private readonly ILocalizedTextService _localizedTextService;
+
     private ContentSettings _contentSettings;
+    private readonly IDisposable? _contentSettingsChangeSubscription;
 
     public UploadFileTypeValidator(
         ILocalizedTextService localizedTextService,
@@ -23,7 +32,7 @@ public UploadFileTypeValidator(
         _localizedTextService = localizedTextService;
         _contentSettings = contentSettings.CurrentValue;
 
-        contentSettings.OnChange(x => _contentSettings = x);
+        _contentSettingsChangeSubscription = contentSettings.OnChange(x => _contentSettings = x);
     }
 
     public IEnumerable<ValidationResult> Validate(object? value, string? valueType, object? dataTypeConfiguration)
@@ -103,4 +112,6 @@ internal static bool TryGetFileExtension(string? fileName, [MaybeNullWhen(false)
         extension = fileName.GetFileExtension().TrimStart(".");
         return true;
     }
+
+    public void Dispose() => _contentSettingsChangeSubscription?.Dispose();
 }

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

@@ -31,8 +31,12 @@ namespace Umbraco.Cms.Core.PropertyEditors.ValueConverters;
 ///     A value converter for TinyMCE that will ensure any macro content is rendered properly even when
 ///     used dynamically.
 /// </summary>
+/// <remarks>
+///     As this class is not registered with DI as a singleton, it must be disposed to release
+///     the settings change subscription and avoid a memory leak.
+/// </remarks>
 [DefaultPropertyValueConverter]
-public class RteMacroRenderingValueConverter : SimpleTinyMceValueConverter, IDeliveryApiPropertyValueConverter
+public class RteMacroRenderingValueConverter : SimpleTinyMceValueConverter, IDeliveryApiPropertyValueConverter, IDisposable
 {
     private readonly HtmlImageSourceParser _imageSourceParser;
     private readonly HtmlLocalLinkParser _linkParser;
@@ -47,7 +51,9 @@ public class RteMacroRenderingValueConverter : SimpleTinyMceValueConverter, IDel
     private readonly ILogger<RteMacroRenderingValueConverter> _logger;
     private readonly IApiElementBuilder _apiElementBuilder;
     private readonly RichTextBlockPropertyValueConstructorCache _constructorCache;
+
     private DeliveryApiSettings _deliveryApiSettings;
+    private readonly IDisposable? _deliveryApiSettingsChangeSubscription;
 
     [Obsolete("Please use the constructor that takes all arguments. Will be removed in V14.")]
     public RteMacroRenderingValueConverter(IUmbracoContextAccessor umbracoContextAccessor, IMacroRenderer macroRenderer,
@@ -107,8 +113,9 @@ public RteMacroRenderingValueConverter(IUmbracoContextAccessor umbracoContextAcc
         _apiElementBuilder = apiElementBuilder;
         _constructorCache = constructorCache;
         _logger = logger;
+
         _deliveryApiSettings = deliveryApiSettingsMonitor.CurrentValue;
-        deliveryApiSettingsMonitor.OnChange(settings => _deliveryApiSettings = settings);
+        _deliveryApiSettingsChangeSubscription = deliveryApiSettingsMonitor.OnChange(settings => _deliveryApiSettings = settings);
     }
 
     public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType) =>
@@ -314,4 +321,6 @@ private class RichTextEditorIntermediateValue : IRichTextEditorIntermediateValue
 
         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)

src/Umbraco.Web.BackOffice/Controllers/HelpController.cs

@@ -6,7 +6,6 @@
 using Newtonsoft.Json;
 using Umbraco.Cms.Core;
 using Umbraco.Cms.Core.Configuration.Models;
-using Umbraco.Cms.Core.DependencyInjection;
 using Umbraco.Cms.Web.Common.Attributes;
 
 namespace Umbraco.Cms.Web.BackOffice.Controllers;
@@ -16,7 +15,8 @@ public class HelpController : UmbracoAuthorizedJsonController
 {
     private static HttpClient? _httpClient;
     private readonly ILogger<HelpController> _logger;
-    private HelpPageSettings? _helpPageSettings;
+
+    private readonly HelpPageSettings? _helpPageSettings;
 
     [ActivatorUtilitiesConstructor]
     public HelpController(
@@ -25,12 +25,9 @@ public HelpController(
     {
         _logger = logger;
 
-        ResetHelpPageSettings(helpPageSettings.CurrentValue);
-        helpPageSettings.OnChange(ResetHelpPageSettings);
+        _helpPageSettings = helpPageSettings.CurrentValue;
     }
 
-    private void ResetHelpPageSettings(HelpPageSettings settings) => _helpPageSettings = settings;
-
     public async Task<List<HelpPage>> GetContextHelpForPage(string section, string tree,
         string baseUrl = "https://our.umbraco.com")
     {

src/Umbraco.Web.BackOffice/Controllers/ImagesController.cs

@@ -23,7 +23,7 @@ public class ImagesController : UmbracoAuthorizedApiController
 {
     private readonly MediaFileManager _mediaFileManager;
     private readonly IImageUrlGenerator _imageUrlGenerator;
-    private ContentSettings _contentSettings;
+    private readonly ContentSettings _contentSettings;
 
     [Obsolete("Use non obsolete-constructor. Scheduled for removal in Umbraco 13.")]
     public ImagesController(
@@ -45,8 +45,6 @@ public ImagesController(
         _mediaFileManager = mediaFileManager;
         _imageUrlGenerator = imageUrlGenerator;
         _contentSettings = contentSettingsMonitor.CurrentValue;
-
-        contentSettingsMonitor.OnChange(x => _contentSettings = x);
     }
 
     /// <summary>