From 9b43a86b94c47c7f0c53c2ec104ff18aaa1e4828 Mon Sep 17 00:00:00 2001 From: Stephan Date: Wed, 2 Oct 2013 11:07:04 +0200 Subject: [PATCH] Core.Resolution - fix concurrency issues --- .../ManyObjectsResolverBase.cs | 75 +++++++++---------- .../ObjectResolution/Resolution.cs | 66 +++++++++------- .../SingleObjectResolverBase.cs | 5 +- .../Resolvers/ResolutionTests.cs | 6 +- 4 files changed, 81 insertions(+), 71 deletions(-) diff --git a/src/Umbraco.Core/ObjectResolution/ManyObjectsResolverBase.cs b/src/Umbraco.Core/ObjectResolution/ManyObjectsResolverBase.cs index c82a6598ff..d977754fc2 100644 --- a/src/Umbraco.Core/ObjectResolution/ManyObjectsResolverBase.cs +++ b/src/Umbraco.Core/ObjectResolution/ManyObjectsResolverBase.cs @@ -169,47 +169,46 @@ namespace Umbraco.Core.ObjectResolution { get { - // ensure we can - if (CanResolveBeforeFrozen == false) - Resolution.EnsureIsFrozen(); + using (Resolution.Reader(CanResolveBeforeFrozen)) + { + // note: we apply .ToArray() to the output of CreateInstance() because that is an IEnumerable that + // comes from the PluginManager we want to be _sure_ that it's not a Linq of some sort, but the + // instances have actually been instanciated when we return. - // note: we apply .ToArray() to the output of CreateInstance() because that is an IEnumerable that - // comes from the PluginManager we want to be _sure_ that it's not a Linq of some sort, but the - // instances have actually been instanciated when we return. + switch (LifetimeScope) + { + case ObjectLifetimeScope.HttpRequest: + // create new instances per HttpContext + using (var l = new UpgradeableReadLock(_lock)) + { + // create if not already there + if (CurrentHttpContext.Items[_httpContextKey] == null) + { + l.UpgradeToWriteLock(); + CurrentHttpContext.Items[_httpContextKey] = CreateInstances().ToArray(); + } + return (TResolved[])CurrentHttpContext.Items[_httpContextKey]; + } - switch (LifetimeScope) - { - case ObjectLifetimeScope.HttpRequest: - // create new instances per HttpContext - using (var l = new UpgradeableReadLock(_lock)) - { - // create if not already there - if (CurrentHttpContext.Items[_httpContextKey] == null) - { - l.UpgradeToWriteLock(); - CurrentHttpContext.Items[_httpContextKey] = CreateInstances().ToArray(); - } - return (TResolved[])CurrentHttpContext.Items[_httpContextKey]; - } + case ObjectLifetimeScope.Application: + // create new instances per application + using (var l = new UpgradeableReadLock(_lock)) + { + // create if not already there + if (_applicationInstances == null) + { + l.UpgradeToWriteLock(); + _applicationInstances = CreateInstances().ToArray(); + } + return _applicationInstances; + } - case ObjectLifetimeScope.Application: - // create new instances per application - using(var l = new UpgradeableReadLock(_lock)) - { - // create if not already there - if (_applicationInstances == null) - { - l.UpgradeToWriteLock(); - _applicationInstances = CreateInstances().ToArray(); - } - return _applicationInstances; - } - - case ObjectLifetimeScope.Transient: - default: - // create new instances each time - return CreateInstances().ToArray(); - } + case ObjectLifetimeScope.Transient: + default: + // create new instances each time + return CreateInstances().ToArray(); + } + } } } diff --git a/src/Umbraco.Core/ObjectResolution/Resolution.cs b/src/Umbraco.Core/ObjectResolution/Resolution.cs index 0dd30c9dc0..b6d44a2b15 100644 --- a/src/Umbraco.Core/ObjectResolution/Resolution.cs +++ b/src/Umbraco.Core/ObjectResolution/Resolution.cs @@ -13,8 +13,8 @@ namespace Umbraco.Core.ObjectResolution /// internal static class Resolution { - private static readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim(); - private volatile static bool _isFrozen; + private static readonly ReaderWriterLockSlim ConfigurationLock = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion); + private volatile static bool _isFrozen; /// /// Occurs when resolution is frozen. @@ -25,18 +25,26 @@ namespace Umbraco.Core.ObjectResolution /// /// Gets or sets a value indicating whether resolution of objects is frozen. /// - public static bool IsFrozen + // internal for unit tests, use ReadFrozen if you want to be sure + internal static bool IsFrozen { - get { return _isFrozen; } + get + { + using (new ReadLock(ConfigurationLock)) + { + return _isFrozen; + } + } } - public static void EnsureIsFrozen() - { - if (!_isFrozen) - { - throw new InvalidOperationException("Resolution is not frozen, it is not yet possible to get values from it."); - } - } + public static IDisposable Reader(bool canReadUnfrozen = false) + { + IDisposable l = new ReadLock(ConfigurationLock); + if (canReadUnfrozen || _isFrozen) return l; + + l.Dispose(); + throw new InvalidOperationException("Resolution is not frozen, it is not yet possible to get values from it."); + } /// /// Returns a disposable object that represents safe access to unfrozen resolution configuration. @@ -46,13 +54,11 @@ namespace Umbraco.Core.ObjectResolution { get { - IDisposable l = new WriteLock(_lock); - if (_isFrozen) - { - l.Dispose(); - throw new InvalidOperationException("Resolution is frozen, it is not possible to configure it anymore."); - } - return l; + IDisposable l = new WriteLock(ConfigurationLock); + if (_isFrozen == false) return l; + + l.Dispose(); + throw new InvalidOperationException("Resolution is frozen, it is not possible to configure it anymore."); } } @@ -72,16 +78,15 @@ namespace Umbraco.Core.ObjectResolution // keep the class here because it needs write-access to Resolution.IsFrozen private class DirtyBackdoor : IDisposable { - private static readonly ReaderWriterLockSlim DirtyLock = new ReaderWriterLockSlim(); - private IDisposable _lock; - private bool _frozen; + private readonly IDisposable _lock; + private readonly bool _frozen; public DirtyBackdoor() { LogHelper.Debug(typeof(DirtyBackdoor), "Creating back door for resolution"); - _lock = new WriteLock(DirtyLock); + _lock = new WriteLock(ConfigurationLock); _frozen = _isFrozen; _isFrozen = false; } @@ -103,11 +108,15 @@ namespace Umbraco.Core.ObjectResolution { LogHelper.Debug(typeof(Resolution), "Freezing resolution"); - if (_isFrozen) - throw new InvalidOperationException("Resolution is frozen. It is not possible to freeze it again."); + using (new WriteLock(ConfigurationLock)) + { + if (_isFrozen) + throw new InvalidOperationException("Resolution is frozen. It is not possible to freeze it again."); - _isFrozen = true; - if (Frozen != null) + _isFrozen = true; + } + + if (Frozen != null) Frozen(null, null); } @@ -119,7 +128,10 @@ namespace Umbraco.Core.ObjectResolution { LogHelper.Debug(typeof(DirtyBackdoor), "Resetting resolution"); - _isFrozen = false; + using (new WriteLock(ConfigurationLock)) + { + _isFrozen = false; + } Frozen = null; } } diff --git a/src/Umbraco.Core/ObjectResolution/SingleObjectResolverBase.cs b/src/Umbraco.Core/ObjectResolution/SingleObjectResolverBase.cs index b4a54f66b6..27a82b17ef 100644 --- a/src/Umbraco.Core/ObjectResolution/SingleObjectResolverBase.cs +++ b/src/Umbraco.Core/ObjectResolution/SingleObjectResolverBase.cs @@ -102,10 +102,7 @@ namespace Umbraco.Core.ObjectResolution { get { - // ensure we can - if (CanResolveBeforeFrozen == false) - Resolution.EnsureIsFrozen(); - + using (Resolution.Reader(CanResolveBeforeFrozen)) using (new ReadLock(_lock)) { if (!_canBeNull && _value == null) diff --git a/src/Umbraco.Tests/Resolvers/ResolutionTests.cs b/src/Umbraco.Tests/Resolvers/ResolutionTests.cs index 8e9da977a2..37cf8bcb04 100644 --- a/src/Umbraco.Tests/Resolvers/ResolutionTests.cs +++ b/src/Umbraco.Tests/Resolvers/ResolutionTests.cs @@ -87,14 +87,16 @@ namespace Umbraco.Tests.Resolvers [ExpectedException(typeof(InvalidOperationException))] public void ResolutionCanDetectIfNotFrozen() { - Resolution.EnsureIsFrozen(); // throws + using (Resolution.Reader()) // throws + {} } [Test] public void ResolutionCanEnsureIsFrozen() { Resolution.Freeze(); - Resolution.EnsureIsFrozen(); + using (Resolution.Reader()) // ok + {} } [Test]