volosoft / castle-windsor-ms-adapter

Castle Windsor ASP.NET Core / Microsoft.Extensions.DependencyInjection Adapter
https://www.nuget.org/packages/Castle.Windsor.MsDependencyInjection
MIT License
85 stars 29 forks source link

Fix Memory leaks with ScopedWindsorServiceProvider #27

Closed flexem closed 6 years ago

flexem commented 6 years ago

Fixes #26

hikalkan commented 6 years ago

Thanks a lot. I will review and merge it.

hikalkan commented 6 years ago

You are totally right. Sorry for the bug and thank you very much for investigating and fixing it.

I also released serviceprovider inside servicescope: https://github.com/volosoft/castle-windsor-ms-adapter/commit/80ead0b609b73c9e9f7e44ec41d1c369fc7a78f4

flexem commented 6 years ago

The Windsor don't track the instance which do not implement IDisposable interface.So may be do not need release it.Of cause,release it may be more better.

and You reminded me, I think release the GlobalScopedWindsorServiceProvider in it's Dispose method may be More necessary. https://github.com/volosoft/castle-windsor-ms-adapter/blob/80ead0b609b73c9e9f7e44ec41d1c369fc7a78f4/src/Castle.Windsor.MsDependencyInjection/GlobalScopedWindsorServiceProvider.cs#L14

hikalkan commented 6 years ago

Hi,

I just learned that "Windsor does not track the instance which do not implement IDisposable." I'm suprised, but it's reasonable.

However, we should not rely on that. If your non-disposable dependency depends on a disposable dependency, then castle tracks non-disposable too (tested)! So, we should always release an object if resolved directly from contaier, because we can not know if the class will have a disposable dependency in the future.

flexem commented 6 years ago

Yes, You are right! How about "release the GlobalScopedWindsorServiceProvider in it's Dispose method"?

hikalkan commented 6 years ago

OwnMsLifetimeScope.Dispose(); in GlobalScopedWindsorServiceProvider is correct.

flexem commented 6 years ago

I mean this:

using System;

namespace Castle.Windsor.MsDependencyInjection
{
    public class GlobalScopedWindsorServiceProvider : ScopedWindsorServiceProvider, IDisposable
    {
        private readonly IWindsorContainer _container;
        private bool _isDisposed;

        public GlobalScopedWindsorServiceProvider(IWindsorContainer container,
            MsLifetimeScopeProvider msLifetimeScopeProvider) : base(container, msLifetimeScopeProvider)
        {
            _container = container;
        }

        public void Dispose()
        {
            if (_isDisposed) return;
            OwnMsLifetimeScope.Dispose();
            _isDisposed = true;
            _container.Release(this);
        }
    }
}
hikalkan commented 6 years ago

Yes, it would be fine.