Remove locks and switch to ConcurrentDictionary implementation. #15928

diger74 · 2024-03-21T14:34:49Z

Original implementation was causing deadlocks when calling Content Delivery API in some cases.

Prerequisites

This fixes #15827

Description

The original implementation of the property source values cache was based on Dictionary plus lock objects. It was there from long time ago, it was bulletproof and wasn't causing any issues. Until Content Delivery API was introduced.

Before Content Delivery API the cache was simply containing references to other objects since it was all in memory. Delivery API brought the option to expand reference properties into objects, that in turn can have their own properties. Colliding 2 requests for related items started creating a deadlock situation.

At first, I thought the locks were trying to defend nucache read-write scenario, however after the deeper investigation I realized nucache was implemented with ConcurrentDictionary already so there shouldn't be any need for extra locks.

Then I saw a simple Dictionary usage in Property class implementation and noticed only the use of this dictionary was requiring locks. When I swapped Dictionary to ConcurrentDictionary my original issue with deadlocks has gone.

I don't have a full historical knowledge of how this implementation was used and what additional side-effects could be (I'm new to Umbraco, only a few months). If you have a wider set of test tools you can run to identify this fix is safe that would be much appreciated.

And finally, if this fix is okay, is it possible to incorporate something similar for v12 as well, please? Our project has just gone live and we don't have short term plans (budget restriction) for major versions upgrade, however installing a minor version patch is a quick thing to do.

Appreciate your time and have a nice day,
Dmitry

…nal implementation was causing deadlocks when calling Content Delivery API in some cases.

github-actions · 2024-03-21T14:35:04Z

Hi there @diger74, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
The automated tests all pass (see "Checks" tab on this PR)
The level of security for this contribution is the same or improved
The level of performance for this contribution is the same or improved
Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

georgebid · 2024-03-22T08:45:52Z

Hey @diger74, thanks a lot for your PR to fix #15928, and for raising the issue. Someone on the core collaborators team will review this soon 👍

diger74 · 2024-04-01T16:36:10Z

Hey, is there a chance someone could look at my pull request, please?

JasonElkin · 2024-04-02T21:04:12Z

Hey @diger74, yes!

Thanks for this, and for the AMAZING minimal reproducible example 🙌

Let me get this reviewed and tested and I'll come back to you if anything needs tweaking!

JasonElkin · 2024-04-02T22:27:50Z

Thanks for this @diger74,

I've tested it and it fixes the problem as expected! (thanks again for the k6 script etc. really very helpful).

Before I go ahead and merge, I need to have a nerdy chat with HQ about performance to make sure we're doing the best we can here as ConcurrentDictionary slows things down a bit (which is to be expected). You may have noticed that I added a couple of quick wins like swapping to GetOrAdd and Lazy initialisation - this looks to improve things a bit.

diger74 · 2024-04-03T13:14:50Z

Hi @JasonElkin, thanks for your feedback. I'm relatively new to Umbraco therefore was a bit hesitant to add anything more than just swapping dictionary implementation. Yeah, your suggestions look good, thanks for progressing!

Nuklon · 2024-04-03T16:21:17Z

src/Umbraco.PublishedCache.NuCache/Property.cs

@@ -382,26 +349,18 @@ private class CacheValue

    private class CacheValues : CacheValue
    {
-        private Dictionary<CompositeStringStringKey, CacheValue>? _values;
+        private readonly Lazy<ConcurrentDictionary<CompositeStringStringKey, CacheValue>> _values = new();


Lazy shouldn't be necessary here.

Thanks @Nuklon, this is something I was wondering about.

Anecdotally, always creating looks to be faster, but I'm not sure of the memory implications so popped it in a Lazy (just so then we're creating the same number of dictionaries that we were before).

Nuklon · 2024-04-03T16:25:25Z

src/Umbraco.PublishedCache.NuCache/Property.cs

@@ -33,7 +33,7 @@ internal class Property : PublishedPropertyBase
    private object? _interValue;

    // the variant source and inter values
-    private Dictionary<CompositeStringStringKey, SourceInterValue>? _sourceValues;
+    private readonly Lazy<ConcurrentDictionary<CompositeStringStringKey, SourceInterValue>> _sourceValues = new();


Just create the dictionary and avoid Lazy completely?

Same as above, if creating every time is more efficient overall then happy to go with that - but keen to make sure.

Agree. Lazy has historically caused a lot of trouble with disposal and memory leakage.

Hmm. Come to think of it, the _sourceValues dictionary is only created for variant properties. In a LOT of cases, the property is not going to be variant, so the up-front allocation of a dictionary is an overhead compared to today.

Perhaps the Lazy approach is justified here.

Just browsing a bit through the code it seems this _sourceValues is used quite a lot (either through HasValue or GetSourceValue).

nzdev · 2024-04-08T01:44:21Z

Probably not ideal to use ConcurrentDictionary here. https://frugalcafe.beehiiv.com/p/silver-bullet-concurrentdictionary.
Reason being the memory usage will increase, and this may only be coincidentally be working due to the way locks are shared across multiple keys internally in ConcurrentDictionary.
Could a test be added that reproduces the deadlock.

