fix(advancedsearch): clear pause and sample exit before signaling the drain (#2549)
## Summary `AdvancedSearchThreadWorker.Execute` signals `_stopEvent` **before** clearing `_pause` and **before** reading the exit condition. `Sleep()` unblocks the instant that signal fires, so the owning thread can begin the next cycle while the worker is still finishing the previous one — and the worker's two trailing operations then land on the new cycle's state. `SerializationThreadWorker` already orders the same handshake correctly and documents why (`Projects/Server/Serialization/SerializationThreadWorker.cs`): ```csharp // The owning thread may start another pause cycle the moment _stopEvent is set // (Exit does exactly that). Clear _pause and sample the exit condition before // signaling, or the new cycle's pause request is clobbered / its Sleep orphaned. var exiting = Core.Closing || worker._exit; Volatile.Write(ref worker._pause, false); worker._stopEvent.Set(); ``` This applies the same ordering to the search worker. Three lines; no behavior change on the happy path. ## The two failures **Reuse hang.** The next cycle's `Wake`/`Push`/`Sleep` writes `_pause = true`, then the worker's stale `_pause = false` lands on top of it. The inner loop never observes `pauseRequested`, its queue is already empty, and it spins on `Thread.Yield()` forever — so the owning thread's next `Sleep()` waits on a `_stopEvent` that is never set again. A single search wakes each worker exactly once, so this only surfaces once `_threadWorkers` is reused by a later search. **Orphaned `Exit()`.** `Exit()` sets `_exit`, `Wake()`s, then `Sleep()`s — the moment the drain's `Sleep()` returns. Reading `_exit` *after* the signal, the worker can observe that fresh `_exit`, return without ever consuming the `Wake`, and leave `Exit()`'s `Sleep()` waiting on a signal nobody will send. The `_thread.IsAlive` guard doesn't close this: the thread passes the check and returns immediately after. ## Verification Verified with two throwaway timing tests — 25k reuse cycles and 2k drain-then-`Exit` cycles, each under a bounded wait: | ordering | result | |---|---| | previous | `Failed: 2, Passed: 3` — both reproduce, cleanly at the 20s bound | | this PR | 3 consecutive runs, 5/5, ~0.6s | **Those tests are deliberately not included.** Their reproduction threshold is a property of one machine's scheduler — at 2k and 200 cycles the buggy build passed — so as permanent tests they'd cost ~560ms and 2000 thread creations on every suite run for a guarantee that may not hold on a CI runner. The ordering is protected the same way `SerializationThreadWorker`'s is: by the comment at the call site. `UOContent.Tests`: **597/597**.
This commit is contained in:
parent
9c11ccdb80
commit
1a9cec1dbb
1 changed files with 7 additions and 2 deletions
|
|
@ -117,10 +117,15 @@ public class AdvancedSearchThreadWorker
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
worker._stopEvent.Set(); // Allow the main thread to continue now that we are finished
|
// The owning thread may start another cycle the moment _stopEvent is set (Exit does exactly
|
||||||
|
// that). Clear _pause and sample the exit condition before signaling, or the new cycle's
|
||||||
|
// pause request is clobbered / its Sleep orphaned. Matches SerializationThreadWorker.
|
||||||
|
var exiting = Core.Closing || Volatile.Read(ref worker._exit);
|
||||||
Volatile.Write(ref worker._pause, false);
|
Volatile.Write(ref worker._pause, false);
|
||||||
|
|
||||||
if (Core.Closing || Volatile.Read(ref worker._exit))
|
worker._stopEvent.Set(); // Allow the main thread to continue now that we are finished
|
||||||
|
|
||||||
|
if (exiting)
|
||||||
{
|
{
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue