Skip to content

Backport Fix memory leak with OptionsMonitor #868 to .NET Core 2.1 #3552

Closed

Description

@shantha05

Issue
Currently there is a memory leak in using OptionsMonitor in .NET Core 2.1 and this has been fixed as part of .NET Core 3.0 and above. (#868)

Solution
Given .NET Core 2.1 has LTS support until 21-08-2021, is it possible to backport this fix to .NET Core 2.1

Alternatives considered
There is no alternative other than upgrading to .NET Core 3.1 and we have a lot of dependencies that needs to be considered which is not going to help us to resolve the memory leak issue in a timely fashion.

Activity

  1. davidfowl commented on Sep 22, 2020

    @davidfowl
    Member

    Did you run into this? If so where?

  2. shantha05 commented on Sep 22, 2020

    @shantha05
    Author

    Yes, we are using .NET Core 2.1 API hosted on Azure and we are seeing the memory leak behavior there.

  3. davidfowl commented on Sep 22, 2020

    @davidfowl
    Member

    How did you reproduce the issue? This particular one required multiple host instantiations and disposals. Did you track down what caused the problem in your case?

  4. shantha05 commented on Sep 22, 2020

    @shantha05
    Author

    We had multiple app services hosting this API on Azure and we noticed the memory leak behavior with the the implementation of OptionsMonitor and OnChange event and when I commented invoking the OnChange event the memory leak stopped.

  5. davidfowl commented on Sep 22, 2020

    @davidfowl
    Member

    @shantha05 were you disposing the IDisposable returned from OnChange in your scenario and it was still leaking?

  6. shantha05 commented on Sep 22, 2020

    @shantha05
    Author

    Yes, it was also tried and still it was leaking memory

  7. davidfowl commented on Sep 22, 2020

    @davidfowl
    Member

    Were you spinning up multiple instances of the host in a single process? The reason I'm asking these questions is because that PR fixes a very specific issue that would be hard to reproduce without doing something atypical.

  8. shantha05 commented on Sep 22, 2020

    @shantha05
    Author

    We had 2 different App Services in 2 resource groups hosted in 2 locations and routing traffic using Traffic Manager so it's 2 different processes

  9. davidfowl commented on Sep 22, 2020

    @davidfowl
    Member

    Then I don't see how that specific PR fixes the issue you're describing. Can you show the code snippet of the leak?

  10. shantha05 commented on Sep 22, 2020

    @shantha05
    Author

    This is the code that was commented to stop the memory leak.

    productsOptions.OnChange((config) => {
    _productSettings = config;
    });

    where productOptions is IOptionsMonitor productsOptions

  11. davidfowl commented on Sep 22, 2020

    @davidfowl
    Member

    How often did you call OnChange? Once per application? Or once per request? Where is the code that stores the result and disposed it

  12. shantha05 commented on Sep 22, 2020

    @shantha05
    Author

    The API is called 'n' number of times as part of the CertificateAuthenticationHandler once per request. Every CertificateAuthenticationHandler registers itself to the OptionsMonitor.OnChange event.

  13. davidfowl commented on Sep 22, 2020

    @davidfowl
    Member

    Can you show the registration and disposal code?

  14. shantha05 commented on Sep 22, 2020

    @shantha05
    Author
        private IDisposable _listener;
    
        public CertificateAuthenticationHandler(
            IProductRepository repository, 
            IOptionsMonitor<Products> productsOptions,
            IOptionsMonitor<CertAuthenticationOptions> options, 
            ILoggerFactory loggerFactory, 
            UrlEncoder encoder, 
            ISystemClock clock) 
            : base(options, loggerFactory, encoder, clock)
        {
            _repository = repository;
            _productSettings = productsOptions.CurrentValue;
            _listener = productsOptions.OnChange((config) => {
                _productSettings = config;
            });
        }
    
        ~CertificateAuthenticationHandler() {
            _listener.Dispose();
        }
    
  15. davidfowl commented on Sep 22, 2020

    @davidfowl
    Member

    Why is there a finalizer? Make the CertificateAuthenticationHandler implement IDisposable and call _listener.Dispose in the Dispose method. That should solve your problem.

  16. davidfowl commented on Sep 22, 2020

    @davidfowl
    Member

    Even better would be to use IOptionsSnapshot<T> to get the current snapshot of options (assuming this service is scoped or transient). That would resolve the lifetime mismatch going on here.

  17. shantha05 commented on Sep 22, 2020

    @shantha05
    Author

    Thanks David. Let me give that a try

  18. added
    waiting-author-feedback 📭The author of this issue needs to respond in order for us to continue investigating this issue.
    on Sep 23, 2020
  19. ghost closed this as completedon Sep 30, 2020
  20. ghost locked as resolved and limited conversation to collaborators on Oct 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions