Skip to content

SingleThreadedThreadPool.Dispose() drops queued work: it cancels the worker before awaiting it (measured 197/200) #318

Description

Summary

SingleThreadedThreadPool.Dispose() cancels its worker rather than draining its queue, so work that
was enqueued but not yet started is silently dropped. Because DisposeAsync awaits _workerTask, the
shape reads as "wait for outstanding work to finish" — and it does wait, but only for the item already
in flight.

This bit two of the three consumers in DoxReloaded,
where the pool carries persistence writes: a settings change made immediately before teardown was dropped.

Why the queue is not drained

public async ValueTask DisposeAsync()
{
    _active = false;
    _disposed = true;
    _cancellationTokenSource.Cancel();   // <-- before the await
    Signal();
    try { await _workerTask.ConfigureAwait(false); }
    ...
}

private async Task DoWorkAsync()
{
    while (_active && !cancellationToken.IsCancellationRequested)   // <-- both already false
    {
        if (_work.TryDequeue(out WorkItem workItem)) { ... }
        ...
    }
}

By the time the worker re-evaluates its loop condition, _active is false and the token is
cancelled, so it exits with items still in _work.

Measured

Against this file's own source, on .NET 9, 200 trials each:

scenario result
enqueue 25 items, Dispose() immediately lost work in 200/200 trials, mean 20.9 of 25 dropped
enqueue 1 item, Dispose() immediately dropped in 197/200 trials (98.5 %)

And the window is narrow, which is what makes it hard to notice — the same probe with a delay between the
enqueue and the Dispose:

gap item dropped
0 ms 297/300 (99.0 %)
1 ms 0/300
2 ms+ 0/300

So a consumer usually wins the race and the drop stays invisible until the one time it matters.

Two suggestions, either of which would have prevented it

  1. Offer a drain. A CompleteAsync() / DrainAsync() that stops accepting new work, lets the queue
    run down, and then cancels. Consumers that enqueue durable work (persistence) need this; consumers
    that enqueue recomputable work (we have one that enqueues level generation) correctly want today's
    cancel-on-dispose behaviour, so both should be expressible.
  2. Failing that, document it at the declaration — that Dispose discards queued items, and that
    anything durable must be written by the caller on teardown. The await _workerTask inside
    DisposeAsync is what makes the current behaviour surprising, so the doc comment is the natural place
    to say what that await does and does not cover.

Separately: Dispose() blocks the calling thread on an async task

public void Dispose()
{
    DisposeAsync().AsTask().GetAwaiter().GetResult();
}

This is safe today only because DoWorkAsync uses ConfigureAwait(false) throughout, so no
continuation is posted back to Unity's main thread. A work item whose own continuations capture the
synchronization context would deadlock the Editor when Dispose() is called from the main thread — which
is where OnDestroy runs. Worth a note at minimum, since .GetAwaiter().GetResult() on Unity's main
thread is otherwise a hard rule to avoid.

Consumer-side workaround, for anyone who lands here first

Do not rely on the pool draining. On teardown: dispose the pool first (so nothing can still be writing),
then write the final state synchronously on the calling thread. Draining would be the wrong fix anyway
where every queued item rewrites the same file — only the last state matters.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions