From 19241995e867330983c7d6fe4bfa5c5445ed512f Mon Sep 17 00:00:00 2001 From: Sebastiaan Janssen Date: Thu, 12 Apr 2018 16:23:33 +0200 Subject: [PATCH 1/2] Cherry picked - Fix scope leaks caused by database messenger [U4-11207] #2580 --- src/Umbraco.Core/Persistence/PetaPoco.cs | 15 +++-- src/Umbraco.Core/Scoping/NoScope.cs | 3 + src/Umbraco.Core/Scoping/Scope.cs | 2 +- src/Umbraco.Core/Scoping/ScopeProvider.cs | 57 +++++++++++++++++-- .../BatchedDatabaseServerMessenger.cs | 48 +++++++++++----- 5 files changed, 97 insertions(+), 28 deletions(-) diff --git a/src/Umbraco.Core/Persistence/PetaPoco.cs b/src/Umbraco.Core/Persistence/PetaPoco.cs index 79b3ce3871..4692cb4b51 100644 --- a/src/Umbraco.Core/Persistence/PetaPoco.cs +++ b/src/Umbraco.Core/Persistence/PetaPoco.cs @@ -402,7 +402,7 @@ namespace Umbraco.Core.Persistence } // Helper to handle named parameters from object properties - static Regex rxParams = new Regex(@"(? args_dest) { return rxParams.Replace(_sql, m => @@ -545,7 +545,7 @@ namespace Umbraco.Core.Persistence } // Create a command - static Regex rxParamsPrefix = new Regex(@"(?(sql.SQL, sql.Arguments); } - Regex rxSelect = new Regex(@"\A\s*(SELECT|EXECUTE|CALL)\s", RegexOptions.Compiled | RegexOptions.Singleline | RegexOptions.IgnoreCase | RegexOptions.Multiline); - Regex rxFrom = new Regex(@"\A\s*FROM\s", RegexOptions.Compiled | RegexOptions.Singleline | RegexOptions.IgnoreCase | RegexOptions.Multiline); + static readonly Regex rxSelect = new Regex(@"\A\s*(SELECT|EXECUTE|CALL)\s", RegexOptions.Compiled | RegexOptions.Singleline | RegexOptions.IgnoreCase | RegexOptions.Multiline); + static readonly Regex rxFrom = new Regex(@"\A\s*FROM\s", RegexOptions.Compiled | RegexOptions.Singleline | RegexOptions.IgnoreCase | RegexOptions.Multiline); string AddSelectClause(string sql) { if (sql.StartsWith(";")) @@ -701,9 +701,9 @@ namespace Umbraco.Core.Persistence return Fetch(sql.SQL, sql.Arguments); } - static Regex rxColumns = new Regex(@"\A\s*SELECT\s+((?:\((?>\((?)|\)(?<-depth>)|.?)*(?(depth)(?!))\)|.)*?)(?\((?)|\)(?<-depth>)|.?)*(?(depth)(?!))\)|[\w\(\)\.])+(?:\s+(?:ASC|DESC))?(?:\s*,\s*(?:\((?>\((?)|\)(?<-depth>)|.?)*(?(depth)(?!))\)|[\w\(\)\.])+(?:\s+(?:ASC|DESC))?)*", RegexOptions.IgnoreCase | RegexOptions.Multiline | RegexOptions.Singleline | RegexOptions.Compiled); - static Regex rxDistinct = new Regex(@"\ADISTINCT\s", RegexOptions.IgnoreCase | RegexOptions.Multiline | RegexOptions.Singleline | RegexOptions.Compiled); + static readonly Regex rxColumns = new Regex(@"\A\s*SELECT\s+((?:\((?>\((?)|\)(?<-depth>)|.?)*(?(depth)(?!))\)|.)*?)(?\((?)|\)(?<-depth>)|.?)*(?(depth)(?!))\)|[\w\(\)\.])+(?:\s+(?:ASC|DESC))?(?:\s*,\s*(?:\((?>\((?)|\)(?<-depth>)|.?)*(?(depth)(?!))\)|[\w\(\)\.])+(?:\s+(?:ASC|DESC))?)*", RegexOptions.IgnoreCase | RegexOptions.Multiline | RegexOptions.Singleline | RegexOptions.Compiled); + static readonly Regex rxDistinct = new Regex(@"\ADISTINCT\s", RegexOptions.IgnoreCase | RegexOptions.Multiline | RegexOptions.Singleline | RegexOptions.Compiled); public static bool SplitSqlForPaging(string sql, out string sqlCount, out string sqlSelectRemoved, out string sqlOrderBy) { sqlSelectRemoved = null; @@ -2546,5 +2546,4 @@ namespace Umbraco.Core.Persistence } } } - } diff --git a/src/Umbraco.Core/Scoping/NoScope.cs b/src/Umbraco.Core/Scoping/NoScope.cs index a21815173c..73353ce3da 100644 --- a/src/Umbraco.Core/Scoping/NoScope.cs +++ b/src/Umbraco.Core/Scoping/NoScope.cs @@ -20,6 +20,7 @@ namespace Umbraco.Core.Scoping public NoScope(ScopeProvider scopeProvider) { _scopeProvider = scopeProvider; + Timestamp = DateTime.Now; #if DEBUG_SCOPES _scopeProvider.RegisterScope(this); #endif @@ -28,6 +29,8 @@ namespace Umbraco.Core.Scoping private readonly Guid _instanceId = Guid.NewGuid(); public Guid InstanceId { get { return _instanceId; } } + public DateTime Timestamp { get; } + /// public bool CallContext { get { return false; } } diff --git a/src/Umbraco.Core/Scoping/Scope.cs b/src/Umbraco.Core/Scoping/Scope.cs index bfee772872..ed23913719 100644 --- a/src/Umbraco.Core/Scoping/Scope.cs +++ b/src/Umbraco.Core/Scoping/Scope.cs @@ -374,7 +374,7 @@ namespace Umbraco.Core.Scoping } var parent = ParentScope; - _scopeProvider.AmbientScope = parent; + _scopeProvider.AmbientScope = parent; // might be null = this is how scopes are removed from context objects #if DEBUG_SCOPES _scopeProvider.Disposed(this); diff --git a/src/Umbraco.Core/Scoping/ScopeProvider.cs b/src/Umbraco.Core/Scoping/ScopeProvider.cs index dfcf7985ae..8b0f84b5d0 100644 --- a/src/Umbraco.Core/Scoping/ScopeProvider.cs +++ b/src/Umbraco.Core/Scoping/ScopeProvider.cs @@ -2,10 +2,12 @@ using System.Collections; using System.Collections.Generic; using System.Data; +using System.Linq; using System.Runtime.Remoting.Messaging; using System.Text; using System.Web; using Umbraco.Core.Events; +using Umbraco.Core.Logging; using Umbraco.Core.Persistence; #if DEBUG_SCOPES using System.Linq; @@ -66,22 +68,38 @@ namespace Umbraco.Core.Scoping // tests, any other things (see https://msdn.microsoft.com/en-us/library/dn458353(v=vs.110).aspx), // but we don't want to make all of our objects serializable since they are *not* meant to be // used in cross-AppDomain scenario anyways. + // // in addition, whatever goes into the logical call context is serialized back and forth any - // time cross-AppDomain code executes, so if we put an "object" there, we'll can *another* - // "object" instance - and so we cannot use a random object as a key. + // time cross-AppDomain code executes, so if we put an "object" there, we'll get *another* + // "object" instance back - and so we cannot use a random object as a key. + // // so what we do is: we register a guid in the call context, and we keep a table mapping those // guids to the actual objects. the guid serializes back and forth without causing any issue, // and we can retrieve the actual objects from the table. - // only issue: how are we supposed to clear the table? we can't, really. objects should take - // care of de-registering themselves from context. - // everything we use does, except the NoScope scope, which just stays there // - // during tests, NoScope can to into call context... nothing much we can do about it + // so far, the only objects that go into this table are scopes (using ScopeItemKey) and + // scope contexts (using ContextItemKey). private static readonly object StaticCallContextObjectsLock = new object(); private static readonly Dictionary StaticCallContextObjects = new Dictionary(); + // normal scopes and scope contexts take greate care removing themselves when disposed, so it + // is all safe. OTOH the NoScope *CANNOT* remove itself, this is by design, it *WILL* leak and + // there is little (nothing) we can do about it - NoScope exists for backward compatibility + // reasons and relying on it is greatly discouraged. + // + // however... we can *try* at protecting the app against memory leaks, by collecting NoScope + // instances that are too old. if anything actually *need* to retain a NoScope instance for + // a long time, it will break. but that's probably ok. so: the constants below define how + // long a NoScope instance can stay in the table before being removed, and how often we should + // collect the table - and collecting happens anytime SetCallContextObject is invoked + + private static readonly TimeSpan StaticCallContextNoScopeLifeSpan = TimeSpan.FromMinutes(30); + private static readonly TimeSpan StaticCallContextCollectPeriod = TimeSpan.FromMinutes(4); + private static DateTime _staticCallContextLastCollect = DateTime.MinValue; + + #if DEBUG_SCOPES public Dictionary CallContextObjects { @@ -156,6 +174,7 @@ namespace Umbraco.Core.Scoping //Logging.LogHelper.Debug("At:\r\n" + Head(Environment.StackTrace, 24)); #endif StaticCallContextObjects.Remove(objectKey); + CollectStaticCallContextObjectsLocked(); } } else @@ -171,11 +190,37 @@ namespace Umbraco.Core.Scoping //Logging.LogHelper.Debug("At:\r\n" + Head(Environment.StackTrace, 24)); #endif StaticCallContextObjects.Add(objectKey, value); + CollectStaticCallContextObjectsLocked(); } CallContext.LogicalSetData(key, objectKey); } } + private static void CollectStaticCallContextObjectsLocked() + { + // is it time to collect? + var now = DateTime.Now; + if (now - _staticCallContextLastCollect <= StaticCallContextCollectPeriod) + return; + + // disable warning: this method is invoked from within a lock + // ReSharper disable InconsistentlySynchronizedField + var threshold = now.Add(-StaticCallContextNoScopeLifeSpan); + var guids = StaticCallContextObjects + .Where(x => x.Value is NoScope noScope && noScope.Timestamp < threshold) + .Select(x => x.Key) + .ToList(); + if (guids.Count > 0) + { + LogHelper.Warn($"Collected {guids.Count} NoScope instances from StaticCallContextObjects."); + foreach (var guid in guids) + StaticCallContextObjects.Remove(guid); + } + // ReSharper restore InconsistentlySynchronizedField + + _staticCallContextLastCollect = now; + } + // this is for tests exclusively until we have a proper accessor in v8 internal static Func HttpContextItemsGetter { get; set; } diff --git a/src/Umbraco.Web/BatchedDatabaseServerMessenger.cs b/src/Umbraco.Web/BatchedDatabaseServerMessenger.cs index 973ba341f2..6862986ec2 100644 --- a/src/Umbraco.Web/BatchedDatabaseServerMessenger.cs +++ b/src/Umbraco.Web/BatchedDatabaseServerMessenger.cs @@ -9,6 +9,7 @@ using Umbraco.Core.Models.Rdbms; using Umbraco.Core.Sync; using Umbraco.Web.Routing; using Umbraco.Core.Logging; +using Umbraco.Core.Scoping; using Umbraco.Web.Scheduling; namespace Umbraco.Web @@ -21,9 +22,12 @@ namespace Umbraco.Web /// public class BatchedDatabaseServerMessenger : DatabaseServerMessenger { + private readonly ApplicationContext _appContext; + public BatchedDatabaseServerMessenger(ApplicationContext appContext, bool enableDistCalls, DatabaseServerMessengerOptions options) : base(appContext, enableDistCalls, options) { + _appContext = appContext; Scheduler.Initializing += Scheduler_Initializing; } @@ -42,7 +46,7 @@ namespace Umbraco.Web //start the background task runner for processing instructions const int delayMilliseconds = 60000; var instructionProcessingRunner = new BackgroundTaskRunner("InstructionProcessing", ApplicationContext.ProfilingLogger.Logger); - var instructionProcessingTask = new InstructionProcessing(instructionProcessingRunner, this, delayMilliseconds, Options.ThrottleSeconds * 1000); + var instructionProcessingTask = new InstructionProcessing(instructionProcessingRunner, this, _appContext.ScopeProvider, delayMilliseconds, Options.ThrottleSeconds * 1000); instructionProcessingRunner.TryAdd(instructionProcessingTask); e.Add(instructionProcessingTask); } @@ -73,18 +77,31 @@ namespace Umbraco.Web private class InstructionProcessing : RecurringTaskBase { private readonly DatabaseServerMessenger _messenger; + private readonly IScopeProvider _scopeProvider; public InstructionProcessing(IBackgroundTaskRunner runner, DatabaseServerMessenger messenger, + IScopeProvider scopeProvider, int delayMilliseconds, int periodMilliseconds) : base(runner, delayMilliseconds, periodMilliseconds) { _messenger = messenger; + _scopeProvider = scopeProvider; } public override bool PerformRun() { - _messenger.Sync(); + // beware! + // DatabaseServerMessenger uses _appContext.DatabaseContext.Database without creating + // scopes, and since we are running in a background task, there will be no ambient + // scope (as would be the case within a web request), and so we would end up creating + // (and leaking) a NoScope instance, which is bad - better make sure we have a true + // scope here! - see U4-11207 + using (var scope = _scopeProvider.CreateScope()) + { + _messenger.Sync(); + scope.Complete(); + } //return true to repeat return true; } @@ -121,14 +138,17 @@ namespace Umbraco.Web batch.Clear(); //Write the instructions but only create JSON blobs with a max instruction count equal to MaxProcessingInstructionCount - foreach (var instructionsBatch in instructions.InGroupsOf(Options.MaxProcessingInstructionCount)) + using (var scope = _appContext.ScopeProvider.CreateScope()) { - WriteInstructions(instructionsBatch); + foreach (var instructionsBatch in instructions.InGroupsOf(Options.MaxProcessingInstructionCount)) + { + WriteInstructions(scope, instructionsBatch); + } + scope.Complete(); } - } - private void WriteInstructions(IEnumerable instructions) + private void WriteInstructions(IScope scope, IEnumerable instructions) { var dto = new CacheInstructionDto { @@ -137,8 +157,7 @@ namespace Umbraco.Web OriginIdentity = LocalIdentity, InstructionCount = instructions.Sum(x => x.JsonIdCount) }; - - ApplicationContext.DatabaseContext.Database.Insert(dto); + scope.Database.Insert(dto); } protected ICollection GetBatch(bool create) @@ -179,16 +198,19 @@ namespace Umbraco.Web if (batch == null) { //only write the json blob with a maximum count of the MaxProcessingInstructionCount - foreach (var maxBatch in instructions.InGroupsOf(Options.MaxProcessingInstructionCount)) + using (var scope = _appContext.ScopeProvider.CreateScope()) { - WriteInstructions(maxBatch); + foreach (var maxBatch in instructions.InGroupsOf(Options.MaxProcessingInstructionCount)) + { + WriteInstructions(scope, maxBatch); + } + scope.Complete(); } } else { batch.Add(new RefreshInstructionEnvelope(servers, refresher, instructions)); } - - } + } } -} \ No newline at end of file +} From 1e2598a3a374d35dbb62c6ab0c4a4fdb5a2662b3 Mon Sep 17 00:00:00 2001 From: Sebastiaan Janssen Date: Thu, 12 Apr 2018 16:36:41 +0200 Subject: [PATCH 2/2] Bumps version to 7.8.2 --- src/SolutionInfo.cs | 4 ++-- src/Umbraco.Core/Configuration/UmbracoVersion.cs | 2 +- src/Umbraco.Web.UI/Umbraco.Web.UI.csproj | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/SolutionInfo.cs b/src/SolutionInfo.cs index 4296625d27..e39fd33eb2 100644 --- a/src/SolutionInfo.cs +++ b/src/SolutionInfo.cs @@ -11,5 +11,5 @@ using System.Resources; [assembly: AssemblyVersion("1.0.*")] -[assembly: AssemblyFileVersion("7.8.1")] -[assembly: AssemblyInformationalVersion("7.8.1")] \ No newline at end of file +[assembly: AssemblyFileVersion("7.8.2")] +[assembly: AssemblyInformationalVersion("7.8.2")] \ No newline at end of file diff --git a/src/Umbraco.Core/Configuration/UmbracoVersion.cs b/src/Umbraco.Core/Configuration/UmbracoVersion.cs index 8ee2095de6..1025d97c3d 100644 --- a/src/Umbraco.Core/Configuration/UmbracoVersion.cs +++ b/src/Umbraco.Core/Configuration/UmbracoVersion.cs @@ -6,7 +6,7 @@ namespace Umbraco.Core.Configuration { public class UmbracoVersion { - private static readonly Version Version = new Version("7.8.1"); + private static readonly Version Version = new Version("7.8.2"); /// /// Gets the current version of Umbraco. diff --git a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj index 03ab1f50f6..6b39c6ca05 100644 --- a/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj +++ b/src/Umbraco.Web.UI/Umbraco.Web.UI.csproj @@ -1024,9 +1024,9 @@ xcopy "$(ProjectDir)"..\packages\SqlServerCE.4.0.0.1\x86\*.* "$(TargetDir)x86\" True True - 7810 + 7820 / - http://localhost:7810 + http://localhost:7820 7800 / http://localhost:7800