Skip to content

Use a dedicated Lock for the export claim, not Umbraco's dictionary - #1020

Merged
KevinJump merged 1 commit into
v18/mainfrom
v18/fix-claim-lock-pattern
Aug 5, 2026
Merged

Use a dedicated Lock for the export claim, not Umbraco's dictionary#1020
KevinJump merged 1 commit into
v18/mainfrom
v18/fix-claim-lock-pattern

Conversation

@KevinJump

Copy link
Copy Markdown
Owner

Follow-up to the note on #1019.

What

PublishableContentHandlerBase.ClaimItemForExport (added in #1018) locked on notification.State — Umbraco's dictionary, not ours. Anything else holding a reference to it could contend with us on an object neither side knows the other is using. That's the pattern #998 moved TemplateWatcher away from.

Now a private static readonly Lock, matching #998.

Why a static

The obvious alternative — a per-instance lock — doesn't work: the whole point of the claim is that it's shared across the notifications of one operation, and handlers are resolved per notification dispatch. A static is what covers them all.

Statics on a generic type are per closed type, so PublishableContentHandlerBase<IContent> and <IElement> get one each. That's a bonus rather than a problem: documents and elements never share a notification state, so they have nothing to contend over.

Contention isn't a concern either way — the critical section is a dictionary lookup and a HashSet.Add, and uSync's own imports pause events, so this only runs on editor saves.

Test

Added Concurrent_Claims_Only_Let_One_Caller_Through: 50 items × 8 threads racing the claim through Parallel.For, asserting exactly 50 claims succeed.

I checked it actually fails without the lock rather than passing regardless — it throws IndexOutOfRangeException out of HashSet.Add as concurrent adds corrupt the set:

--IndexOutOfRangeException
   at System.Collections.Generic.HashSet`1.AddIfNotPresent(T value, Int32& location)
   at System.Collections.Generic.HashSet`1.Add(T item)
   at uSync.BackOffice.SyncHandlers.Handlers.PublishableContentHandlerBase`1.ClaimItemForExport(...)

Builds clean; 211 tests pass.

Note

No changelog entry — this is an internal correctness fix to code that hasn't shipped in a release yet (#1018 is unreleased on v18/main), and the behaviour is unchanged.

Worth saying plainly: Umbraco raises the notifications for one operation sequentially, so this guard has always been belt-and-braces rather than something we expect to contend on. It matters because the state dictionary is a plain Dictionary and SyncScopedNotificationPublisher can dispatch on a queued background thread when BackgroundNotifications is on — the cost of being wrong is a corrupted set, not just a duplicate export.

🤖 Generated with Claude Code

ClaimItemForExport locked on notification.State - Umbraco's dictionary,
not ours. Anything else holding a reference to it could contend with us on
an object neither side knows the other is using. Replaced with a private
static Lock, following the pattern from #998.

Statics on a generic type are per closed type, so documents and elements
get a lock each. That suits us: they never share a notification state, so
they have nothing to contend over.

Added a test that races the claim across threads. Verified it fails without
the lock - concurrent HashSet.Add corrupts the set and throws
IndexOutOfRangeException - so it actually proves the guard is doing
something.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@KevinJump
KevinJump merged commit 3c3ff54 into v18/main Aug 5, 2026
5 checks passed
@KevinJump
KevinJump deleted the v18/fix-claim-lock-pattern branch August 5, 2026 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant