diff --git a/src/Umbraco.Core/DisposableObject.cs b/src/Umbraco.Core/DisposableObject.cs index 516a9712e5..82c045e8f5 100644 --- a/src/Umbraco.Core/DisposableObject.cs +++ b/src/Umbraco.Core/DisposableObject.cs @@ -1,5 +1,4 @@ using System; -using System.Threading; namespace Umbraco.Core { @@ -8,24 +7,22 @@ namespace Umbraco.Core /// /// /// Can also be used as a pattern for when inheriting is not possible. - /// + /// /// See also: https://msdn.microsoft.com/en-us/library/b1yfkh5e%28v=vs.110%29.aspx /// See also: https://lostechies.com/chrispatterson/2012/11/29/idisposable-done-right/ - /// + /// /// Note: if an object's ctor throws, it will never be disposed, and so if that ctor /// has allocated disposable objects, it should take care of disposing them. /// public abstract class DisposableObject : IDisposable { - private bool _disposed; private readonly object _locko = new object(); // gets a value indicating whether this instance is disposed. // for internal tests only (not thread safe) - //TODO make this internal + rename "Disposed" when we can break compatibility - public bool IsDisposed { get { return _disposed; } } + public bool Disposed { get; private set; } - // implements IDisposable + // implements IDisposable public void Dispose() { Dispose(true); @@ -38,13 +35,12 @@ namespace Umbraco.Core Dispose(false); } - //TODO make this private, non-virtual when we can break compatibility - protected virtual void Dispose(bool disposing) + private void Dispose(bool disposing) { lock (_locko) { - if (_disposed) return; - _disposed = true; + if (Disposed) return; + Disposed = true; } DisposeUnmanagedResources(); diff --git a/src/Umbraco.Tests/BootManagers/CoreBootManagerTests.cs b/src/Umbraco.Tests/BootManagers/CoreBootManagerTests.cs index 84cca241b7..ca4b049745 100644 --- a/src/Umbraco.Tests/BootManagers/CoreBootManagerTests.cs +++ b/src/Umbraco.Tests/BootManagers/CoreBootManagerTests.cs @@ -33,7 +33,7 @@ namespace Umbraco.Tests.BootManagers TestApplicationEventHandler.Reset(); } - + /// /// test application using a CoreBootManager instance to boot /// @@ -75,7 +75,7 @@ namespace Umbraco.Tests.BootManagers container.RegisterSingleton(); } } - + /// /// test event handler /// @@ -86,13 +86,13 @@ namespace Umbraco.Tests.BootManagers Initialized = false; Starting = false; Started = false; - Disposed = false; + HasBeenDisposed = false; } - public static bool Initialized = false; - public static bool Starting = false; - public static bool Started = false; - public static bool Disposed = false; + public static bool Initialized; + public static bool Starting; + public static bool Started; + public static bool HasBeenDisposed; public void OnApplicationInitialized(UmbracoApplicationBase umbracoApplication, ApplicationContext applicationContext) { @@ -111,7 +111,7 @@ namespace Umbraco.Tests.BootManagers protected override void DisposeResources() { - Disposed = true; + HasBeenDisposed = true; } } @@ -121,8 +121,8 @@ namespace Umbraco.Tests.BootManagers using (var app = new TestApp()) { app.StartApplication(app, new EventArgs()); - - Assert.IsTrue(TestApplicationEventHandler.Disposed); + + Assert.IsTrue(TestApplicationEventHandler.HasBeenDisposed); } } diff --git a/src/Umbraco.Tests/Cache/PublishedCache/PublishedContentCacheTests.cs b/src/Umbraco.Tests/Cache/PublishedCache/PublishedContentCacheTests.cs index 704278f059..b2c754b077 100644 --- a/src/Umbraco.Tests/Cache/PublishedCache/PublishedContentCacheTests.cs +++ b/src/Umbraco.Tests/Cache/PublishedCache/PublishedContentCacheTests.cs @@ -75,8 +75,7 @@ namespace Umbraco.Tests.Cache.PublishedCache facadeService.Object, new WebSecurity(_httpContextFactory.HttpContext, ApplicationContext), settings, - Enumerable.Empty(), - null); + Enumerable.Empty()); _cache = _umbracoContext.ContentCache; } diff --git a/src/Umbraco.Tests/PublishedContent/PublishedContentMoreTests.cs b/src/Umbraco.Tests/PublishedContent/PublishedContentMoreTests.cs index 80999a2725..22abc00084 100644 --- a/src/Umbraco.Tests/PublishedContent/PublishedContentMoreTests.cs +++ b/src/Umbraco.Tests/PublishedContent/PublishedContentMoreTests.cs @@ -72,8 +72,7 @@ namespace Umbraco.Tests.PublishedContent facadeService.Object, new WebSecurity(httpContext, ApplicationContext), Mock.Of(), - Enumerable.Empty(), - null); + Enumerable.Empty()); Umbraco.Web.Current.SetUmbracoContext(ctx, true); } diff --git a/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs b/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs index be681c9e72..812f5ed5e4 100644 --- a/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs +++ b/src/Umbraco.Tests/Scheduling/BackgroundTaskRunnerTests.cs @@ -547,7 +547,7 @@ namespace Umbraco.Tests.Scheduling runner.Shutdown(false, true); // check that task has been disposed (timer has been killed, etc) - Assert.IsTrue(task.IsDisposed); + Assert.IsTrue(task.Disposed); } } diff --git a/src/Umbraco.Tests/Security/BackOfficeCookieManagerTests.cs b/src/Umbraco.Tests/Security/BackOfficeCookieManagerTests.cs index f6265e7432..5352002b73 100644 --- a/src/Umbraco.Tests/Security/BackOfficeCookieManagerTests.cs +++ b/src/Umbraco.Tests/Security/BackOfficeCookieManagerTests.cs @@ -43,7 +43,7 @@ namespace Umbraco.Tests.Security Mock.Of(), appCtx, Mock.Of(), new WebSecurity(Mock.Of(), appCtx), - Mock.Of(), new List(), false); + Mock.Of(), new List()); var mgr = new BackOfficeCookieManager(Mock.Of(accessor => accessor.UmbracoContext == umbCtx)); @@ -68,7 +68,7 @@ namespace Umbraco.Tests.Security Mock.Of(), appCtx, Mock.Of(), new WebSecurity(Mock.Of(), appCtx), - Mock.Of(), new List(), false); + Mock.Of(), new List()); var mgr = new BackOfficeCookieManager(Mock.Of(accessor => accessor.UmbracoContext == umbCtx)); diff --git a/src/Umbraco.Tests/Web/WebExtensionMethodTests.cs b/src/Umbraco.Tests/Web/WebExtensionMethodTests.cs index e57269240d..5d6dfdf2e3 100644 --- a/src/Umbraco.Tests/Web/WebExtensionMethodTests.cs +++ b/src/Umbraco.Tests/Web/WebExtensionMethodTests.cs @@ -42,8 +42,7 @@ namespace Umbraco.Tests.Web Mock.Of(), new WebSecurity(Mock.Of(), appCtx), Mock.Of(), - new List(), - false); + new List()); var r1 = new RouteData(); r1.DataTokens.Add(Core.Constants.Web.UmbracoContextDataToken, umbCtx); @@ -62,8 +61,7 @@ namespace Umbraco.Tests.Web Mock.Of(), new WebSecurity(Mock.Of(), appCtx), Mock.Of(), - new List(), - false); + new List()); var r1 = new RouteData(); r1.DataTokens.Add(Core.Constants.Web.UmbracoContextDataToken, umbCtx); @@ -92,8 +90,7 @@ namespace Umbraco.Tests.Web Mock.Of(), new WebSecurity(Mock.Of(), appCtx), Mock.Of(), - new List(), - false); + new List()); var httpContext = Mock.Of(); diff --git a/src/Umbraco.Web/Current.cs b/src/Umbraco.Web/Current.cs index 8a0dc7b12f..8e7144e634 100644 --- a/src/Umbraco.Web/Current.cs +++ b/src/Umbraco.Web/Current.cs @@ -55,6 +55,7 @@ namespace Umbraco.Web { if (UmbracoContextAccessor.UmbracoContext != null && canReplace == false) throw new InvalidOperationException("Current UmbracoContext can be set only once per request."); + UmbracoContextAccessor.UmbracoContext?.Dispose(); // dispose the one that is being replaced, if any UmbracoContextAccessor.UmbracoContext = value; } } @@ -62,7 +63,11 @@ namespace Umbraco.Web // this is because of the weird mixed accessor we're using that can store things in thread-static var public static void ClearUmbracoContext() { - UmbracoContextAccessor.UmbracoContext = null; + lock (Locker) + { + UmbracoContextAccessor.UmbracoContext?.Dispose(); // dispose the one that is being cleared, if any + UmbracoContextAccessor.UmbracoContext = null; + } } // cannot set - it's set by whatever creates the facade, which should have the accessor injected diff --git a/src/Umbraco.Web/Scheduling/RecurringTaskBase.cs b/src/Umbraco.Web/Scheduling/RecurringTaskBase.cs index 567f85f1f5..82c83d81b4 100644 --- a/src/Umbraco.Web/Scheduling/RecurringTaskBase.cs +++ b/src/Umbraco.Web/Scheduling/RecurringTaskBase.cs @@ -67,7 +67,7 @@ namespace Umbraco.Web.Scheduling if (_runner.TryAdd(this)) _timer.Change(_periodMilliseconds, 0); else - Dispose(true); + Dispose(); } /// diff --git a/src/Umbraco.Web/UmbracoContext.cs b/src/Umbraco.Web/UmbracoContext.cs index 9ea394dd4a..124410ad26 100644 --- a/src/Umbraco.Web/UmbracoContext.cs +++ b/src/Umbraco.Web/UmbracoContext.cs @@ -67,7 +67,7 @@ namespace Umbraco.Web // create, assign the singleton, and return umbracoContext = CreateContext(httpContext, applicationContext, facadeService, webSecurity, umbracoSettings, urlProviders); - Web.Current.SetUmbracoContext(umbracoContext, replaceContext); + Web.Current.SetUmbracoContext(umbracoContext, replaceContext); // will dispose the one that is being replaced return umbracoContext; } @@ -135,9 +135,14 @@ namespace Umbraco.Web IFacadeService facadeService, WebSecurity webSecurity) { - // ensure that this instance is disposed when the request terminates, - // though we *also* ensure this happens in the Umbraco module since the - // UmbracoCOntext is added to the HttpContext items. + // ensure that this instance is disposed when the request terminates, though we *also* ensure + // this happens in the Umbraco module since the UmbracoCOntext is added to the HttpContext items. + // + // also, it *can* be returned by the container with a PerRequest lifetime, meaning that the + // container *could* also try to dispose it. + // + // all in all, this context may be disposed more than once, but DisposableObject ensures that + // it is ok and it will be actually disposed only once. httpContext.DisposeOnPipelineCompleted(this); if (httpContext == null) throw new ArgumentNullException(nameof(httpContext)); @@ -357,12 +362,13 @@ namespace Umbraco.Web } } - protected override void DisposeResources() { + // DisposableObject ensures that this runs only once + Security.DisposeIfDisposable(); - //If not running in a web ctx, ensure the thread based instance is nulled + // reset - important when running outside of http context Web.Current.SetUmbracoContext(null, true); // help caches release resources