JasonElkin · 2024-04-08T11:53:57Z

Thanks @nzdev, that's a really helpful article!

In #5827, there's a reproducible. It's a race condition that can occur when two documents reference each other via properties.

DocA -> ContentPickerProperty -> DocB
DocB -> ContentPickerProperty -> DocA

If two threads access DocA and DocB simultaneously then they deadlock because each document is waiting for the other's properties to resolve.

The fix makes logical sense. Umbraco needs to support the above scenario, so the property class shouldn't be locking - and this PR removes the locks. On top of this, as @diger74 points out, the locks are for the dictionary, but they also lock idempotent methods that don't touch the dictionary at all.

We don't want to add unnecessary memory overhead, but we need exactly what ConcurrentDictionary does here. The dictionary will only be used for properties with variants, and it's likely to only contain a few entries - using the constructor that set's the default capacity, as pointed out in the article should help: new ConcurrentDictionary(-1, 5). So, we won't be adding bytes and bytes of additional memory for every property and we're also not using the slow/expensive APIs.

If there's a better way of caching the SourceInterValues that doesn't add so much overhead of course we'd prefer that - any ideas?

Nuklon · 2024-04-09T10:02:06Z

src/Umbraco.PublishedCache.NuCache/Property.cs

@@ -33,7 +33,7 @@ internal class Property : PublishedPropertyBase
    private object? _interValue;

    // the variant source and inter values
-    private readonly Lazy<ConcurrentDictionary<CompositeStringStringKey, SourceInterValue>> _sourceValues = new();
+    private readonly Lazy<ConcurrentDictionary<CompositeStringStringKey, SourceInterValue>> _sourceValues = new(() => new(-1, 5));


I think this should be set depending on the expected number of entries, what's common?

Hey @Nuklon,

This SO sums up the choice here...

If you want to

optimize speed: take an initial capacity which is a prime about 30% bigger than the expected maximum dictionary size. This avoids resizing.

optimize the memory footprint: take a prime which is about 30% bigger than the minimum expected size.

a balance between speed and memory footprint: Take a number in between the two from above. But in any case, take a prime.

I'll double check the most common number, but it's _low, as it's effectively number of languages x variations so 5 seemed pretty sensible here.

# Conflicts: # src/Umbraco.PublishedCache.NuCache/Property.cs

kjac · 2024-04-10T10:15:12Z

So. V14 has been merged to the contrib branch and the contrib branch as in turn been merged to this PR. That leaves this PR in a bit of a pickle because it essentially doesn't work against V13.3 RC anymore 😢

As we want these changes in V13.3 RC, I have ported the changes to a new PR that targets V13.3 RC. See #16023

I'm going to close this one. Let's keep the dialog going on #16023.

* Ported over #15928 changes for 13.3 RC * Use GetOrAdd() * Lock dictionary initialization --------- Co-authored-by: Jason Elkin <jasonelkin86@gmail.com>

