U4-11207 - Leaking NoScope instances in ScopeProvider.StaticCallContextObjects

Created by Stephan 12 Apr 2018, 08:17:53 Updated by Jacob Midtgaard-Olesen 07 May 2018, 08:17:39

Tags: Unscheduled Gold partner

see https://our.umbraco.org/forum/using-umbraco-and-getting-started/91397-792-memory-leak-in-umbracocorescopingscopeproviderstaticcallcontextobjects

see also (gold partner zendesk ticket)

website seems to have a massive memory leak, and analyzing a memory dump shows that the ScopeProvider.StaticCallContextObjects keeps growing, adding instances of NoScope, each owning an instance of UmbracoDatabase, each owning instances of some Regex.

Reported affected version: 7.9.2 (and probably any version after that one)

Comments

Stephan 12 Apr 2018, 08:58:07

Umbraco has this concept of "ambient scopes" which means that when a scope is created, it's available "in context" and one does not need to pass scopes around. Pretty much like HttpContext or TransactionScope from Microsoft. Underneath, scopes are stored in the thread logical call context.

However, it's a bad idea to store true object instances in there, because in some cases (running unit tests, running on some IIS Express environments, running in debug mode in VS with R# installed) the logicall call context gets serialized and de-serialized, and unless the objects in there are serializable, weird things happen.

So instead we store guids in the call context, and then we use StaticCallContextObjects to map from guid to actual object. When a scope is created, a guid is added to the call context, and the scope is added to StaticCallContextObjects. When the scope is eventually disposed, the guid is removed from call context, and the scope is removed from StaticCallContextObjects. Works fine, no leak, all good.

However, it is still possible to get a database connection via DatabaseContext.Database. When this happens and there is an ambient scope, the database connection comes from the scope. But when there is no ambient scope, and for backward compatibility reason, we have to create a weird NoScope thing which ''does'' get added to the context... but is ''very'' hard to remove, since it's never really properly disposed.

Directly using DatabaseContext.Database is ''strongly'' discouraged, and will in fact be impossible in v8.

So at that point... ''something'' must be aggressively using DatabaseContext.Database outside of a scope context. Need to find out what.


Stephan 12 Apr 2018, 08:58:24

About the regular expression leaks... PetaPoco Database class uses various regular expressions to parse and manage the SQL code. Practically all of them (rxParams, rxColumns...) are static, but two of them: rxSelect and rxFrom. I cannot see why these two would need to be per-instance, as MSDN states in [Thead safety in Regular Expression|https://docs.microsoft.com/en-us/dotnet/standard/base-types/thread-safety-in-regular-expressions] that ''"matching methods can be called from any thread and never alter any global state"''.

I am assuming these two regular expressions were just overlooked, and am turning them to static. This means we should allocate two Regex less ''per database connection'' which is cool.


Stephan 12 Apr 2018, 09:02:35

Examining a memory dump of a leaking process, we can see that most of the leaked databases' last executed sql statement were "SELECT TOP 100 * ... FROM umbracoCacheInstruction..." which points us to the BatchedDatabaseServerMessenger. The messenger originally synced at the beginning of ''requests'' and requests have an implicit scope. Syncing was moved to a background task a part of U4-10150 and (to make it short) it involves using DatabaseContext.Database. But, now, outside of any scope. QED.

Fixing by wrapping the background task execution in a "using (var scope = ...)" statement.


Stephan 12 Apr 2018, 09:03:34

Note: U4-10150 was merged sometimes before 7.8.0, so assuming that potentially all versions 7.8 and later are impacted.


Stephan 12 Apr 2018, 09:09:31

Clarification:

Using DatabaseContext.Database ''will leak'' anytime there is no ambient scope. This is not something that can be "fixed". The fix is to wrap things into a scope. And then ideally, to use scope.Database (cleaner)--but DatabaseContext.Database is safe.

There ''is'' an ambient scope during web requests, because Umbraco maintains one, but there is ''no'' ambient scope during background tasks, etc.


Stephan 12 Apr 2018, 09:15:39

PR https://github.com/umbraco/Umbraco-CMS/pull/2580


Stephan 12 Apr 2018, 10:02:08

Have updated the PR - to add a mechanism that periodically collects NoScope instances that have been in the StaticCallContextObjects map for too long, thus assuming they are leaking. It's writing to the log, as a warning, when there are instances to collect. It would be "normal" to see a few of them being collected -- but not tons of them.

Although this can potentially... hide some other issues (as in, we wouldn't have discovered that the database messenger wasn't using a scope)... it's also making the whole thing more robust so it probably is a Good Thing(TM).


Claus Jensen 12 Apr 2018, 12:50:30

Fixed up a few things in the PR (+ a few things per shan's request) and have tested this with a full restore of a site. Didn't find anything broken in regards to that.

Also confirmed that the clearing of abandoned scopes is run, confirmed that instructions are written to the database table and confirmed that they are also still picked up.


Shannon Deminick 12 Apr 2018, 23:38:49

I see no issues with this implementation, moving to fixed. Glad this was discovered!


Morten Bock 17 Apr 2018, 11:03:12

@zpqrtbnk Regarding "Directly using DatabaseContext.Database is strongly discouraged", is there a best practice example somewhere? I tried searching the docs, but i only found an example that actually uses that approach.

Just wondering what the right way to leverage the cores DB stuff is.


Stephan 17 Apr 2018, 11:28:06

Duh, need to update the docs then. Best practice:

using (var scope = ApplicationContext.Current.ScopeProvider.CreateScope()) { var database = scope.Database; // use database scope.Complete(); }

completing the scope is required to commit the underlying transaction. The transaction commits when the scope is completed. Scopes can be nested, they'll use only 1 database transaction which is commited when the outermost scope is completed.

That's a ''very'' quick example. Definitively need to update the doc.


Ronald Barendse 17 Apr 2018, 11:51:16

@zpqrtbnk Thanks for the example and explanation how this in fact works on the database level!

Regarding the missing scopes on background tasks: does this also apply for scheduled tasks (defined in umbracoSettings.config) and health checks? Or just for BackgroundWorker, Thread.Start and ThreadPool.QueueUserWorkItem methods that start new threads?


Shannon Deminick 17 Apr 2018, 23:58:38

Scheduled tasks in Umbraco are web requests so they already operate in a web context


Priority: Major

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.8.0, 7.9.2

Due in version: 7.10.3, 7.8.2, 7.9.5

Sprint: Sprint 82

Story Points: 1

Cycle: 9