diff --git a/src/Umbraco.Core/Scoping/IInstanceIdentifiable.cs b/src/Umbraco.Core/Scoping/IInstanceIdentifiable.cs new file mode 100644 index 0000000000..4c88e1c1b5 --- /dev/null +++ b/src/Umbraco.Core/Scoping/IInstanceIdentifiable.cs @@ -0,0 +1,9 @@ +using System; + +namespace Umbraco.Core.Scoping +{ + public interface IInstanceIdentifiable + { + Guid InstanceId { get; } + } +} diff --git a/src/Umbraco.Core/Scoping/IScope.cs b/src/Umbraco.Core/Scoping/IScope.cs index a0d994b2f2..0386401346 100644 --- a/src/Umbraco.Core/Scoping/IScope.cs +++ b/src/Umbraco.Core/Scoping/IScope.cs @@ -8,7 +8,7 @@ namespace Umbraco.Core.Scoping /// /// Represents a scope. /// - public interface IScope : IDisposable + public interface IScope : IDisposable, IInstanceIdentifiable { /// /// Gets the scope database. @@ -39,9 +39,5 @@ namespace Umbraco.Core.Scoping /// Completes the scope. /// void Complete(); - -#if DEBUG_SCOPES - Guid InstanceId { get; } -#endif } } diff --git a/src/Umbraco.Core/Scoping/NoScope.cs b/src/Umbraco.Core/Scoping/NoScope.cs index c2d89ff632..a6e265c1f2 100644 --- a/src/Umbraco.Core/Scoping/NoScope.cs +++ b/src/Umbraco.Core/Scoping/NoScope.cs @@ -24,10 +24,8 @@ namespace Umbraco.Core.Scoping #endif } -#if DEBUG_SCOPES private readonly Guid _instanceId = Guid.NewGuid(); public Guid InstanceId { get { return _instanceId; } } -#endif /// public bool CallContext { get { return false; } } diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index 4730495ba9..dff4a18017 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -133,10 +133,8 @@ 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 to force call-context public bool CallContext @@ -189,6 +187,8 @@ namespace Umbraco.Core.Scoping // the parent scope (in a nested scopes chain) public IScopeInternal ParentScope { get; set; } + public bool Attached { get; set; } + // the original scope (when attaching a detachable scope) public IScopeInternal OrigScope { get; set; } @@ -329,7 +329,18 @@ namespace Umbraco.Core.Scoping EnsureNotDisposed(); if (this != _scopeProvider.AmbientScope) + { +#if DEBUG_SCOPES + var ambient = _scopeProvider.AmbientScope; + Logging.LogHelper.Debug("Dispose error (" + (ambient == null ? "no" : "other") + " ambient)"); + if (ambient == null) + throw new InvalidOperationException("Not the ambient scope (no ambient scope)."); + var infos = _scopeProvider.GetScopeInfo(ambient); + throw new InvalidOperationException("Not the ambient scope (see current ambient ctor stack trace).\r\n" + infos.CtorStack); +#else throw new InvalidOperationException("Not the ambient scope."); +#endif + } #if DEBUG_SCOPES _scopeProvider.Disposed(this); @@ -432,6 +443,9 @@ namespace Umbraco.Core.Scoping { // get out of the way, restore original _scopeProvider.SetAmbient(OrigScope, OrigContext); + Attached = false; + OrigScope = null; + OrigContext = null; } }); } diff --git a/src/Umbraco.Core/Scoping/ScopeContext.cs b/src/Umbraco.Core/Scoping/ScopeContext.cs index 7503271b5a..cca0be560d 100644 --- a/src/Umbraco.Core/Scoping/ScopeContext.cs +++ b/src/Umbraco.Core/Scoping/ScopeContext.cs @@ -3,7 +3,7 @@ using System.Collections.Generic; namespace Umbraco.Core.Scoping { - public class ScopeContext + public class ScopeContext : IInstanceIdentifiable { private Dictionary _enlisted; @@ -27,6 +27,9 @@ namespace Umbraco.Core.Scoping throw new AggregateException("Exceptions were thrown by listed actions.", exceptions); } + private readonly Guid _instanceId = Guid.NewGuid(); + public Guid InstanceId { get { return _instanceId; } } + private IDictionary Enlisted { get diff --git a/src/Umbraco.Core/Scoping/ScopeProvider.cs b/src/Umbraco.Core/Scoping/ScopeProvider.cs index 7101258447..c500a633d0 100644 --- a/src/Umbraco.Core/Scoping/ScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/ScopeProvider.cs @@ -37,7 +37,7 @@ namespace Umbraco.Core.Scoping { // cannot re-attached over leaked scope/context // except of course over NoScope (which leaks) - var ambientScope = AmbientScopeInternal; + var ambientScope = GetCallContextObject(ScopeItemKey); if (ambientScope != null) { var ambientNoScope = ambientScope as NoScope; @@ -47,7 +47,8 @@ namespace Umbraco.Core.Scoping // this should rollback any pending transaction ambientNoScope.Dispose(); } - if (AmbientContextInternal != null) throw new Exception("Found leaked context when restoring call context."); + if (GetCallContextObject(ContextItemKey) != null) + throw new Exception("Found leaked context when restoring call context."); var t = (Tuple) o; SetCallContextObject(ScopeItemKey, t.Item1); @@ -101,27 +102,41 @@ namespace Umbraco.Core.Scoping lock (StaticCallContextObjectsLock) { object callContextObject; - return StaticCallContextObjects.TryGetValue(objectKey, out callContextObject) ? (T)callContextObject : null; + if (StaticCallContextObjects.TryGetValue(objectKey, out callContextObject)) + { +#if DEBUG_SCOPES + //Logging.LogHelper.Debug("GotObject " + objectKey.ToString("N").Substring(0, 8)); +#endif + return (T) callContextObject; + } +#if DEBUG_SCOPES + //Logging.LogHelper.Debug("MissedObject " + objectKey.ToString("N").Substring(0, 8)); +#endif + return null; } } - private static void SetCallContextObject(string key, object value) + private static void SetCallContextObject(string key, IInstanceIdentifiable value) { #if DEBUG_SCOPES // manage the 'context' that contains the scope (null, "http" or "call") - // first, null-register the existing value - var ambientKey = CallContext.LogicalGetData(ScopeItemKey).AsGuid(); - object o = null; - lock (StaticCallContextObjectsLock) + // only for scopes of course! + if (key == ScopeItemKey) { - if (ambientKey != default (Guid)) - StaticCallContextObjects.TryGetValue(ambientKey, out o); + // first, null-register the existing value + var ambientKey = CallContext.LogicalGetData(ScopeItemKey).AsGuid(); + object o = null; + lock (StaticCallContextObjectsLock) + { + if (ambientKey != default(Guid)) + StaticCallContextObjects.TryGetValue(ambientKey, out o); + } + var ambientScope = o as IScope; + if (ambientScope != null) RegisterContext(ambientScope, null); + // then register the new value + var scope = value as IScope; + if (scope != null) RegisterContext(scope, "call"); } - var ambientScope = o as IScope; - if (ambientScope != null) RegisterContext(ambientScope, null); - // then register the new value - var scope = value as IScope; - if (scope != null) RegisterContext(scope, "call"); #endif if (value == null) { @@ -130,6 +145,9 @@ namespace Umbraco.Core.Scoping if (objectKey == default (Guid)) return; lock (StaticCallContextObjectsLock) { +#if DEBUG_SCOPES + //Logging.LogHelper.Debug("RemoveObject " + objectKey.ToString("N").Substring(0, 8)); +#endif StaticCallContextObjects.Remove(objectKey); } } @@ -138,9 +156,12 @@ namespace Umbraco.Core.Scoping // note - we are *not* detecting an already-existing value // because our code in this class *always* sets to null before // setting to a real value - var objectKey = Guid.NewGuid(); + var objectKey = value.InstanceId; lock (StaticCallContextObjectsLock) { +#if DEBUG_SCOPES + //Logging.LogHelper.Debug("AddObject " + objectKey.ToString("N").Substring(0, 8)); +#endif StaticCallContextObjects.Add(objectKey, value); } CallContext.LogicalSetData(key, objectKey); @@ -181,12 +202,16 @@ namespace Umbraco.Core.Scoping } #if DEBUG_SCOPES // manage the 'context' that contains the scope (null, "http" or "call") - // first, null-register the existing value - var ambientScope = (IScope)httpContextItems[ScopeItemKey]; - if (ambientScope != null) RegisterContext(ambientScope, null); - // then register the new value - var scope = value as IScope; - if (scope != null) RegisterContext(scope, "http"); + // only for scopes of course! + if (key == ScopeItemKey) + { + // first, null-register the existing value + var ambientScope = (IScope)httpContextItems[ScopeItemKey]; + if (ambientScope != null) RegisterContext(ambientScope, null); + // then register the new value + var scope = value as IScope; + if (scope != null) RegisterContext(scope, "http"); + } #endif if (value == null) httpContextItems.Remove(key); @@ -195,7 +220,7 @@ namespace Umbraco.Core.Scoping return true; } - #endregion +#endregion #region Ambient Context @@ -292,9 +317,6 @@ namespace Umbraco.Core.Scoping return; } - if (context == null) - throw new ArgumentNullException("context"); - if (scope.CallContext == false && SetHttpContextObject(ScopeItemKey, scope, false)) { SetHttpContextObject(ScopeRefItemKey, StaticScopeReference); @@ -327,6 +349,10 @@ namespace Umbraco.Core.Scoping if (otherScope.Detachable == false) throw new ArgumentException("Not a detachable scope."); + if (otherScope.Attached) + throw new InvalidOperationException("Already attached."); + + otherScope.Attached = true; otherScope.OrigScope = AmbientScope; otherScope.OrigContext = AmbientContext; @@ -355,6 +381,7 @@ namespace Umbraco.Core.Scoping SetAmbient(scope.OrigScope, scope.OrigContext); scope.OrigScope = null; scope.OrigContext = null; + scope.Attached = false; return scope; } @@ -446,7 +473,7 @@ namespace Umbraco.Core.Scoping // all scope instances that are currently beeing tracked private static readonly object StaticScopeInfosLock = new object(); - private static readonly List StaticScopeInfos = new List(); + private static readonly Dictionary StaticScopeInfos = new Dictionary(); public IEnumerable ScopeInfos { @@ -454,7 +481,7 @@ namespace Umbraco.Core.Scoping { lock (StaticScopeInfosLock) { - return StaticScopeInfos.ToArray(); // capture in an array + return StaticScopeInfos.Values.ToArray(); // capture in an array } } } @@ -463,7 +490,8 @@ namespace Umbraco.Core.Scoping { lock (StaticScopeInfosLock) { - return StaticScopeInfos.FirstOrDefault(x => x.Scope == scope); + ScopeInfo scopeInfo; + return StaticScopeInfos.TryGetValue(scope, out scopeInfo) ? scopeInfo : null; } } @@ -477,8 +505,9 @@ namespace Umbraco.Core.Scoping { lock (StaticScopeInfosLock) { - if (StaticScopeInfos.Any(x => x.Scope == scope)) throw new Exception("oops: already registered."); - StaticScopeInfos.Add(new ScopeInfo(scope, Environment.StackTrace)); + if (StaticScopeInfos.ContainsKey(scope)) throw new Exception("oops: already registered."); + //Logging.LogHelper.Debug("Register " + scope.InstanceId.ToString("N").Substring(0, 8)); + StaticScopeInfos[scope] = new ScopeInfo(scope, Environment.StackTrace); } } @@ -488,13 +517,17 @@ namespace Umbraco.Core.Scoping { lock (StaticScopeInfosLock) { - var info = StaticScopeInfos.FirstOrDefault(x => x.Scope == scope); + ScopeInfo info; + if (StaticScopeInfos.TryGetValue(scope, out info) == false) info = null; if (info == null) { if (context == null) return; throw new Exception("oops: unregistered scope."); } + //Logging.LogHelper.Debug("Register context " + (context ?? "null") + " for " + scope.InstanceId.ToString("N").Substring(0, 8)); if (context == null) info.NullStack = Environment.StackTrace; + //if (context == null) + // Logging.LogHelper.Debug("STACK\r\n" + info.NullStack); info.Context = context; } } @@ -503,11 +536,12 @@ namespace Umbraco.Core.Scoping { lock (StaticScopeInfosLock) { - var info = StaticScopeInfos.FirstOrDefault(x => x.Scope == scope); - if (info != null) + if (StaticScopeInfos.ContainsKey(scope)) { // enable this by default - StaticScopeInfos.Remove(info); + //Console.WriteLine("unregister " + scope.InstanceId.ToString("N").Substring(0, 8)); + StaticScopeInfos.Remove(scope); + //Logging.LogHelper.Debug("Remove " + scope.InstanceId.ToString("N").Substring(0, 8)); // instead, enable this to keep *all* scopes // beware, there can be a lot of scopes! diff --git a/src/Umbraco.Core/Umbraco.Core.csproj b/src/Umbraco.Core/Umbraco.Core.csproj index 9fcff37763..738634c858 100644 --- a/src/Umbraco.Core/Umbraco.Core.csproj +++ b/src/Umbraco.Core/Umbraco.Core.csproj @@ -559,6 +559,7 @@ + diff --git a/src/Umbraco.Tests/Scoping/ScopeTests.cs b/src/Umbraco.Tests/Scoping/ScopeTests.cs index 87884abd26..747e5b1f40 100644 --- a/src/Umbraco.Tests/Scoping/ScopeTests.cs +++ b/src/Umbraco.Tests/Scoping/ScopeTests.cs @@ -1,15 +1,12 @@ using System; using System.Collections; -using System.Collections.Generic; using System.Runtime.Remoting.Messaging; -using System.Web; -using Moq; +using System.Threading; using NUnit.Framework; using Umbraco.Core; using Umbraco.Core.Persistence; using Umbraco.Core.Scoping; using Umbraco.Tests.TestHelpers; -using Umbraco.Core.Events; namespace Umbraco.Tests.Scoping { @@ -106,8 +103,8 @@ namespace Umbraco.Tests.Scoping public void NestedMigrateScope() { var scopeProvider = DatabaseContext.ScopeProvider; - Assert.IsNull(scopeProvider.AmbientScope); + var httpContextItems = new Hashtable(); ScopeProvider.HttpContextItemsGetter = () => httpContextItems; try @@ -531,17 +528,82 @@ namespace Umbraco.Tests.Scoping } [Test] - public void CallContextScope() + public void CallContextScope1() { var scopeProvider = DatabaseContext.ScopeProvider; - var scope = scopeProvider.CreateScope(); - Assert.IsNotNull(scopeProvider.AmbientScope); - using (new SafeCallContext()) + using (var scope = scopeProvider.CreateScope()) { - Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.IsNotNull(scopeProvider.AmbientContext); + using (new SafeCallContext()) + { + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + + using (var newScope = scopeProvider.CreateScope()) + { + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientScope.ParentScope); + Assert.IsNotNull(scopeProvider.AmbientContext); + } + + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + } + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.AreSame(scope, scopeProvider.AmbientScope); + } + + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + } + + [Test] + public void CallContextScope2() + { + var scopeProvider = DatabaseContext.ScopeProvider; + Assert.IsNull(scopeProvider.AmbientScope); + + var httpContextItems = new Hashtable(); + ScopeProvider.HttpContextItemsGetter = () => httpContextItems; + try + { + using (var scope = scopeProvider.CreateScope()) + { + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.IsNotNull(scopeProvider.AmbientContext); + using (new SafeCallContext()) + { + // pretend it's another thread + ScopeProvider.HttpContextItemsGetter = null; + + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + + using (var newScope = scopeProvider.CreateScope()) + { + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientScope.ParentScope); + Assert.IsNotNull(scopeProvider.AmbientContext); + } + + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + + // back to original thread + ScopeProvider.HttpContextItemsGetter = () => httpContextItems; + } + Assert.IsNotNull(scopeProvider.AmbientScope); + Assert.AreSame(scope, scopeProvider.AmbientScope); + } + + Assert.IsNull(scopeProvider.AmbientScope); + Assert.IsNull(scopeProvider.AmbientContext); + } + finally + { + ScopeProvider.HttpContextItemsGetter = null; } - Assert.IsNotNull(scopeProvider.AmbientScope); - Assert.AreSame(scope, scopeProvider.AmbientScope); } [Test]