From f3c5d7c9d6e81695dc795e5f5655a11c018bb11a Mon Sep 17 00:00:00 2001 From: Arun Pereira Date: Thu, 30 Jul 2026 11:33:22 -0400 Subject: [PATCH 1/2] Run feature factories outside the OperationFeatureCollection write lock GetOrSetSafe invoked the caller's factory while holding _writeLock, which lets two execution locks be taken in opposite orders: Operation.GetSelectionSet takes the operation lock, then CompileSelectionSet writes selection features (OperationCompiler sets the optimizer feature), so it holds the operation lock and wants the feature lock. GetOrCreateSelectorExpression takes the feature lock, then the factory builds the selector expression, which calls Operation.GetSelectionSet, so it holds the feature lock and wants the operation lock. Two threads hitting both paths on one operation deadlock permanently. Every later request completing a composite value then blocks on the same operation lock, the pool injects threads to replace them, and the server stops answering. Only the first execution of an operation compiles selection sets and selector expressions, so it takes a schema with many distinct operations to hit reliably, but it needs no unusual input and does not recover. The factory now runs before the lock is taken, which is entered only to store the result. Racing callers can each build a value, the first stored wins, and every caller receives that instance, matching ConcurrentDictionary.GetOrAdd. --- .../OperationFeatureCollection.Selections.cs | 51 ++++++--- .../Processing/OperationFeatureCollection.cs | 48 +++++--- .../OperationFeatureCollectionTests.cs | 103 ++++++++++++++++++ 3 files changed, 166 insertions(+), 36 deletions(-) create mode 100644 src/HotChocolate/Core/test/Execution.Tests/Processing/OperationFeatureCollectionTests.cs diff --git a/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.Selections.cs b/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.Selections.cs index bb718cc5eb7..193e02b8cae 100644 --- a/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.Selections.cs +++ b/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.Selections.cs @@ -52,19 +52,29 @@ internal TFeature GetOrSetSafe(int selectionId, Func factory { ArgumentNullException.ThrowIfNull(factory); - if (!TryGet(selectionId, out var feature)) + if (TryGet(selectionId, out var feature)) { - lock (_writeLock) + return feature; + } + + // The factory runs outside the lock on purpose. Feature factories reach back into the + // operation (building a projection expression calls Operation.GetSelectionSet), which takes + // the operation lock, while compiling a selection set writes selection features from under + // that same lock. Invoking the factory here would let the two locks be taken in opposite + // orders and deadlock. Racing callers may each build a value; the first one stored wins and + // every caller gets that instance, as with ConcurrentDictionary.GetOrAdd. + var created = factory(); + + lock (_writeLock) + { + if (TryGet(selectionId, out var existing)) { - if (!TryGet(selectionId, out feature)) - { - feature = factory(); - this[selectionId, typeof(TFeature)] = feature; - } + return existing; } - } - return feature; + this[selectionId, typeof(TFeature)] = created; + return created; + } } internal TFeature GetOrSetSafe( @@ -74,19 +84,24 @@ internal TFeature GetOrSetSafe( { ArgumentNullException.ThrowIfNull(factory); - if (!TryGet(selectionId, out var feature)) + if (TryGet(selectionId, out var feature)) { - lock (_writeLock) + return feature; + } + + // Runs outside the lock, see the overload above. + var created = factory(context); + + lock (_writeLock) + { + if (TryGet(selectionId, out var existing)) { - if (!TryGet(selectionId, out feature)) - { - feature = factory(context); - this[selectionId, typeof(TFeature)] = feature; - } + return existing; } - } - return feature; + this[selectionId, typeof(TFeature)] = created; + return created; + } } internal IEnumerable> GetFeatures(int selectionId) diff --git a/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.cs b/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.cs index 292351b12b0..9480531b025 100644 --- a/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.cs +++ b/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.cs @@ -94,19 +94,26 @@ public TFeature GetOrSetSafe(Func factory) { ArgumentNullException.ThrowIfNull(factory); - if (!TryGet(out var feature)) + if (TryGet(out var feature)) { - lock (_writeLock) + return feature; + } + + // Invoked outside the lock so a factory that reaches back into the operation cannot deadlock + // against a thread holding the operation lock and writing features here. The first value + // stored wins and is returned to every caller. + var created = factory(); + + lock (_writeLock) + { + if (TryGet(out var existing)) { - if (!TryGet(out feature)) - { - feature = factory(); - this[typeof(TFeature)] = feature; - } + return existing; } - } - return feature; + this[typeof(TFeature)] = created; + return created; + } } internal TFeature GetOrSetSafe( @@ -115,19 +122,24 @@ internal TFeature GetOrSetSafe( { ArgumentNullException.ThrowIfNull(factory); - if (!TryGet(out var feature)) + if (TryGet(out var feature)) { - lock (_writeLock) + return feature; + } + + // Runs outside the lock, see the overload above. + var created = factory(context); + + lock (_writeLock) + { + if (TryGet(out var existing)) { - if (!TryGet(out feature)) - { - feature = factory(context); - this[typeof(TFeature)] = feature; - } + return existing; } - } - return feature; + this[typeof(TFeature)] = created; + return created; + } } /// diff --git a/src/HotChocolate/Core/test/Execution.Tests/Processing/OperationFeatureCollectionTests.cs b/src/HotChocolate/Core/test/Execution.Tests/Processing/OperationFeatureCollectionTests.cs new file mode 100644 index 00000000000..0d206c2794a --- /dev/null +++ b/src/HotChocolate/Core/test/Execution.Tests/Processing/OperationFeatureCollectionTests.cs @@ -0,0 +1,103 @@ +namespace HotChocolate.Execution.Processing; + +/// +/// Feature factories reach back into the operation: building a projection selector expression calls +/// Operation.GetSelectionSet, which takes the operation lock. Compiling a selection set runs the +/// other way round, holding the operation lock while it writes selection features here. Invoking a +/// factory while this collection's write lock is held lets the two locks be taken in opposite orders, +/// which deadlocks the executor. +/// +public class OperationFeatureCollectionTests +{ + private static readonly TimeSpan s_timeout = TimeSpan.FromSeconds(10); + + [Fact] + public void GetOrSetSafe_Should_NotHoldWriteLock_When_SelectionFactoryRuns() + { + // arrange + var features = new OperationFeatureCollection(); + var concurrentWriteFinished = new ManualResetEventSlim(false); + + // act + // the write from the other thread takes the collection lock, so it only completes if the + // factory is not holding it + var feature = features.GetOrSetSafe( + selectionId: 1, + () => + { + var writer = Task.Run(() => + { + features[2, typeof(OtherFeature)] = new OtherFeature(); + concurrentWriteFinished.Set(); + }); + + concurrentWriteFinished.Wait(s_timeout); + writer.Wait(s_timeout); + return new Feature(); + }); + + // assert + Assert.True( + concurrentWriteFinished.IsSet, + "the concurrent write blocked, so the factory ran while holding the write lock"); + Assert.True(features.TryGet(1, out var stored)); + Assert.Same(feature, stored); + } + + [Fact] + public void GetOrSetSafe_Should_NotHoldWriteLock_When_OperationFactoryRuns() + { + // arrange + var features = new OperationFeatureCollection(); + var concurrentWriteFinished = new ManualResetEventSlim(false); + + // act + var feature = features.GetOrSetSafe(() => + { + var writer = Task.Run(() => + { + features[typeof(OtherFeature)] = new OtherFeature(); + concurrentWriteFinished.Set(); + }); + + concurrentWriteFinished.Wait(s_timeout); + writer.Wait(s_timeout); + return new Feature(); + }); + + // assert + Assert.True( + concurrentWriteFinished.IsSet, + "the concurrent write blocked, so the factory ran while holding the write lock"); + Assert.Same(feature, features.Get()); + } + + [Fact] + public async Task GetOrSetSafe_Should_ReturnTheSameInstanceToEveryCaller_When_CallersRace() + { + // arrange + // the factory now runs outside the lock, so racing callers can each build a value + var features = new OperationFeatureCollection(); + var start = new ManualResetEventSlim(false); + var callers = Enumerable.Range(0, 8) + .Select(_ => Task.Run(() => + { + start.Wait(s_timeout); + return features.GetOrSetSafe(selectionId: 1, () => new Feature()); + })) + .ToArray(); + + // act + start.Set(); + var features1 = await Task.WhenAll(callers) + .WaitAsync(s_timeout, TestContext.Current.CancellationToken); + + // assert + Assert.True(features.TryGet(1, out var stored)); + Assert.All(features1, f => Assert.Same(stored, f)); + } + + private sealed class Feature; + + private sealed class OtherFeature; +} From fe58c309702a0a299e6df9f18abb4ff50e540b74 Mon Sep 17 00:00:00 2001 From: Arun Pereira Date: Fri, 31 Jul 2026 03:09:10 -0400 Subject: [PATCH 2/2] Gate feature creation per feature instead of holding the write lock Review feedback: building a feature exactly once is deliberate, because creation is costly where this is used, so the previous commit's GetOrAdd semantics were the wrong trade. Creation is now gated per feature by a Lazy, which keeps it to once while the build itself happens with no lock held. A single write lock guards every feature of the operation, which is the only reason the two deadlocking paths are in each other's way: they touch different features. Building a projection selector calls Operation.GetSelectionSet and so wants the operation lock, while compiling a selection set holds the operation lock and writes an optimizer feature here. Gating per feature lets both proceed. Note that purity of the built value does not avoid this. CreateSelectorExpression is pure, and still deadlocks, because it acquires the operation lock while this lock is held; the cycle is in the lock order, not in the value. Racing callers may each allocate a Lazy, but only the one that wins the slot is evaluated, so the factory runs once per feature. Lazy rather than Lazy because the trimmer requires a parameterless constructor on the type argument, which a feature need not have, and the value is stored as object regardless. The two deadlock tests asserted after the factory returned, by which point the lock is released and the concurrent write completes on its own, so they passed even when the lock was held. They now assert on the wait inside the factory. A third test covers single creation under a race, which fails against the previous commit. --- .../OperationFeatureCollection.Selections.cs | 41 +++++++++++--- .../Processing/OperationFeatureCollection.cs | 30 ++++++++-- .../OperationFeatureCollectionTests.cs | 56 +++++++++++++++---- 3 files changed, 101 insertions(+), 26 deletions(-) diff --git a/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.Selections.cs b/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.Selections.cs index 193e02b8cae..b60f688344a 100644 --- a/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.Selections.cs +++ b/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.Selections.cs @@ -1,3 +1,4 @@ +using System.Collections.Concurrent; using System.Collections.Immutable; using System.Diagnostics.CodeAnalysis; @@ -6,6 +7,10 @@ namespace HotChocolate.Execution.Processing; [SuppressMessage("ReSharper", "NonAtomicCompoundOperator")] public sealed partial class OperationFeatureCollection { + // One creation gate per feature, so a feature is still built exactly once while the build itself + // runs outside the write lock. Lives as long as the operation, like the features themselves. + private readonly ConcurrentDictionary<(int, Type), object> _selectionFactories = new(); + internal object? this[int selectionId, Type featureType] { get @@ -57,13 +62,16 @@ internal TFeature GetOrSetSafe(int selectionId, Func factory return feature; } - // The factory runs outside the lock on purpose. Feature factories reach back into the - // operation (building a projection expression calls Operation.GetSelectionSet), which takes - // the operation lock, while compiling a selection set writes selection features from under - // that same lock. Invoking the factory here would let the two locks be taken in opposite - // orders and deadlock. Racing callers may each build a value; the first one stored wins and - // every caller gets that instance, as with ConcurrentDictionary.GetOrAdd. - var created = factory(); + // The factory must not run while the write lock is held. Feature factories reach back into the + // operation (building a projection selector calls Operation.GetSelectionSet, which takes the + // operation lock), while compiling a selection set holds the operation lock and writes selection + // features here. Invoking the factory under this lock lets the two be taken in opposite orders, + // which deadlocks the executor. + // + // A single lock guards every feature of the operation, so those two are only ever in each + // other's way because they share it. Gating creation per feature keeps a costly value built + // exactly once without holding anything while it is built. + var created = GetOrCreate(selectionId, factory); lock (_writeLock) { @@ -77,6 +85,21 @@ internal TFeature GetOrSetSafe(int selectionId, Func factory } } + private TFeature GetOrCreate(int selectionId, Func factory) + { + // Lazy rather than Lazy: the value is stored as object anyway, and the + // trimmer requires a parameterless constructor on the type argument, which a feature need not + // have. + var lazy = (Lazy)_selectionFactories.GetOrAdd( + (selectionId, typeof(TFeature)), + static (_, f) => new Lazy(() => f()!, LazyThreadSafetyMode.ExecutionAndPublication), + factory); + + // Racing callers may each build a Lazy, but only the one that won the slot is ever evaluated, + // so the factory runs once per feature. + return (TFeature)lazy.Value; + } + internal TFeature GetOrSetSafe( int selectionId, Func factory, @@ -89,8 +112,8 @@ internal TFeature GetOrSetSafe( return feature; } - // Runs outside the lock, see the overload above. - var created = factory(context); + // Built outside the lock and once per feature, see the overload above. + var created = GetOrCreate(selectionId, () => factory(context)); lock (_writeLock) { diff --git a/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.cs b/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.cs index 9480531b025..e8ce4af6285 100644 --- a/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.cs +++ b/src/HotChocolate/Core/src/Types/Execution/Processing/OperationFeatureCollection.cs @@ -1,4 +1,5 @@ using System.Collections; +using System.Collections.Concurrent; using System.Collections.Immutable; using System.Diagnostics.CodeAnalysis; using HotChocolate.Features; @@ -22,6 +23,10 @@ public sealed partial class OperationFeatureCollection : IFeatureCollection #endif private volatile int _containerRevision; + // One creation gate per feature, so a feature is still built exactly once while the build itself + // runs outside the write lock. + private readonly ConcurrentDictionary _factories = new(); + /// /// Initializes a new instance of . /// @@ -99,10 +104,10 @@ public TFeature GetOrSetSafe(Func factory) return feature; } - // Invoked outside the lock so a factory that reaches back into the operation cannot deadlock - // against a thread holding the operation lock and writing features here. The first value - // stored wins and is returned to every caller. - var created = factory(); + // Built outside the lock, so a factory that reaches back into the operation cannot deadlock + // against a thread holding the operation lock and writing features here, and gated per feature + // so a costly value is still built only once. + var created = GetOrCreate(factory); lock (_writeLock) { @@ -127,8 +132,8 @@ internal TFeature GetOrSetSafe( return feature; } - // Runs outside the lock, see the overload above. - var created = factory(context); + // Built outside the lock and once per feature, see the overload above. + var created = GetOrCreate(() => factory(context)); lock (_writeLock) { @@ -142,6 +147,19 @@ internal TFeature GetOrSetSafe( } } + private TFeature GetOrCreate(Func factory) + { + // Lazy rather than Lazy: the value is stored as object anyway, and the + // trimmer requires a parameterless constructor on the type argument, which a feature need not + // have. + var lazy = (Lazy)_factories.GetOrAdd( + typeof(TFeature), + static (_, f) => new Lazy(() => f()!, LazyThreadSafetyMode.ExecutionAndPublication), + factory); + + return (TFeature)lazy.Value; + } + /// public bool TryGet([NotNullWhen(true)] out TFeature? feature) { diff --git a/src/HotChocolate/Core/test/Execution.Tests/Processing/OperationFeatureCollectionTests.cs b/src/HotChocolate/Core/test/Execution.Tests/Processing/OperationFeatureCollectionTests.cs index 0d206c2794a..ec47346c4cb 100644 --- a/src/HotChocolate/Core/test/Execution.Tests/Processing/OperationFeatureCollectionTests.cs +++ b/src/HotChocolate/Core/test/Execution.Tests/Processing/OperationFeatureCollectionTests.cs @@ -31,15 +31,16 @@ public void GetOrSetSafe_Should_NotHoldWriteLock_When_SelectionFactoryRuns() concurrentWriteFinished.Set(); }); - concurrentWriteFinished.Wait(s_timeout); - writer.Wait(s_timeout); + // Asserted here, not after the factory returns: by then the lock is released and the + // writer completes regardless, so a late check passes even when the lock was held. + Assert.True( + concurrentWriteFinished.Wait(s_timeout), + "the concurrent write blocked, so the factory ran while holding the write lock"); + Assert.True(writer.Wait(s_timeout), "the concurrent write did not finish"); return new Feature(); }); // assert - Assert.True( - concurrentWriteFinished.IsSet, - "the concurrent write blocked, so the factory ran while holding the write lock"); Assert.True(features.TryGet(1, out var stored)); Assert.Same(feature, stored); } @@ -60,15 +61,16 @@ public void GetOrSetSafe_Should_NotHoldWriteLock_When_OperationFactoryRuns() concurrentWriteFinished.Set(); }); - concurrentWriteFinished.Wait(s_timeout); - writer.Wait(s_timeout); + // Asserted here, not after the factory returns: by then the lock is released and the + // writer completes regardless, so a late check passes even when the lock was held. + Assert.True( + concurrentWriteFinished.Wait(s_timeout), + "the concurrent write blocked, so the factory ran while holding the write lock"); + Assert.True(writer.Wait(s_timeout), "the concurrent write did not finish"); return new Feature(); }); // assert - Assert.True( - concurrentWriteFinished.IsSet, - "the concurrent write blocked, so the factory ran while holding the write lock"); Assert.Same(feature, features.Get()); } @@ -76,7 +78,7 @@ public void GetOrSetSafe_Should_NotHoldWriteLock_When_OperationFactoryRuns() public async Task GetOrSetSafe_Should_ReturnTheSameInstanceToEveryCaller_When_CallersRace() { // arrange - // the factory now runs outside the lock, so racing callers can each build a value + // creation is gated per feature, so racing callers share one build var features = new OperationFeatureCollection(); var start = new ManualResetEventSlim(false); var callers = Enumerable.Range(0, 8) @@ -97,6 +99,38 @@ public async Task GetOrSetSafe_Should_ReturnTheSameInstanceToEveryCaller_When_Ca Assert.All(features1, f => Assert.Same(stored, f)); } + [Fact] + public async Task GetOrSetSafe_Should_BuildTheFeatureOnce_When_CallersRace() + { + // arrange + // Feature creation is costly where this is used, so racing callers must not each pay for it. + var features = new OperationFeatureCollection(); + var start = new ManualResetEventSlim(false); + var builds = 0; + var callers = Enumerable.Range(0, 16) + .Select(_ => Task.Run(() => + { + start.Wait(s_timeout); + return features.GetOrSetSafe( + selectionId: 1, + () => + { + Interlocked.Increment(ref builds); + return new Feature(); + }); + })) + .ToArray(); + + // act + start.Set(); + var results = await Task.WhenAll(callers) + .WaitAsync(s_timeout, TestContext.Current.CancellationToken); + + // assert + Assert.Equal(1, Volatile.Read(ref builds)); + Assert.All(results, f => Assert.Same(results[0], f)); + } + private sealed class Feature; private sealed class OtherFeature;