From fd406f48ad2301fd938ad1027aea3010ae85700c Mon Sep 17 00:00:00 2001 From: Ivan Murzak Date: Fri, 6 Nov 2026 02:08:19 -0800 Subject: [PATCH 1/4] fix(csharp): a Pulse while a sliced Monitor.Wait is between two slices is not lost A slice that times out leaves the wait queue until its thread holds the lock again; a Pulse sent in that gap was lost, and the waiter slept until the budget (GuardedCodeKeepsItsMeaning failed intermittently in CI). The agent's Monitor.Pulse / PulseAll now become guard stand-ins that count what they owe the object's sliced waiters; a waiter between two slices takes it as its wake-up. New deterministic test: the pulsing thread holds the lock longer than a slice. Co-Authored-By: Claude Opus 5.5 --- dotnet/CodeExec.Roslyn/src/GuardWriter.cs | 3 ++ dotnet/CodeExec.Tests/GuardTests.cs | 26 ++++++++++ dotnet/CodeExec/ExecutionGuard.cs | 61 +++++++++++++++++++++-- spec/contract.md | 4 +- 4 files changed, 89 insertions(+), 5 deletions(-) diff --git a/dotnet/CodeExec.Roslyn/src/GuardWriter.cs b/dotnet/CodeExec.Roslyn/src/GuardWriter.cs index 20f0d63..98d48ba 100644 --- a/dotnet/CodeExec.Roslyn/src/GuardWriter.cs +++ b/dotnet/CodeExec.Roslyn/src/GuardWriter.cs @@ -265,6 +265,9 @@ void Add(string type, string method, string guard, params string[][] overloads) foreach (var pair in new[] { new[] { "SemaphoreSlim", "SemaphoreWait" }, new[] { "ManualResetEventSlim", "EventWait" }, new[] { "CountdownEvent", "CountdownWait" } }) Add("System.Threading." + pair[0], "Wait", pair[1], none, new[] { Ms }, new[] { Span }, new[] { Token }, new[] { Ms, Token }, new[] { Span, Token }); Add("System.Threading.Monitor", "Wait", "MonitorWait", new[] { "object" }, new[] { "object", Ms }, new[] { "object", Span }); + // Not waits, but the sliced Monitor.Wait needs them: a pulse while its waiter is between two slices is not lost. + Add("System.Threading.Monitor", "Pulse", "MonitorPulse", new[] { "object" }); + Add("System.Threading.Monitor", "PulseAll", "MonitorPulseAll", new[] { "object" }); Add("System.Threading.WaitHandle", "WaitOne", "WaitOne", none, new[] { Ms }, new[] { Span }); Add("System.Threading.WaitHandle", "WaitAll", "WaitAll", new[] { HandleArray }, new[] { HandleArray, Ms }, new[] { HandleArray, Span }); Add("System.Threading.WaitHandle", "WaitAny", "WaitAny", new[] { HandleArray }, new[] { HandleArray, Ms }, new[] { HandleArray, Span }); diff --git a/dotnet/CodeExec.Tests/GuardTests.cs b/dotnet/CodeExec.Tests/GuardTests.cs index 6859c7e..7e37bdc 100644 --- a/dotnet/CodeExec.Tests/GuardTests.cs +++ b/dotnet/CodeExec.Tests/GuardTests.cs @@ -587,6 +587,32 @@ public async Task GuardedCodeKeepsItsMeaning(string code, string expected) Assert.Equal(expected, result.Value); } + // A sliced Monitor.Wait leaves the wait queue each time a slice times out, until it holds the lock again: a Pulse in + // that gap must still wake it. Here the pulsing thread holds the lock for longer than a slice, so the waiter's slice + // always times out before the Pulse (without the pulse count the waiter sleeps until the budget). + [Theory] + [InlineData("System.Threading.Monitor.Pulse(o);", "1")] + [InlineData("System.Threading.Monitor.PulseAll(o);", "2")] + public void APulseBetweenTwoSlicesIsNotLost(string pulse, string woken) + { + var code = "var o = new object();\nvar woken = 0;\nvar entered = new System.Threading.CountdownEvent(2);\n" + + "System.Threading.Thread Waiter() { var t = new System.Threading.Thread(() => { lock (o) { entered.Signal(); if (System.Threading.Monitor.Wait(o, 3000)) woken++; } }); t.Start(); return t; }\n" + + "var a = Waiter();\nvar b = Waiter();\nentered.Wait();\n" + + "lock (o) { System.Threading.Thread.Sleep(200); " + pulse + " }\n" + + "a.Join();\nb.Join();\nwoken"; + var result = Fixtures.Runner().Execute(new ExecuteRequest { Code = code, TimeoutMs = 20_000 }); + Assert.True(result.Success, result.Error + " " + string.Join(" | ", result.Diagnostics)); + Assert.Equal(woken, result.Value); + } + + [Fact] + public void PulseWithoutTheLockStillThrows() + { + var result = Fixtures.Runner().Execute(new ExecuteRequest { Code = "try { System.Threading.Monitor.Pulse(new object()); } catch (System.Threading.SynchronizationLockException) { return \"sync\"; }\nreturn \"\";" }); + Assert.True(result.Success, result.Error); + Assert.Equal("sync", result.Value); + } + // Inserted guard calls sit on lines of their own: the agent's lines AND columns stay. [Theory] [InlineData("while (false) { } int x = undefinedThing;", 1, 27)] diff --git a/dotnet/CodeExec/ExecutionGuard.cs b/dotnet/CodeExec/ExecutionGuard.cs index 37cbc9b..491d0fd 100644 --- a/dotnet/CodeExec/ExecutionGuard.cs +++ b/dotnet/CodeExec/ExecutionGuard.cs @@ -406,10 +406,63 @@ public static T Ready(T task) where T : global::System.Threading.Tasks.Task public static bool CountdownWait(global::System.Threading.CountdownEvent waitable, global::System.TimeSpan timeout, global::System.Threading.CancellationToken cancellationToken) => CountdownWait(waitable, Ms(timeout), cancellationToken); public static bool CountdownWait(global::System.Threading.CountdownEvent waitable, int millisecondsTimeout, global::System.Threading.CancellationToken cancellationToken) => Sliced(millisecondsTimeout, ms => waitable.Wait(ms, cancellationToken)); - // Waiting again after a slice misses no Pulse: the lock is held again between two slices. - public static bool MonitorWait(object obj) => Sliced(-1, ms => global::System.Threading.Monitor.Wait(obj, ms)); - public static bool MonitorWait(object obj, int millisecondsTimeout) => Sliced(millisecondsTimeout, ms => global::System.Threading.Monitor.Wait(obj, ms)); - public static bool MonitorWait(object obj, global::System.TimeSpan timeout) => Sliced(Ms(timeout), ms => global::System.Threading.Monitor.Wait(obj, ms)); + // A slice that times out leaves the wait queue until it holds the lock again: a Pulse in that gap would be lost (the + // pulsing thread may take the lock first). So the agent's Pulse / PulseAll also count what they owe the sliced waiters + // of that object (all under its lock), and a waiter between two slices takes what it is owed as its wake-up. + sealed class Waiting + { + public int Waiters, Owed; + } + + static readonly global::System.Runtime.CompilerServices.ConditionalWeakTable _waiting = + new global::System.Runtime.CompilerServices.ConditionalWeakTable(); + + public static bool MonitorWait(object obj) => MonitorWait(obj, -1); + public static bool MonitorWait(object obj, global::System.TimeSpan timeout) => MonitorWait(obj, Ms(timeout)); + public static bool MonitorWait(object obj, int millisecondsTimeout) + { + if (obj == null || !global::System.Threading.Monitor.IsEntered(obj)) + return global::System.Threading.Monitor.Wait(obj, millisecondsTimeout); // the framework's own exception + var waiting = _waiting.GetValue(obj, _ => new Waiting()); + waiting.Waiters++; + try + { + return Sliced(millisecondsTimeout, ms => + { + var pulsed = global::System.Threading.Monitor.Wait(obj, ms); + if (waiting.Owed > 0) + { + waiting.Owed--; + return true; // pulsed in the queue, or between two slices + } + return pulsed; + }); + } + finally + { + // Whatever ends the wait, the lock is held again here. + if (--waiting.Waiters == 0) + _waiting.Remove(obj); // no entry outlives the wait: it would keep the snippet's code loaded + else if (waiting.Owed > waiting.Waiters) + waiting.Owed = waiting.Waiters; + } + } + + public static void MonitorPulse(object obj) + { + global::System.Threading.Monitor.Pulse(obj); // throws, as written, when this thread does not hold the lock + Waiting waiting; + if (_waiting.TryGetValue(obj, out waiting) && waiting.Owed < waiting.Waiters) + waiting.Owed++; + } + + public static void MonitorPulseAll(object obj) + { + global::System.Threading.Monitor.PulseAll(obj); + Waiting waiting; + if (_waiting.TryGetValue(obj, out waiting)) + waiting.Owed = waiting.Waiters; + } public static bool WaitOne(global::System.Threading.WaitHandle handle) => Sliced(-1, ms => handle.WaitOne(ms)); public static bool WaitOne(global::System.Threading.WaitHandle handle, int millisecondsTimeout) => Sliced(millisecondsTimeout, ms => handle.WaitOne(ms)); diff --git a/spec/contract.md b/spec/contract.md index 1570977..5abb280 100644 --- a/spec/contract.md +++ b/spec/contract.md @@ -155,7 +155,9 @@ the host turns them off (`CodeRunnerOptions.ExecutionGuards`): `SemaphoreSlim.Wait`, `ManualResetEventSlim.Wait`, `CountdownEvent.Wait`, `Monitor.Wait`, `WaitHandle.WaitOne` / `WaitAll` / `WaitAny`, in their forms without a timeout and with one (`int`, `TimeSpan`, with or without a `CancellationToken`; .NET 9+ `Task.WaitAll(IEnumerable)`), and `task.ConfigureAwait(b).GetAwaiter().GetResult()`. - Each becomes the guard's stand-in of the same parameters; values, exceptions, timeouts and wake-ups stay the same, a + Each becomes the guard's stand-in of the same parameters; values, exceptions, timeouts and wake-ups stay the same + (`Monitor.Pulse` / `PulseAll` in the agent's code become stand-ins too, so a pulse sent while a sliced `Monitor.Wait` + is between two slices still wakes it; two waiters can then both wake from one `Pulse`, as from a spurious wake-up), a wait that succeeded is never followed by a stop (it may have acquired a semaphore or a mutex), and a wait of at most 50 ms is the framework's own, as is a wait without bound where nothing could stop it (a run without budget, not work of a run). `Task.Wait` / `Task.WaitAll` without a timeout and without a cancelable token, `Result` and `GetResult` From 61fd697da23cc90f06c93bfd72d104ab2e209aa7 Mon Sep 17 00:00:00 2001 From: Ivan Murzak Date: Fri, 6 Nov 2026 02:10:50 -0800 Subject: [PATCH 2/4] fix(csharp): the guard keeps no thread-static in the collectible snippet assembly .NET 10 test runs crashed now and then in the GC: a segfault in CFinalize::ScanForFinalization on an object with a null method table (crash dump from CI, Roslyn 4.0.1 line, symbolized). The guard's run tracking used a [ThreadStatic] of a snippet type plus generic instantiations over snippet types (AsyncLocal, ConditionalWeakTable) inside the collectible assembly; dead threads keep thread-static storage of an unloaded context. They are now ThreadLocal / AsyncLocal / ConditionalWeakTable, framework instantiations that are not collectible. Contract changelog notes the Monitor fix. Co-Authored-By: Claude Opus 5.5 --- dotnet/CodeExec/ExecutionGuard.cs | 37 +++++++++++++++++-------------- spec/contract.md | 2 ++ 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/dotnet/CodeExec/ExecutionGuard.cs b/dotnet/CodeExec/ExecutionGuard.cs index 491d0fd..51e26b9 100644 --- a/dotnet/CodeExec/ExecutionGuard.cs +++ b/dotnet/CodeExec/ExecutionGuard.cs @@ -80,7 +80,7 @@ public static class ExecutionGuard /// 10 ms apart, back to every iteration when they are not: slow iterations overshoot the budget by about 20 ms, /// fast ones pay one clock read per 256 iterations. The statics are shared by all threads: a race only shifts a /// check. Work handed to a thread or task start (, ) carries its run: a - /// thread-static while it runs synchronously, an AsyncLocal for asynchronous task work (it flows into the + /// thread-local while it runs synchronously, an AsyncLocal for asynchronous task work (it flows into the /// work's awaits); the tasks and threads it starts in turn belong to the same run. Once a run that started work has /// ended, every clock check reads it, and work of an ended run stops there. A thread entry swallows the stop: an /// exception left unhandled on a thread would end the process. @@ -94,8 +94,11 @@ internal static class __CodeExecGuard static Run _current; // The run of the work running: synchronous work on this thread, and asynchronous task work (it flows into the work's // awaits). Nothing else carries it: a timer, an async void continuation or a thread started by work keeps none of it. - [global::System.ThreadStatic] static Run _thread; - static readonly global::System.Threading.AsyncLocal _async = new global::System.Threading.AsyncLocal(); + // Held by framework types instantiated over object, not by a thread-static or a generic instantiation over a type of + // this (collectible) assembly: with the thread-static, .NET 10 test runs crashed now and then in the GC (a null method + // table while it scanned for finalization), which points at what dead threads kept of an unloaded snippet's statics. + static readonly global::System.Threading.ThreadLocal _thread = new global::System.Threading.ThreadLocal(); + static readonly global::System.Threading.AsyncLocal _async = new global::System.Threading.AsyncLocal(); /// One run: whether it has ended, whether it started work, and how much of that work has not finished. public sealed class Run @@ -141,7 +144,7 @@ public static bool Init() public static bool Running() => _stopped == 0 && (_orphans == 0 || (_live == 0 && _asyncOrphans == 0) || !Orphaned()); // The AsyncLocal is slow to read: only once asynchronous task work has ever run. - static Run Current() => _thread ?? (_asyncWork != 0 ? _async.Value : null); + static Run Current() => (Run)_thread.Value ?? (_asyncWork != 0 ? (Run)_async.Value : null); static bool Orphaned() { @@ -184,16 +187,16 @@ static void Done(Run run) static void Do(Run run, global::System.Action work, bool swallow) { Started(); - var thread = _thread; + var thread = _thread.Value; var flowing = _asyncWork != 0 ? _async.Value : null; - _thread = run; + _thread.Value = run; if (flowing != null) _async.Value = null; // what the work starts without a wrapper (a timer, an async void continuation) is not the run's try { work(); } catch (global::System.Exception) when (swallow && !Running()) { } finally { - _thread = thread; + _thread.Value = thread; if (flowing != null) _async.Value = flowing; Done(run); @@ -214,9 +217,9 @@ static TResult Get(Run run, global::System.Func work) if (global::System.Threading.Volatile.Read(ref run.Ended) != 0) _asyncOrphans = 1; Started(); - var thread = _thread; + var thread = _thread.Value; var flowing = _async.Value; - _thread = null; + _thread.Value = null; _async.Value = run; try { @@ -235,7 +238,7 @@ static TResult Get(Run run, global::System.Func work) } finally { - _thread = thread; + _thread.Value = thread; _async.Value = flowing; } } @@ -414,8 +417,8 @@ sealed class Waiting public int Waiters, Owed; } - static readonly global::System.Runtime.CompilerServices.ConditionalWeakTable _waiting = - new global::System.Runtime.CompilerServices.ConditionalWeakTable(); + static readonly global::System.Runtime.CompilerServices.ConditionalWeakTable _waiting = + new global::System.Runtime.CompilerServices.ConditionalWeakTable(); // over object: see _thread public static bool MonitorWait(object obj) => MonitorWait(obj, -1); public static bool MonitorWait(object obj, global::System.TimeSpan timeout) => MonitorWait(obj, Ms(timeout)); @@ -423,7 +426,7 @@ public static bool MonitorWait(object obj, int millisecondsTimeout) { if (obj == null || !global::System.Threading.Monitor.IsEntered(obj)) return global::System.Threading.Monitor.Wait(obj, millisecondsTimeout); // the framework's own exception - var waiting = _waiting.GetValue(obj, _ => new Waiting()); + var waiting = (Waiting)_waiting.GetValue(obj, _ => new Waiting()); waiting.Waiters++; try { @@ -451,16 +454,16 @@ public static bool MonitorWait(object obj, int millisecondsTimeout) public static void MonitorPulse(object obj) { global::System.Threading.Monitor.Pulse(obj); // throws, as written, when this thread does not hold the lock - Waiting waiting; - if (_waiting.TryGetValue(obj, out waiting) && waiting.Owed < waiting.Waiters) + object entry; + if (_waiting.TryGetValue(obj, out entry) && entry is Waiting waiting && waiting.Owed < waiting.Waiters) waiting.Owed++; } public static void MonitorPulseAll(object obj) { global::System.Threading.Monitor.PulseAll(obj); - Waiting waiting; - if (_waiting.TryGetValue(obj, out waiting)) + object entry; + if (_waiting.TryGetValue(obj, out entry) && entry is Waiting waiting) waiting.Owed = waiting.Waiters; } diff --git a/spec/contract.md b/spec/contract.md index 5abb280..4330c36 100644 --- a/spec/contract.md +++ b/spec/contract.md @@ -303,6 +303,8 @@ Lengths are in characters: UTF-16 code units in C#, code points in Python. A lim ## Changelog +- **Unreleased** — C#: a `Monitor.Pulse` sent while a sliced `Monitor.Wait` is between two slices wakes it (it was + lost, and the waiter slept until the budget); the agent's `Monitor.Pulse` / `PulseAll` become guard stand-ins (§3.6). - **v0.4.0** — execution guards (§3.6): C# snippets, full source and includes stop with `Runtime` (`Recursion too deep`) before a stack overflow and with `Timeout` once `timeoutMs` is spent, also in synchronous code on the main thread; Python stops with `Timeout` through a watchdog (§4). `timeoutMs` applies to both runtimes; `0` From e2ce285900af4b2db8737772e4dd231b4517356e Mon Sep 17 00:00:00 2001 From: Ivan Murzak Date: Fri, 6 Nov 2026 02:18:59 -0800 Subject: [PATCH 3/4] fix(csharp): one Pulse wakes one sliced waiter (review) The owed-wake-up count is now the only signal: while sliced waiters exist a Pulse becomes PulseAll plus one owed wake-up, the first waiter to hold the lock takes it and the others wait on. Before, the native pulse and the count could each wake a different waiter. Contract notes the limits (waits and pulses outside the guarded code). New test: two Pulses, three waiters. Co-Authored-By: Claude Opus 5.5 --- dotnet/CodeExec.Tests/GuardTests.cs | 14 +++++++++ dotnet/CodeExec/ExecutionGuard.cs | 49 ++++++++++++++++++++--------- spec/contract.md | 3 +- 3 files changed, 50 insertions(+), 16 deletions(-) diff --git a/dotnet/CodeExec.Tests/GuardTests.cs b/dotnet/CodeExec.Tests/GuardTests.cs index 7e37bdc..4c593e5 100644 --- a/dotnet/CodeExec.Tests/GuardTests.cs +++ b/dotnet/CodeExec.Tests/GuardTests.cs @@ -605,6 +605,20 @@ public void APulseBetweenTwoSlicesIsNotLost(string pulse, string woken) Assert.Equal(woken, result.Value); } + // One Pulse wakes exactly one sliced waiter, also when the others' slices time out around it. + [Fact] + public void OnePulseWakesOneWaiter() + { + const string code = "var o = new object();\nvar woken = 0;\nvar entered = new System.Threading.CountdownEvent(3);\n" + + "var threads = new System.Collections.Generic.List();\n" + + "for (var i = 0; i < 3; i++) { var t = new System.Threading.Thread(() => { lock (o) { entered.Signal(); if (System.Threading.Monitor.Wait(o, 1500)) woken++; } }); t.Start(); threads.Add(t); }\n" + + "entered.Wait();\nfor (var k = 0; k < 2; k++) { lock (o) { System.Threading.Thread.Sleep(70); System.Threading.Monitor.Pulse(o); } System.Threading.Thread.Sleep(30); }\n" + + "foreach (var t in threads) t.Join();\nwoken"; + var result = Fixtures.Runner().Execute(new ExecuteRequest { Code = code, TimeoutMs = 20_000 }); + Assert.True(result.Success, result.Error + " " + string.Join(" | ", result.Diagnostics)); + Assert.Equal("2", result.Value); + } + [Fact] public void PulseWithoutTheLockStillThrows() { diff --git a/dotnet/CodeExec/ExecutionGuard.cs b/dotnet/CodeExec/ExecutionGuard.cs index 51e26b9..8e61309 100644 --- a/dotnet/CodeExec/ExecutionGuard.cs +++ b/dotnet/CodeExec/ExecutionGuard.cs @@ -410,8 +410,9 @@ public static T Ready(T task) where T : global::System.Threading.Tasks.Task public static bool CountdownWait(global::System.Threading.CountdownEvent waitable, int millisecondsTimeout, global::System.Threading.CancellationToken cancellationToken) => Sliced(millisecondsTimeout, ms => waitable.Wait(ms, cancellationToken)); // A slice that times out leaves the wait queue until it holds the lock again: a Pulse in that gap would be lost (the - // pulsing thread may take the lock first). So the agent's Pulse / PulseAll also count what they owe the sliced waiters - // of that object (all under its lock), and a waiter between two slices takes what it is owed as its wake-up. + // pulsing thread may take the lock first). So the agent's Pulse / PulseAll count what they owe the object's sliced + // waiters (all under its lock), and that count alone decides who wakes: a Pulse wakes them all (PulseAll), the first + // to hold the lock takes the one wake-up, the others wait on. One Pulse still wakes one waiter, none is lost. sealed class Waiting { public int Waiters, Owed; @@ -424,22 +425,13 @@ sealed class Waiting public static bool MonitorWait(object obj, global::System.TimeSpan timeout) => MonitorWait(obj, Ms(timeout)); public static bool MonitorWait(object obj, int millisecondsTimeout) { - if (obj == null || !global::System.Threading.Monitor.IsEntered(obj)) + if (obj == null || millisecondsTimeout < -1 || !global::System.Threading.Monitor.IsEntered(obj)) return global::System.Threading.Monitor.Wait(obj, millisecondsTimeout); // the framework's own exception var waiting = (Waiting)_waiting.GetValue(obj, _ => new Waiting()); waiting.Waiters++; try { - return Sliced(millisecondsTimeout, ms => - { - var pulsed = global::System.Threading.Monitor.Wait(obj, ms); - if (waiting.Owed > 0) - { - waiting.Owed--; - return true; // pulsed in the queue, or between two slices - } - return pulsed; - }); + return Sliced(millisecondsTimeout, ms => TakeWakeUp(obj, waiting, ms)); } finally { @@ -451,12 +443,39 @@ public static bool MonitorWait(object obj, int millisecondsTimeout) } } + // Waits up to ms (-1: without bound) for a wake-up owed to this object's waiters. + static bool TakeWakeUp(object obj, Waiting waiting, int ms) + { + var start = global::System.Diagnostics.Stopwatch.GetTimestamp(); + for (;;) + { + var left = ms; + if (ms > 0) + { + var elapsed = (global::System.Diagnostics.Stopwatch.GetTimestamp() - start) * 1000 / global::System.Diagnostics.Stopwatch.Frequency; + left = elapsed >= ms ? 0 : (int)(ms - elapsed); + } + global::System.Threading.Monitor.Wait(obj, left); + if (waiting.Owed > 0) + { + waiting.Owed--; + return true; + } + if (left == 0) + return false; + } + } + public static void MonitorPulse(object obj) { - global::System.Threading.Monitor.Pulse(obj); // throws, as written, when this thread does not hold the lock object entry; - if (_waiting.TryGetValue(obj, out entry) && entry is Waiting waiting && waiting.Owed < waiting.Waiters) + if (obj != null && _waiting.TryGetValue(obj, out entry) && entry is Waiting waiting && waiting.Owed < waiting.Waiters) + { + global::System.Threading.Monitor.PulseAll(obj); // throws, as Pulse does, when this thread does not hold the lock waiting.Owed++; + } + else + global::System.Threading.Monitor.Pulse(obj); } public static void MonitorPulseAll(object obj) diff --git a/spec/contract.md b/spec/contract.md index 4330c36..87fb0eb 100644 --- a/spec/contract.md +++ b/spec/contract.md @@ -157,7 +157,8 @@ the host turns them off (`CodeRunnerOptions.ExecutionGuards`): `CancellationToken`; .NET 9+ `Task.WaitAll(IEnumerable)`), and `task.ConfigureAwait(b).GetAwaiter().GetResult()`. Each becomes the guard's stand-in of the same parameters; values, exceptions, timeouts and wake-ups stay the same (`Monitor.Pulse` / `PulseAll` in the agent's code become stand-ins too, so a pulse sent while a sliced `Monitor.Wait` - is between two slices still wakes it; two waiters can then both wake from one `Pulse`, as from a spurious wake-up), a + is between two slices still wakes it, and one `Pulse` wakes one such waiter; a `Monitor.Wait` on the same object + outside the agent's guarded code may then wake spuriously, and a pulse sent from such code can still be missed), a wait that succeeded is never followed by a stop (it may have acquired a semaphore or a mutex), and a wait of at most 50 ms is the framework's own, as is a wait without bound where nothing could stop it (a run without budget, not work of a run). `Task.Wait` / `Task.WaitAll` without a timeout and without a cancelable token, `Result` and `GetResult` From 1ceed148780d2a8040218d4319c88d532eb14dd2 Mon Sep 17 00:00:00 2001 From: Ivan Murzak Date: Fri, 6 Nov 2026 02:25:48 -0800 Subject: [PATCH 4/4] refactor(csharp): simplify the Pulse stand-ins and the thread-local (simplify) Plain Pulse again (only the owed count decides who wakes, so no PulseAll and no extra wake-ups), one ElapsedMs helper for Sliced and TakeWakeUp, one WaitingOn lookup; AsyncLocal and the typed table back (only the thread-local needed to move off collectible thread-statics); Current() reads IsValueCreated first so a read allocates nothing; the comment states the crash cause as the suspect it is. Contract: the Pulse note is a sentence of its own. Monitor tests folded into GuardedCodeKeepsItsMeaning rows. Co-Authored-By: Claude Opus 5.5 --- dotnet/CodeExec.Tests/GuardTests.cs | 46 +++------------------ dotnet/CodeExec/ExecutionGuard.cs | 64 +++++++++++++++-------------- spec/contract.md | 10 ++--- 3 files changed, 44 insertions(+), 76 deletions(-) diff --git a/dotnet/CodeExec.Tests/GuardTests.cs b/dotnet/CodeExec.Tests/GuardTests.cs index 4c593e5..7ee87a1 100644 --- a/dotnet/CodeExec.Tests/GuardTests.cs +++ b/dotnet/CodeExec.Tests/GuardTests.cs @@ -573,6 +573,12 @@ public void GuardsCanBeTurnedOff() [InlineData("Task.WaitAll(new[] { Task.Delay(10_000) }, 120)", "False")] [InlineData("System.Threading.WaitHandle.WaitAny(new System.Threading.WaitHandle[] { new System.Threading.ManualResetEvent(false) }, 120)", "258")] [InlineData("var o = new object();\nvar t = new System.Threading.Thread(() => { System.Threading.Thread.Sleep(200); lock (o) System.Threading.Monitor.Pulse(o); });\nbool got;\nlock (o) { t.Start(); got = System.Threading.Monitor.Wait(o); }\nt.Join();\ngot", "True")] + // A Pulse while a sliced waiter is between two slices (the pulsing thread holds the lock longer than a slice) still + // wakes it, and one Pulse wakes one waiter. + [InlineData("var o = new object();\nvar woken = 0;\nvar entered = new System.Threading.CountdownEvent(2);\nvar all = new System.Collections.Generic.List();\nfor (var i = 0; i < 2; i++) { var t = new System.Threading.Thread(() => { lock (o) { entered.Signal(); if (System.Threading.Monitor.Wait(o, 2000)) woken++; } }); t.Start(); all.Add(t); }\nentered.Wait();\nlock (o) { System.Threading.Thread.Sleep(200); System.Threading.Monitor.Pulse(o); }\nforeach (var t in all) t.Join();\nwoken", "1")] + [InlineData("var o = new object();\nvar woken = 0;\nvar entered = new System.Threading.CountdownEvent(2);\nvar all = new System.Collections.Generic.List();\nfor (var i = 0; i < 2; i++) { var t = new System.Threading.Thread(() => { lock (o) { entered.Signal(); if (System.Threading.Monitor.Wait(o, 2000)) woken++; } }); t.Start(); all.Add(t); }\nentered.Wait();\nlock (o) { System.Threading.Thread.Sleep(200); System.Threading.Monitor.PulseAll(o); }\nforeach (var t in all) t.Join();\nwoken", "2")] + [InlineData("var o = new object();\nvar woken = 0;\nvar entered = new System.Threading.CountdownEvent(3);\nvar all = new System.Collections.Generic.List();\nfor (var i = 0; i < 3; i++) { var t = new System.Threading.Thread(() => { lock (o) { entered.Signal(); if (System.Threading.Monitor.Wait(o, 1500)) woken++; } }); t.Start(); all.Add(t); }\nentered.Wait();\nfor (var k = 0; k < 2; k++) { lock (o) { System.Threading.Thread.Sleep(70); System.Threading.Monitor.Pulse(o); } System.Threading.Thread.Sleep(30); }\nforeach (var t in all) t.Join();\nwoken", "2")] + [InlineData("try { System.Threading.Monitor.Pulse(new object()); } catch (System.Threading.SynchronizationLockException) { return \"sync\"; }\nreturn \"\";", "sync")] [InlineData("var e = new System.Threading.ManualResetEventSlim();\nTask.Run(() => { System.Threading.Thread.Sleep(150); e.Set(); });\ne.Wait();\ne.IsSet", "True")] [InlineData("Task.Run(async () => { await Task.Delay(150); return 4; }).GetAwaiter().GetResult()", "4")] [InlineData("try { Task.Run(() => { System.Threading.Thread.Sleep(100); throw new InvalidOperationException(\"r\"); return 1; }).GetAwaiter().GetResult(); } catch (InvalidOperationException e) { return e.Message; }\nreturn \"\";", "r")] @@ -587,46 +593,6 @@ public async Task GuardedCodeKeepsItsMeaning(string code, string expected) Assert.Equal(expected, result.Value); } - // A sliced Monitor.Wait leaves the wait queue each time a slice times out, until it holds the lock again: a Pulse in - // that gap must still wake it. Here the pulsing thread holds the lock for longer than a slice, so the waiter's slice - // always times out before the Pulse (without the pulse count the waiter sleeps until the budget). - [Theory] - [InlineData("System.Threading.Monitor.Pulse(o);", "1")] - [InlineData("System.Threading.Monitor.PulseAll(o);", "2")] - public void APulseBetweenTwoSlicesIsNotLost(string pulse, string woken) - { - var code = "var o = new object();\nvar woken = 0;\nvar entered = new System.Threading.CountdownEvent(2);\n" - + "System.Threading.Thread Waiter() { var t = new System.Threading.Thread(() => { lock (o) { entered.Signal(); if (System.Threading.Monitor.Wait(o, 3000)) woken++; } }); t.Start(); return t; }\n" - + "var a = Waiter();\nvar b = Waiter();\nentered.Wait();\n" - + "lock (o) { System.Threading.Thread.Sleep(200); " + pulse + " }\n" - + "a.Join();\nb.Join();\nwoken"; - var result = Fixtures.Runner().Execute(new ExecuteRequest { Code = code, TimeoutMs = 20_000 }); - Assert.True(result.Success, result.Error + " " + string.Join(" | ", result.Diagnostics)); - Assert.Equal(woken, result.Value); - } - - // One Pulse wakes exactly one sliced waiter, also when the others' slices time out around it. - [Fact] - public void OnePulseWakesOneWaiter() - { - const string code = "var o = new object();\nvar woken = 0;\nvar entered = new System.Threading.CountdownEvent(3);\n" - + "var threads = new System.Collections.Generic.List();\n" - + "for (var i = 0; i < 3; i++) { var t = new System.Threading.Thread(() => { lock (o) { entered.Signal(); if (System.Threading.Monitor.Wait(o, 1500)) woken++; } }); t.Start(); threads.Add(t); }\n" - + "entered.Wait();\nfor (var k = 0; k < 2; k++) { lock (o) { System.Threading.Thread.Sleep(70); System.Threading.Monitor.Pulse(o); } System.Threading.Thread.Sleep(30); }\n" - + "foreach (var t in threads) t.Join();\nwoken"; - var result = Fixtures.Runner().Execute(new ExecuteRequest { Code = code, TimeoutMs = 20_000 }); - Assert.True(result.Success, result.Error + " " + string.Join(" | ", result.Diagnostics)); - Assert.Equal("2", result.Value); - } - - [Fact] - public void PulseWithoutTheLockStillThrows() - { - var result = Fixtures.Runner().Execute(new ExecuteRequest { Code = "try { System.Threading.Monitor.Pulse(new object()); } catch (System.Threading.SynchronizationLockException) { return \"sync\"; }\nreturn \"\";" }); - Assert.True(result.Success, result.Error); - Assert.Equal("sync", result.Value); - } - // Inserted guard calls sit on lines of their own: the agent's lines AND columns stay. [Theory] [InlineData("while (false) { } int x = undefinedThing;", 1, 27)] diff --git a/dotnet/CodeExec/ExecutionGuard.cs b/dotnet/CodeExec/ExecutionGuard.cs index 8e61309..59ef692 100644 --- a/dotnet/CodeExec/ExecutionGuard.cs +++ b/dotnet/CodeExec/ExecutionGuard.cs @@ -94,11 +94,12 @@ internal static class __CodeExecGuard static Run _current; // The run of the work running: synchronous work on this thread, and asynchronous task work (it flows into the work's // awaits). Nothing else carries it: a timer, an async void continuation or a thread started by work keeps none of it. - // Held by framework types instantiated over object, not by a thread-static or a generic instantiation over a type of - // this (collectible) assembly: with the thread-static, .NET 10 test runs crashed now and then in the GC (a null method - // table while it scanned for finalization), which points at what dead threads kept of an unloaded snippet's statics. + // A ThreadLocal over object, not a thread-static: this assembly is collectible, and with the thread-static .NET 10 + // test runs crashed now and then in the GC (a null method table while it scanned for finalization). The suspect is + // the runtime's thread-static storage of an unloaded assembly on threads that outlive it; ThreadLocal keeps + // its storage in the framework (ThreadLocal would have thread-statics of a collectible instantiation again). static readonly global::System.Threading.ThreadLocal _thread = new global::System.Threading.ThreadLocal(); - static readonly global::System.Threading.AsyncLocal _async = new global::System.Threading.AsyncLocal(); + static readonly global::System.Threading.AsyncLocal _async = new global::System.Threading.AsyncLocal(); /// One run: whether it has ended, whether it started work, and how much of that work has not finished. public sealed class Run @@ -144,7 +145,8 @@ public static bool Init() public static bool Running() => _stopped == 0 && (_orphans == 0 || (_live == 0 && _asyncOrphans == 0) || !Orphaned()); // The AsyncLocal is slow to read: only once asynchronous task work has ever run. - static Run Current() => (Run)_thread.Value ?? (_asyncWork != 0 ? (Run)_async.Value : null); + // IsValueCreated first: reading Value on a thread that never set it would allocate its storage there. + static Run Current() => (_thread.IsValueCreated ? (Run)_thread.Value : null) ?? (_asyncWork != 0 ? _async.Value : null); static bool Orphaned() { @@ -187,7 +189,7 @@ static void Done(Run run) static void Do(Run run, global::System.Action work, bool swallow) { Started(); - var thread = _thread.Value; + var thread = _thread.IsValueCreated ? _thread.Value : null; var flowing = _asyncWork != 0 ? _async.Value : null; _thread.Value = run; if (flowing != null) @@ -217,7 +219,7 @@ static TResult Get(Run run, global::System.Func work) if (global::System.Threading.Volatile.Read(ref run.Ended) != 0) _asyncOrphans = 1; Started(); - var thread = _thread.Value; + var thread = _thread.IsValueCreated ? _thread.Value : null; var flowing = _async.Value; _thread.Value = null; _async.Value = run; @@ -284,8 +286,7 @@ static bool Sliced(int timeout, global::System.Func wait) { if (timeout > 0) { - var elapsed = (global::System.Diagnostics.Stopwatch.GetTimestamp() - start) * 1000 / global::System.Diagnostics.Stopwatch.Frequency; - var left = timeout - elapsed; + var left = timeout - ElapsedMs(start); if (left <= SliceMs) return wait(left > 0 ? (int)left : 0); } @@ -295,6 +296,9 @@ static bool Sliced(int timeout, global::System.Func wait) } } + static long ElapsedMs(long start) => + (global::System.Diagnostics.Stopwatch.GetTimestamp() - start) * 1000 / global::System.Diagnostics.Stopwatch.Frequency; + /// A TimeSpan timeout in ms; -2 when out of range (the framework's own wait throws for it). static int Ms(global::System.TimeSpan timeout) { @@ -410,16 +414,22 @@ public static T Ready(T task) where T : global::System.Threading.Tasks.Task public static bool CountdownWait(global::System.Threading.CountdownEvent waitable, int millisecondsTimeout, global::System.Threading.CancellationToken cancellationToken) => Sliced(millisecondsTimeout, ms => waitable.Wait(ms, cancellationToken)); // A slice that times out leaves the wait queue until it holds the lock again: a Pulse in that gap would be lost (the - // pulsing thread may take the lock first). So the agent's Pulse / PulseAll count what they owe the object's sliced - // waiters (all under its lock), and that count alone decides who wakes: a Pulse wakes them all (PulseAll), the first - // to hold the lock takes the one wake-up, the others wait on. One Pulse still wakes one waiter, none is lost. + // pulsing thread may take the lock first). So the agent's Pulse / PulseAll also count the wake-ups they owe the + // object's sliced waiters (under its lock), and only that count wakes one: a waiter woken by the Pulse, or one between + // two slices, takes it once it holds the lock again; whoever finds none waits on. One Pulse wakes one waiter. sealed class Waiting { public int Waiters, Owed; } - static readonly global::System.Runtime.CompilerServices.ConditionalWeakTable _waiting = - new global::System.Runtime.CompilerServices.ConditionalWeakTable(); // over object: see _thread + static readonly global::System.Runtime.CompilerServices.ConditionalWeakTable _waiting = + new global::System.Runtime.CompilerServices.ConditionalWeakTable(); + + static Waiting WaitingOn(object obj) + { + Waiting waiting; + return obj != null && _waiting.TryGetValue(obj, out waiting) ? waiting : null; + } public static bool MonitorWait(object obj) => MonitorWait(obj, -1); public static bool MonitorWait(object obj, global::System.TimeSpan timeout) => MonitorWait(obj, Ms(timeout)); @@ -427,7 +437,7 @@ public static bool MonitorWait(object obj, int millisecondsTimeout) { if (obj == null || millisecondsTimeout < -1 || !global::System.Threading.Monitor.IsEntered(obj)) return global::System.Threading.Monitor.Wait(obj, millisecondsTimeout); // the framework's own exception - var waiting = (Waiting)_waiting.GetValue(obj, _ => new Waiting()); + var waiting = _waiting.GetValue(obj, _ => new Waiting()); waiting.Waiters++; try { @@ -443,18 +453,14 @@ public static bool MonitorWait(object obj, int millisecondsTimeout) } } - // Waits up to ms (-1: without bound) for a wake-up owed to this object's waiters. + // Waits up to ms (-1: without bound) for a wake-up owed to the object's waiters. Waking without one (it went to + // another waiter) waits on for the time left, so a short or last slice does not end early. static bool TakeWakeUp(object obj, Waiting waiting, int ms) { var start = global::System.Diagnostics.Stopwatch.GetTimestamp(); for (;;) { - var left = ms; - if (ms > 0) - { - var elapsed = (global::System.Diagnostics.Stopwatch.GetTimestamp() - start) * 1000 / global::System.Diagnostics.Stopwatch.Frequency; - left = elapsed >= ms ? 0 : (int)(ms - elapsed); - } + var left = ms <= 0 ? ms : (int)global::System.Math.Max(0, ms - ElapsedMs(start)); global::System.Threading.Monitor.Wait(obj, left); if (waiting.Owed > 0) { @@ -468,21 +474,17 @@ static bool TakeWakeUp(object obj, Waiting waiting, int ms) public static void MonitorPulse(object obj) { - object entry; - if (obj != null && _waiting.TryGetValue(obj, out entry) && entry is Waiting waiting && waiting.Owed < waiting.Waiters) - { - global::System.Threading.Monitor.PulseAll(obj); // throws, as Pulse does, when this thread does not hold the lock + global::System.Threading.Monitor.Pulse(obj); // throws, as written, when this thread does not hold the lock + var waiting = WaitingOn(obj); + if (waiting != null && waiting.Owed < waiting.Waiters) waiting.Owed++; - } - else - global::System.Threading.Monitor.Pulse(obj); } public static void MonitorPulseAll(object obj) { global::System.Threading.Monitor.PulseAll(obj); - object entry; - if (_waiting.TryGetValue(obj, out entry) && entry is Waiting waiting) + var waiting = WaitingOn(obj); + if (waiting != null) waiting.Owed = waiting.Waiters; } diff --git a/spec/contract.md b/spec/contract.md index 87fb0eb..8ce15d9 100644 --- a/spec/contract.md +++ b/spec/contract.md @@ -155,10 +155,7 @@ the host turns them off (`CodeRunnerOptions.ExecutionGuards`): `SemaphoreSlim.Wait`, `ManualResetEventSlim.Wait`, `CountdownEvent.Wait`, `Monitor.Wait`, `WaitHandle.WaitOne` / `WaitAll` / `WaitAny`, in their forms without a timeout and with one (`int`, `TimeSpan`, with or without a `CancellationToken`; .NET 9+ `Task.WaitAll(IEnumerable)`), and `task.ConfigureAwait(b).GetAwaiter().GetResult()`. - Each becomes the guard's stand-in of the same parameters; values, exceptions, timeouts and wake-ups stay the same - (`Monitor.Pulse` / `PulseAll` in the agent's code become stand-ins too, so a pulse sent while a sliced `Monitor.Wait` - is between two slices still wakes it, and one `Pulse` wakes one such waiter; a `Monitor.Wait` on the same object - outside the agent's guarded code may then wake spuriously, and a pulse sent from such code can still be missed), a + Each becomes the guard's stand-in of the same parameters; values, exceptions, timeouts and wake-ups stay the same, a wait that succeeded is never followed by a stop (it may have acquired a semaphore or a mutex), and a wait of at most 50 ms is the framework's own, as is a wait without bound where nothing could stop it (a run without budget, not work of a run). `Task.Wait` / `Task.WaitAll` without a timeout and without a cancelable token, `Result` and `GetResult` @@ -167,7 +164,10 @@ the host turns them off (`CodeRunnerOptions.ExecutionGuards`): context's thread runs there and returns its value. A task the scheduler declines, one with no body to run (an async method's, whose continuation waits for the blocked thread), a wait on a thread-pool thread, and every wait on a runtime without the framework's inline path are waited for in slices and stop at the budget. A body run inline that - is not the snippet's own code is stopped only when it returns. Not covered (stopped + is not the snippet's own code is stopped only when it returns. `Monitor.Pulse` / `PulseAll` in the agent's code become + stand-ins too: a pulse sent while a sliced `Monitor.Wait` is between two slices still wakes it, and one `Pulse` wakes + one waiter (a pulse sent from code the guards do not rewrite can still be missed by a waiter between two slices). + Not covered (stopped only when the call returns): such a wait with named arguments, inside a conditional access (`a?.Task.Result`), as a property pattern path (`x is { Task.Result: 1 }`), through `base.`, a method group or a delegate; .NET 9+ `Task.WaitAll` given a span; `lock` / `Monitor.Enter` and other waits (`BlockingCollection.Take`, `Barrier`,