From 5e0fbe5634ececedd7f9cddf4371028d34ad9db1 Mon Sep 17 00:00:00 2001 From: Stephan Date: Tue, 1 Oct 2013 21:58:57 +0200 Subject: [PATCH 1/5] Macros - bugfix parameters --- src/Umbraco.Web/umbraco.presentation/macro.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/Umbraco.Web/umbraco.presentation/macro.cs b/src/Umbraco.Web/umbraco.presentation/macro.cs index a457ceac72..7011426d9a 100644 --- a/src/Umbraco.Web/umbraco.presentation/macro.cs +++ b/src/Umbraco.Web/umbraco.presentation/macro.cs @@ -1312,8 +1312,11 @@ namespace umbraco break; case "mediaCurrent": - var c = new Content(int.Parse(macroPropertyValue)); - macroXmlNode.AppendChild(macroXml.ImportNode(c.ToXml(umbraco.content.Instance.XmlContent, false), true)); + if (string.IsNullOrEmpty(macroPropertyValue) == false) + { + var c = new Content(int.Parse(macroPropertyValue)); + macroXmlNode.AppendChild(macroXml.ImportNode(c.ToXml(umbraco.content.Instance.XmlContent, false), true)); + } break; default: From cdd1a0a4cb77969abab94140282c236811922c52 Mon Sep 17 00:00:00 2001 From: Stephan Date: Wed, 2 Oct 2013 11:07:04 +0200 Subject: [PATCH 2/5] Core.Resolution - fix concurrency issues Conflicts: src/Umbraco.Core/ObjectResolution/Resolution.cs --- .../ManyObjectsResolverBase.cs | 75 ++++++++-------- .../ObjectResolution/Resolution.cs | 88 +++++++++++++------ .../SingleObjectResolverBase.cs | 5 +- .../Resolvers/ResolutionTests.cs | 6 +- 4 files changed, 104 insertions(+), 70 deletions(-) diff --git a/src/Umbraco.Core/ObjectResolution/ManyObjectsResolverBase.cs b/src/Umbraco.Core/ObjectResolution/ManyObjectsResolverBase.cs index 4f3bc2665b..5a1514d130 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 d9e5977e4a..17097ee7a5 100644 --- a/src/Umbraco.Core/ObjectResolution/Resolution.cs +++ b/src/Umbraco.Core/ObjectResolution/Resolution.cs @@ -1,5 +1,7 @@ using System; +using System.Linq; using System.Threading; +using Umbraco.Core.Logging; namespace Umbraco.Core.ObjectResolution { @@ -12,9 +14,10 @@ namespace Umbraco.Core.ObjectResolution /// internal static class Resolution { - private static readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim(); + private static readonly ReaderWriterLockSlim ConfigurationLock = new ReaderWriterLockSlim(LockRecursionPolicy.SupportsRecursion); + private volatile static bool _isFrozen; - /// + /// /// Occurs when resolution is frozen. /// /// Occurs only once, since resolution can be frozen only once. @@ -23,14 +26,27 @@ namespace Umbraco.Core.ObjectResolution /// /// Gets or sets a value indicating whether resolution of objects is frozen. /// - public static bool IsFrozen { get; private set; } - - public static void EnsureIsFrozen() + // internal for unit tests, use ReadFrozen if you want to be sure + internal static bool IsFrozen { - if (!IsFrozen) - throw new InvalidOperationException("Resolution is not frozen, it is not yet possible to get values from it."); + get + { + using (new ReadLock(ConfigurationLock)) + { + return _isFrozen; + } + } } + 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. /// @@ -39,13 +55,11 @@ namespace Umbraco.Core.ObjectResolution { get { - IDisposable l = new WriteLock(_lock); - if (Resolution.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."); } } @@ -65,21 +79,24 @@ namespace Umbraco.Core.ObjectResolution // keep the class here because it needs write-access to Resolution.IsFrozen private class DirtyBackdoor : IDisposable { - private static readonly System.Threading.ReaderWriterLockSlim _dirtyLock = new ReaderWriterLockSlim(); - private IDisposable _lock; - private bool _frozen; + private readonly IDisposable _lock; + private readonly bool _frozen; public DirtyBackdoor() { - _lock = new WriteLock(_dirtyLock); - _frozen = Resolution.IsFrozen; - Resolution.IsFrozen = false; + LogHelper.Debug(typeof(DirtyBackdoor), "Creating back door for resolution"); + + _lock = new WriteLock(ConfigurationLock); + _frozen = _isFrozen; + _isFrozen = false; } public void Dispose() { - Resolution.IsFrozen = _frozen; + LogHelper.Debug(typeof(DirtyBackdoor), "Disposing back door for resolution"); + + _isFrozen = _frozen; _lock.Dispose(); } } @@ -90,11 +107,17 @@ namespace Umbraco.Core.ObjectResolution /// resolution is already frozen. public static void Freeze() { - if (Resolution.IsFrozen) - throw new InvalidOperationException("Resolution is frozen. It is not possible to freeze it again."); + LogHelper.Debug(typeof(Resolution), "Freezing resolution"); - IsFrozen = true; - if (Frozen != null) + 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) Frozen(null, null); } @@ -104,7 +127,20 @@ namespace Umbraco.Core.ObjectResolution /// To be used in unit tests. internal static void Reset() { - IsFrozen = false; + LogHelper.Debug(typeof(Resolution), "Resetting resolution"); + + /* + var trace = new System.Diagnostics.StackTrace(); + var testing = trace.GetFrames().Any(frame => + frame.GetMethod().DeclaringType.FullName.StartsWith("Umbraco.Tests")); + if (testing == false) + throw new InvalidOperationException("Only unit tests can reset configuration."); + */ + + 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] From e6bb999d83072d4515dc3e80f4bdd24b38c1271c Mon Sep 17 00:00:00 2001 From: Stephan Date: Wed, 2 Oct 2013 11:42:17 +0200 Subject: [PATCH 3/5] Core.Resolution - umbraco.cms.Action should NOT reset resolution --- src/umbraco.cms/Actions/Action.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/umbraco.cms/Actions/Action.cs b/src/umbraco.cms/Actions/Action.cs index 95c2d45c2a..1dda0948fd 100644 --- a/src/umbraco.cms/Actions/Action.cs +++ b/src/umbraco.cms/Actions/Action.cs @@ -56,7 +56,7 @@ namespace umbraco.BusinessLogic.Actions using (Umbraco.Core.ObjectResolution.Resolution.DirtyBackdoorToConfiguration) { //TODO: Based on the above, this is a big hack as types should all be cleared on package install! - ActionsResolver.Reset(); + ActionsResolver.Reset(false); // and do NOT reset the whole resolution! ActionHandlers.Clear(); //TODO: Based on the above, this is a big hack as types should all be cleared on package install! From f9663a9e80936ad4e8f56bafecead071074dbc2b Mon Sep 17 00:00:00 2001 From: Stephan Date: Wed, 2 Oct 2013 13:59:02 +0200 Subject: [PATCH 4/5] Core.EnumerableExtensions - refactor InGroupsOf to enumerate source only once --- src/Umbraco.Core/EnumerableExtensions.cs | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/Umbraco.Core/EnumerableExtensions.cs b/src/Umbraco.Core/EnumerableExtensions.cs index 3fa4847a26..dfdd26cd84 100644 --- a/src/Umbraco.Core/EnumerableExtensions.cs +++ b/src/Umbraco.Core/EnumerableExtensions.cs @@ -13,14 +13,23 @@ namespace Umbraco.Core { public static IEnumerable> InGroupsOf(this IEnumerable source, int groupSize) { - var i = 0; - var length = source.Count(); + if (source == null) + throw new NullReferenceException("source"); - while ((i * groupSize) < length) + // enumerate the source only once! + var enumerator = source.GetEnumerator(); + + while (enumerator.MoveNext()) + yield return EnumerateGroup(enumerator, groupSize); + } + + private static IEnumerable EnumerateGroup(IEnumerator enumerator, int count) + { + var c = 1; + do { - yield return source.Skip(i * groupSize).Take(groupSize); - i++; - } + yield return enumerator.Current; + } while (c++ < count && enumerator.MoveNext()); } From f64b8d680d3c31e89c4be3d634b3f08dde90ccaa Mon Sep 17 00:00:00 2001 From: Stephan Date: Wed, 2 Oct 2013 14:00:53 +0200 Subject: [PATCH 5/5] Core.StringExtensions - switch over to the new DefaultShortStringHelper --- src/Umbraco.Core/CoreBootManager.cs | 17 ++++++++++------- src/Umbraco.Core/StringExtensions.cs | 15 ++++++++++++++- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/Umbraco.Core/CoreBootManager.cs b/src/Umbraco.Core/CoreBootManager.cs index 57d31d7e00..6f69766f9d 100644 --- a/src/Umbraco.Core/CoreBootManager.cs +++ b/src/Umbraco.Core/CoreBootManager.cs @@ -264,14 +264,17 @@ namespace Umbraco.Core PropertyEditorValueConvertersResolver.Current.AddType(); PropertyEditorValueConvertersResolver.Current.AddType(); - // this is how we'd switch over to DefaultShortStringHelper _and_ still use - // UmbracoSettings UrlReplaceCharacters... - //ShortStringHelperResolver.Current = new ShortStringHelperResolver( - // new DefaultShortStringHelper().WithConfig(DefaultShortStringHelper.ApplyUrlReplaceCharacters)); - - // use the Legacy one for now + // use the new DefaultShortStringHelper but sort-of remain compatible + // - use UmbracoSettings UrlReplaceCharacters + // - allow underscores in terms, allow leading digits ShortStringHelperResolver.Current = new ShortStringHelperResolver( - new LegacyShortStringHelper()); + new DefaultShortStringHelper() + .WithConfig(CleanStringType.Url, DefaultShortStringHelper.ApplyUrlReplaceCharacters, + allowUnderscoreInTerm: true, allowLeadingDigits: true)); + + // that was the old one + //ShortStringHelperResolver.Current = new ShortStringHelperResolver( + // new LegacyShortStringHelper()); UrlSegmentProviderResolver.Current = new UrlSegmentProviderResolver( typeof (DefaultUrlSegmentProvider)); diff --git a/src/Umbraco.Core/StringExtensions.cs b/src/Umbraco.Core/StringExtensions.cs index e730bafbdc..8582324c51 100644 --- a/src/Umbraco.Core/StringExtensions.cs +++ b/src/Umbraco.Core/StringExtensions.cs @@ -781,8 +781,21 @@ namespace Umbraco.Core /// does initialise the resolver. private static IShortStringHelper ShortStringHelper { - get { return ShortStringHelperResolver.HasCurrent ? ShortStringHelperResolver.Current.Helper : new LegacyShortStringHelper(); } + get + { + if (ShortStringHelperResolver.HasCurrent) + return ShortStringHelperResolver.Current.Helper; + if (_helper != null) + return _helper; + + // there *has* to be a short string helper, even if the resolver has not + // been initialized - used the default one with default configuration. + _helper = new DefaultShortStringHelper().WithConfig(allowLeadingDigits: true); + _helper.Freeze(); + return _helper; + } } + private static IShortStringHelper _helper; /// /// Returns a new string in which all occurences of specified strings are replaced by other specified strings.