From 7ed6c0c1a2bd75c10e7d90e858ae7ac426f1ff66 Mon Sep 17 00:00:00 2001 From: Steve Megson Date: Sun, 24 Nov 2019 13:44:09 +0000 Subject: [PATCH 1/2] Thread safety in EnsureApplicationUrl --- src/Umbraco.Core/RuntimeState.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Core/RuntimeState.cs b/src/Umbraco.Core/RuntimeState.cs index 5d34fe70a1..65c93ece05 100644 --- a/src/Umbraco.Core/RuntimeState.cs +++ b/src/Umbraco.Core/RuntimeState.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Threading; using System.Web; @@ -22,7 +23,7 @@ namespace Umbraco.Core private readonly ILogger _logger; private readonly IUmbracoSettingsSection _settings; private readonly IGlobalSettings _globalSettings; - private readonly HashSet _applicationUrls = new HashSet(); + private readonly ConcurrentDictionary _applicationUrls = new ConcurrentDictionary(); private readonly Lazy _mainDom; private readonly Lazy _serverRegistrar; @@ -106,11 +107,11 @@ namespace Umbraco.Core // (this is a simplified version of what was in 7.x) // note: should this be optional? is it expensive? var url = request == null ? null : ApplicationUrlHelper.GetApplicationUrlFromCurrentRequest(request, _globalSettings); - var change = url != null && !_applicationUrls.Contains(url); + var change = url != null && !_applicationUrls.ContainsKey(url); if (change) { _logger.Info(typeof(ApplicationUrlHelper), "New url {Url} detected, re-discovering application url.", url); - _applicationUrls.Add(url); + _applicationUrls[url] = 0; } if (ApplicationUrl != null && !change) return; From 311e2afde08131b592ea8dbe5c15514ba969ce47 Mon Sep 17 00:00:00 2001 From: Steve Megson Date: Tue, 26 Nov 2019 08:51:48 +0000 Subject: [PATCH 2/2] Use ConcurrentHashSet --- .../Collections/ConcurrentHashSet.cs | 38 +++++++++++++++---- src/Umbraco.Core/RuntimeState.cs | 9 ++--- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/src/Umbraco.Core/Collections/ConcurrentHashSet.cs b/src/Umbraco.Core/Collections/ConcurrentHashSet.cs index 54367ed588..c4dba51acd 100644 --- a/src/Umbraco.Core/Collections/ConcurrentHashSet.cs +++ b/src/Umbraco.Core/Collections/ConcurrentHashSet.cs @@ -70,7 +70,23 @@ namespace Umbraco.Core.Collections /// The number of elements contained in the . /// /// 2 - public int Count => GetThreadSafeClone().Count; + public int Count + { + get + { + try + { + _instanceLocker.EnterReadLock(); + return _innerSet.Count; + } + finally + { + if (_instanceLocker.IsReadLockHeld) + _instanceLocker.ExitReadLock(); + } + + } + } /// /// Gets a value indicating whether the is read-only. @@ -105,8 +121,7 @@ namespace Umbraco.Core.Collections /// public bool TryAdd(T item) { - var clone = GetThreadSafeClone(); - if (clone.Contains(item)) return false; + if (Contains(item)) return false; try { _instanceLocker.EnterWriteLock(); @@ -150,7 +165,16 @@ namespace Umbraco.Core.Collections /// The object to locate in the . public bool Contains(T item) { - return GetThreadSafeClone().Contains(item); + try + { + _instanceLocker.EnterReadLock(); + return _innerSet.Contains(item); + } + finally + { + if (_instanceLocker.IsReadLockHeld) + _instanceLocker.ExitReadLock(); + } } /// @@ -168,13 +192,13 @@ namespace Umbraco.Core.Collections HashSet clone = null; try { - _instanceLocker.EnterWriteLock(); + _instanceLocker.EnterReadLock(); clone = new HashSet(_innerSet, _innerSet.Comparer); } finally { - if (_instanceLocker.IsWriteLockHeld) - _instanceLocker.ExitWriteLock(); + if (_instanceLocker.IsReadLockHeld) + _instanceLocker.ExitReadLock(); } return clone; } diff --git a/src/Umbraco.Core/RuntimeState.cs b/src/Umbraco.Core/RuntimeState.cs index 65c93ece05..74ec70828b 100644 --- a/src/Umbraco.Core/RuntimeState.cs +++ b/src/Umbraco.Core/RuntimeState.cs @@ -1,9 +1,8 @@ using System; -using System.Collections.Concurrent; -using System.Collections.Generic; using System.Threading; using System.Web; using Semver; +using Umbraco.Core.Collections; using Umbraco.Core.Configuration; using Umbraco.Core.Configuration.UmbracoSettings; using Umbraco.Core.Exceptions; @@ -23,7 +22,7 @@ namespace Umbraco.Core private readonly ILogger _logger; private readonly IUmbracoSettingsSection _settings; private readonly IGlobalSettings _globalSettings; - private readonly ConcurrentDictionary _applicationUrls = new ConcurrentDictionary(); + private readonly ConcurrentHashSet _applicationUrls = new ConcurrentHashSet(); private readonly Lazy _mainDom; private readonly Lazy _serverRegistrar; @@ -107,11 +106,11 @@ namespace Umbraco.Core // (this is a simplified version of what was in 7.x) // note: should this be optional? is it expensive? var url = request == null ? null : ApplicationUrlHelper.GetApplicationUrlFromCurrentRequest(request, _globalSettings); - var change = url != null && !_applicationUrls.ContainsKey(url); + var change = url != null && !_applicationUrls.Contains(url); if (change) { _logger.Info(typeof(ApplicationUrlHelper), "New url {Url} detected, re-discovering application url.", url); - _applicationUrls[url] = 0; + _applicationUrls.Add(url); } if (ApplicationUrl != null && !change) return;