Skip to content

Conversation

@AndyButland
Copy link
Contributor

Prerequisites

Addresses #20709

Description

The linked issue points out a potential problem with how we are handling IOptionsMonitor.OnChange in components that aren't registered as singletons.

Why the current pattern is a concern:

IOptionsMonitor<T>.OnChange(...) registers a listener and returns an IDisposable. The code calls deliveryApiSettings.OnChange(settings => _deliveryApiSettings = settings) but does not keep or dispose the returned IDisposable.

The lambda settings => _deliveryApiSettings = settings captures the instance (this), so the monitor holds a strong reference to the instance via the delegate. If the class is registered with a shorter lifetime than the app (e.g., scoped or transient) and the returned IDisposable is never disposed, the instance cannot be garbage collected — leaking memory (and potentially other resources).

I've checked all instances of this and found only the classes amended in this PR to not be registered as singletons. So other instances are safe. I've resolved by either:

  • Add a field private readonly IDisposable? _optionsChangeSubscription;, assigning it in the constructor, implementing IDisposable and disposing of the reference.
  • For controllers, just removed the OnChange - as we won't need it given controllers are transient.

Copilot AI review requested due to automatic review settings November 3, 2025 19:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a memory leak issue by properly disposing of IOptionsMonitor.OnChange subscriptions. The changes ensure that change tracking callbacks registered with IOptionsMonitor.OnChange are properly cleaned up when the classes are disposed.

  • Converts mutable fields to readonly where subscriptions are now properly tracked
  • Captures the IDisposable returned by OnChange to enable proper cleanup
  • Implements IDisposable on classes that subscribe to option changes to dispose of subscriptions

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/Umbraco.Web.BackOffice/Controllers/ImagesController.cs Made _contentSettings field readonly and removed dynamic change tracking
src/Umbraco.Web.BackOffice/Controllers/HelpController.cs Made _helpPageSettings field readonly and removed dynamic change tracking
src/Umbraco.Infrastructure/PropertyEditors/ValueConverters/RteMacroRenderingValueConverter.cs Implemented IDisposable and captured OnChange subscription for proper disposal
src/Umbraco.Infrastructure/PropertyEditors/UploadFileTypeValidator.cs Implemented IDisposable and captured OnChange subscription for proper disposal
src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyValueEditor.cs Implemented IDisposable and captured OnChange subscription for proper disposal
src/Umbraco.Infrastructure/PropertyEditors/ImageCropperPropertyEditor.cs Implemented IDisposable and captured OnChange subscription for proper disposal
src/Umbraco.Infrastructure/PropertyEditors/FileUploadPropertyValueEditor.cs Implemented IDisposable and captured OnChange subscription for proper disposal

@AndyButland AndyButland changed the title Fix memory leak with IOptionsMonitor.OnChange and non-singleton registered components. Fix memory leak with IOptionsMonitor.OnChange and non-singleton registered components (closes #20709 for 13) Nov 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants