From 1f636d05d11259aca8e98cee745c0ee3a33bbab8 Mon Sep 17 00:00:00 2001 From: Stephan Date: Wed, 18 Jan 2017 10:36:11 +0100 Subject: [PATCH] U4-9322 - fix scope leaks --- .../Persistence/LockingRepository.cs | 22 +++- src/Umbraco.Core/Scoping/IScope.cs | 4 + src/Umbraco.Core/Scoping/IScopeProvider.cs | 8 +- src/Umbraco.Core/Scoping/NoScope.cs | 12 ++ src/Umbraco.Core/Scoping/Scope.cs | 12 ++ src/Umbraco.Core/Scoping/ScopeProvider.cs | 106 +++++++++++++++--- src/Umbraco.Core/UmbracoApplicationBase.cs | 9 +- 7 files changed, 146 insertions(+), 27 deletions(-) diff --git a/src/Umbraco.Core/Persistence/LockingRepository.cs b/src/Umbraco.Core/Persistence/LockingRepository.cs index f513073e71..88df2c492e 100644 --- a/src/Umbraco.Core/Persistence/LockingRepository.cs +++ b/src/Umbraco.Core/Persistence/LockingRepository.cs @@ -46,7 +46,7 @@ namespace Umbraco.Core.Persistence public TResult WithReadLocked(Func, TResult> func, bool autoCommit = true) { - var uow = _uowProvider.GetUnitOfWork(); + using (var uow = _uowProvider.GetUnitOfWork()) using (var transaction = uow.Database.GetTransaction(IsolationLevel.RepeatableRead)) { foreach (var lockId in _readLockIds) @@ -65,7 +65,9 @@ namespace Umbraco.Core.Persistence public void WithWriteLocked(Action> action, bool autoCommit = true) { - var uow = _uowProvider.GetUnitOfWork(); + // must use the uow to ensure it's disposed if GetTransaction fails! + + using (var uow = _uowProvider.GetUnitOfWork()) using (var transaction = uow.Database.GetTransaction(IsolationLevel.RepeatableRead)) { foreach (var lockId in _writeLockIds) @@ -79,11 +81,25 @@ namespace Umbraco.Core.Persistence transaction.Complete(); } } + // fixme + // the change above ensures that scopes are properly disposed + // TODO apply to all methods + // now how can we manage the isolation level? + + //uow.ScopeIsolationLevel = IsolationLevel.RepeatableRead; // might throw when creating the scope + // getTransaction here throws because of different levels + // but the exception gets eaten because on the way out, disposing the _outer_ scope fails + // so... how shall we figure it out?! + // - we should be able to set the isolation level on the uow scope + // - should we be able to dispose parent scopes and then what happens? NO! + // + // if we can create scopes with isolation levels what happens when we nest scopes? + // note: this is a *very* special use case } public TResult WithWriteLocked(Func, TResult> func, bool autoCommit = true) { - var uow = _uowProvider.GetUnitOfWork(); + using (var uow = _uowProvider.GetUnitOfWork()) using (var transaction = uow.Database.GetTransaction(IsolationLevel.RepeatableRead)) { foreach (var lockId in _writeLockIds) diff --git a/src/Umbraco.Core/Scoping/IScope.cs b/src/Umbraco.Core/Scoping/IScope.cs index bc23ad3868..4960350575 100644 --- a/src/Umbraco.Core/Scoping/IScope.cs +++ b/src/Umbraco.Core/Scoping/IScope.cs @@ -24,5 +24,9 @@ namespace Umbraco.Core.Scoping /// Completes the scope. /// void Complete(); + +#if DEBUG_SCOPES + Guid InstanceId { get; } +#endif } } diff --git a/src/Umbraco.Core/Scoping/IScopeProvider.cs b/src/Umbraco.Core/Scoping/IScopeProvider.cs index e6237b3af9..6622b1a417 100644 --- a/src/Umbraco.Core/Scoping/IScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/IScopeProvider.cs @@ -1,4 +1,6 @@ -namespace Umbraco.Core.Scoping +using System.Collections.Generic; + +namespace Umbraco.Core.Scoping { /// /// Provides scopes. @@ -43,5 +45,9 @@ /// Only a scope previously attached by can be detached. /// IScope DetachScope(); + +#if DEBUG_SCOPES + IEnumerable ScopeInfos { get; } +#endif } } diff --git a/src/Umbraco.Core/Scoping/NoScope.cs b/src/Umbraco.Core/Scoping/NoScope.cs index 9a2ebec5fe..5c53e89047 100644 --- a/src/Umbraco.Core/Scoping/NoScope.cs +++ b/src/Umbraco.Core/Scoping/NoScope.cs @@ -19,8 +19,16 @@ namespace Umbraco.Core.Scoping public NoScope(ScopeProvider scopeProvider) { _scopeProvider = scopeProvider; +#if DEBUG_SCOPES + _scopeProvider.Register(this); +#endif } +#if DEBUG_SCOPES + private readonly Guid _instanceId = Guid.NewGuid(); + public Guid InstanceId { get { return _instanceId; } } +#endif + /// public UmbracoDatabase Database { @@ -78,6 +86,10 @@ namespace Umbraco.Core.Scoping if (this != _scopeProvider.AmbientScope) throw new InvalidOperationException("Not the ambient scope."); +#if DEBUG_SCOPES + _scopeProvider.Disposed(this); +#endif + if (_database != null) _database.Dispose(); diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 36df7e546b..5b220fe433 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -23,6 +23,9 @@ namespace Umbraco.Core.Scoping { _scopeProvider = scopeProvider; Detachable = detachable; +#if DEBUG_SCOPES + _scopeProvider.Register(this); +#endif } // initializes a new scope in a nested scopes chain, with its parent @@ -45,6 +48,11 @@ namespace Umbraco.Core.Scoping throw new Exception("NoScope instance is not free."); } +#if DEBUG_SCOPES + private readonly Guid _instanceId = Guid.NewGuid(); + public Guid InstanceId { get { return _instanceId; } } +#endif + // a value indicating whether the scope is detachable // ie whether it was created by CreateDetachedScope public bool Detachable { get; private set; } @@ -139,6 +147,10 @@ namespace Umbraco.Core.Scoping if (this != _scopeProvider.AmbientScope) throw new InvalidOperationException("Not the ambient scope."); +#if DEBUG_SCOPES + _scopeProvider.Disposed(this); +#endif + var parent = ParentScope; _scopeProvider.AmbientScope = parent; diff --git a/src/Umbraco.Core/Scoping/ScopeProvider.cs b/src/Umbraco.Core/Scoping/ScopeProvider.cs index 9a1b5d18cd..7f43b7208e 100644 --- a/src/Umbraco.Core/Scoping/ScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/ScopeProvider.cs @@ -1,4 +1,6 @@ using System; +using System.Collections.Generic; +using System.Linq; using System.Runtime.Remoting.Messaging; using System.Web; using Umbraco.Core.Persistence; @@ -46,6 +48,12 @@ namespace Umbraco.Core.Scoping get { return (IScope) CallContext.LogicalGetData(ItemKey); } set { +#if DEBUG_SCOPES + var ambient = (IScope)CallContext.LogicalGetData(ItemKey); + if (ambient != null) RegisterContext(ambient, null); + if (value != null) + RegisterContext(value, "lcc"); +#endif if (value == null) CallContext.FreeNamedDataSlot(ItemKey); else CallContext.LogicalSetData(ItemKey, value); } @@ -56,6 +64,12 @@ namespace Umbraco.Core.Scoping get { return (IScope) HttpContext.Current.Items[ItemKey]; } set { +#if DEBUG_SCOPES + var ambient = (IScope) HttpContext.Current.Items[ItemKey]; + if (ambient != null) RegisterContext(ambient, null); + if (value != null) + RegisterContext(value, "http"); +#endif if (value == null) { HttpContext.Current.Items.Remove(ItemKey); @@ -152,6 +166,9 @@ namespace Umbraco.Core.Scoping var noScope = ambient as NoScope; if (noScope != null) { +#if DEBUG_SCOPES + Disposed(noScope); +#endif // peta poco nulls the shared connection after each command unless there's a trx var database = noScope.DatabaseOrNull; if (database != null && database.InTransaction) @@ -182,29 +199,82 @@ namespace Umbraco.Core.Scoping // so we can track leaks // helps identifying when non-httpContext scopes are created by logging the stack trace - private void LogCallContextStack() - { - var trace = Environment.StackTrace; - if (trace.IndexOf("ScheduledPublishing") > 0) - LogHelper.Debug("CallContext: Scheduled Publishing"); - else if (trace.IndexOf("TouchServerTask") > 0) - LogHelper.Debug("CallContext: Server Registration"); - else if (trace.IndexOf("LogScrubber") > 0) - LogHelper.Debug("CallContext: Log Scrubber"); - else - LogHelper.Debug("CallContext: " + Environment.StackTrace); - } - - private readonly List _scopes = new List(); + //private void LogCallContextStack() + //{ + // var trace = Environment.StackTrace; + // if (trace.IndexOf("ScheduledPublishing") > 0) + // LogHelper.Debug("CallContext: Scheduled Publishing"); + // else if (trace.IndexOf("TouchServerTask") > 0) + // LogHelper.Debug("CallContext: Server Registration"); + // else if (trace.IndexOf("LogScrubber") > 0) + // LogHelper.Debug("CallContext: Log Scrubber"); + // else + // LogHelper.Debug("CallContext: " + Environment.StackTrace); + //} // helps identifying scope leaks by keeping track of all instances - public List Scopes { get { return _scopes; } } + public static readonly List StaticScopeInfos = new List(); - private static void Log(string message, UmbracoDatabase database) + public IEnumerable ScopeInfos { get { return StaticScopeInfos; } } + + //private static void Log(string message, UmbracoDatabase database) + //{ + // LogHelper.Debug(message + " (" + (database == null ? "" : database.InstanceSid) + ")."); + //} + + public void Register(IScope scope) { - LogHelper.Debug(message + " (" + (database == null ? "" : database.InstanceSid) + ")."); + if (StaticScopeInfos.Any(x => x.Scope == scope)) throw new Exception(); + StaticScopeInfos.Add(new ScopeInfo(scope, Environment.StackTrace)); + } + + public static void RegisterContext(IScope scope, string context) + { + var info = StaticScopeInfos.FirstOrDefault(x => x.Scope == scope); + if (info == null) + { + if (context == null) return; + throw new Exception(); + } + if (context == null) info.NullStack = Environment.StackTrace; + info.Context = context; + } + + public void Disposed(IScope scope) + { + var info = StaticScopeInfos.FirstOrDefault(x => x.Scope == scope); + if (info != null) + { + // enable this by default + StaticScopeInfos.Remove(info); + + // instead, enable this to keep *all* scopes + // beware, there can be a lot of scopes! + //info.Disposed = true; + //info.DisposedStack = Environment.StackTrace; + } } #endif - } + +#if DEBUG_SCOPES + public class ScopeInfo + { + public ScopeInfo(IScope scope, string ctorStack) + { + Scope = scope; + Created = DateTime.Now; + CtorStack = ctorStack; + } + + public IScope Scope { get; private set; } + public Guid Parent { get { return (Scope is NoScope || ((Scope) Scope).ParentScope == null) ? Guid.Empty : ((Scope) Scope).ParentScope.InstanceId; } } + public DateTime Created { get; private set; } + public bool Disposed { get; set; } + public string Context { get; set; } + public string CtorStack { get; private set; } // the stacktrace of the scope ctor + public string DisposedStack { get; set; } // the stacktrace when disposed + public string NullStack { get; set; } // the stacktrace when context went null + } +#endif } diff --git a/src/Umbraco.Core/UmbracoApplicationBase.cs b/src/Umbraco.Core/UmbracoApplicationBase.cs index 1bf90fb154..02f50710a9 100644 --- a/src/Umbraco.Core/UmbracoApplicationBase.cs +++ b/src/Umbraco.Core/UmbracoApplicationBase.cs @@ -8,6 +8,7 @@ using System.Web.Hosting; using log4net; using Umbraco.Core.Logging; using Umbraco.Core.ObjectResolution; +using Umbraco.Core.Scoping; namespace Umbraco.Core { @@ -60,12 +61,10 @@ namespace Umbraco.Core //And now we can dispose of our startup handlers - save some memory ApplicationEventsResolver.Current.Dispose(); - // after Umbraco has started there is a database in "context" and that context is + // after Umbraco has started there is a scope in "context" and that context is // going to stay there and never get destroyed nor reused, so we have to ensure that - // the database is disposed (which will auto-remove it from context). - var database = ApplicationContext.Current.DatabaseContext.Database; - if (database != null) // never to happen... unless in weird tests - ApplicationContext.Current.DatabaseContext.Database.Dispose(); + // the scope is disposed (along with database etc) - reset it all entirely + ((ScopeProvider) ApplicationContext.Current.ScopeProvider).Reset(); } ///