* Updates JSON schema for Umbraco 10 with latest references for Forms and Deploy (#15918) * Ported over #15928 changes for 13.3 RC (#16023) * Ported over #15928 changes for 13.3 RC * Use GetOrAdd() * Lock dictionary initialization --------- Co-authored-by: Jason Elkin <jasonelkin86@gmail.com> * Make the API content response builder extendable (#16056) * Make the API content response builder extendable * DeliveryApiJsonTypeResolver needs to be extendable too * bump rc to regular * Bump to next minor * Add blocks in RTE telemetry (#16104) * Add blocks telemetry * Use constants and update tests * V13: Add property type information to telemetry (#16109) * Add property type counts to telemetry * Use constants and fix tests * Update description * V10: Fix for fallback file upload (#14892) (#15868) * Fix for fallback file upload (#14892) * Added check for file type * Removed unneeded null checks and fixed tabs * Cleaning * Cleanups, cleanups, and removal of unneeded null checks * Reverted removal of relationshipservice * Revert null check removals (too risky) --------- Co-authored-by: Ambert van Unen <AvanUnen@ilionx.com> Co-authored-by: Laura Neto <12862535+lauraneto@users.noreply.github.com> (cherry picked from commit 0b5d1f8) * Fix up formatting --------- Co-authored-by: Ambert van Unen <ambertvu@gmail.com> * Implementors using Umbraco.Tests.Integration won't have to override GetLocalizedTextService (cherry picked from commit b001668) (cherry picked from commit 2bb56f1) * Fix logic for retrieving lastKnownElement (cherry picked from commit cae106b) * bump version * Bump version * Bump version * Since v13 properties can sometimes be of type IRichTextEditorIntermediateValue - this was unexpected in the XPath navigator code (#16121) * Webhook log improvements (#16200) * fix: include all headers in webhook log * feat: return webhook log status from server * feat: make webhook logs deep linkable * feat: add webhook log pagination * feat: improve webhook request/response body preview * V13: Optimize custom MVC routing (#16218) * Introduce EagerMatcherPolicy to conditionally bypass content routing * Ensure that the candidate we disable dynamic routing for is valid * Skip Umbraco endpoints * Simplify logic a bit * Move install logic to matcher * Ensure that dynamic routing is still skipped when in upgrade state * Fixup comments * Reduce nesting a bit * Don't show maintenance page when statically routed controllers are hít * Remove excess check, since installer requests are statically routed * V13: Optimize custom MVC routing (#16218) * Introduce EagerMatcherPolicy to conditionally bypass content routing * Ensure that the candidate we disable dynamic routing for is valid * Skip Umbraco endpoints * Simplify logic a bit * Move install logic to matcher * Ensure that dynamic routing is still skipped when in upgrade state * Fixup comments * Reduce nesting a bit * Don't show maintenance page when statically routed controllers are hít * Remove excess check, since installer requests are statically routed (cherry picked from commit ba9ddd1) * Property source level variation should only be applied when configured (#16270) * Property source level variation should only be applied when configured (#16270) (cherry picked from commit ab32bac) * Merge pull request from GHSA-j74q-mv2c-rxmp * Merge pull request from GHSA-j74q-mv2c-rxmp * Merge pull request from GHSA-j74q-mv2c-rxmp * Fix up after merge * Remove obselete test --------- Co-authored-by: Andy Butland <abutland73@gmail.com> Co-authored-by: Kenn Jacobsen <kja@umbraco.dk> Co-authored-by: Jason Elkin <jasonelkin86@gmail.com> Co-authored-by: Sven Geusens <sge@umbraco.dk> Co-authored-by: Mole <nikolajlauridsen@protonmail.ch> Co-authored-by: Ambert van Unen <ambertvu@gmail.com> Co-authored-by: Lars-Erik <lars-erik@aabech.no> Co-authored-by: Joshua Daniel Pratt Nielsen <jdpnielsen@gmail.com> Co-authored-by: Bjarke Berg <mail@bergmania.dk> Co-authored-by: Sebastiaan Janssen <sebastiaan@umbraco.com> Co-authored-by: Rasmus John Pedersen <mail@rjp.dk>

Remove locks and switch to ConcurrentDictionary implementation. Origi…

39362ff

…nal implementation was causing deadlocks when calling Content Delivery API in some cases.

JasonElkin self-assigned this Apr 2, 2024

Concurrent dictionary performance improvements

814864e

Nuklon reviewed Apr 3, 2024

View reviewed changes

Set default size of concurrent dictionary to 5

6f7210c

Nuklon reviewed Apr 9, 2024

View reviewed changes

Merge branch 'contrib' into pr/15928

abe7238

# Conflicts: # src/Umbraco.PublishedCache.NuCache/Property.cs

kjac added a commit that referenced this pull request Apr 10, 2024

Ported over #15928 changes for 13.3 RC

259cdac

kjac mentioned this pull request Apr 10, 2024

Ported over #15928 changes for 13.3 RC #16023

Merged

1 task

kjac closed this Apr 10, 2024

bergmania pushed a commit that referenced this pull request Apr 16, 2024

Ported over #15928 changes for 13.3 RC (#16023)

6379f2f

* Ported over #15928 changes for 13.3 RC * Use GetOrAdd() * Lock dictionary initialization --------- Co-authored-by: Jason Elkin <jasonelkin86@gmail.com>

Migaroez mentioned this pull request Jul 15, 2024

Content Delivery API deadlocks when simultaneously requesting items referencing each other #15827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove locks and switch to ConcurrentDictionary implementation. #15928

Remove locks and switch to ConcurrentDictionary implementation. #15928

diger74 commented Mar 21, 2024 •

edited

Loading

github-actions bot commented Mar 21, 2024 •

edited by georgebid

Loading

georgebid commented Mar 22, 2024

diger74 commented Apr 1, 2024

JasonElkin commented Apr 2, 2024

JasonElkin commented Apr 2, 2024

diger74 commented Apr 3, 2024

Nuklon Apr 3, 2024

JasonElkin Apr 3, 2024

Nuklon Apr 3, 2024

JasonElkin Apr 3, 2024

kjac Apr 5, 2024

kjac Apr 5, 2024

Nuklon Apr 5, 2024

nzdev commented Apr 8, 2024 •

edited

Loading

JasonElkin commented Apr 8, 2024 •

edited

Loading

Nuklon Apr 9, 2024

JasonElkin Apr 10, 2024

kjac commented Apr 10, 2024

Remove locks and switch to ConcurrentDictionary implementation. #15928

Remove locks and switch to ConcurrentDictionary implementation. #15928

Conversation

diger74 commented Mar 21, 2024 • edited Loading

Prerequisites

Description

github-actions bot commented Mar 21, 2024 • edited by georgebid Loading

georgebid commented Mar 22, 2024

diger74 commented Apr 1, 2024

JasonElkin commented Apr 2, 2024

JasonElkin commented Apr 2, 2024

diger74 commented Apr 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzdev commented Apr 8, 2024 • edited Loading

JasonElkin commented Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjac commented Apr 10, 2024

diger74 commented Mar 21, 2024 •

edited

Loading

github-actions bot commented Mar 21, 2024 •

edited by georgebid

Loading

nzdev commented Apr 8, 2024 •

edited

Loading

JasonElkin commented Apr 8, 2024 •

edited

Loading