Stop the size-tracker resizer before clearing objectPages in ObjectAllocatorImpl.Dispose

[Nilesh Patil (nileshpatil6)]

Fixes #2069.

ObjectAllocatorImpl.Dispose() clears objectPages first and calls base.Dispose() last:

var localValues = Interlocked.Exchange(ref objectPages, null);
if (localValues != null)
{
    freePagePool.Dispose();
    foreach (var value in localValues)
        value.Clear();
    base.Dispose();
}

AllocatorBase.Dispose() is where the resizer is stopped, and it deliberately does that first, with the reason written out above the call:

Stop the size-tracker resizer and wait for it to exit BEFORE tearing down the epoch, buffer pool, flush event (and, by the owner, the log device) that it uses; otherwise a still-running resizer can spin on or dereference these cleared resources.

objectPages is the same kind of resource, but it is cleared before that call runs, so the invariant does not hold for it. A resizer still inside ResizeIfNeeded -> ShiftAddresses reaches EvictRecordsInRange, which dereferences it directly:

var objectIdMap = objectPages[GetPageIndexForAddress(address)].objectIdMap;

so the NullReferenceException in the issue's stack trace, which OnPagesClosedWorker escalates to Environment.FailFast, taking the whole process down.

This moves the stop to the top of the override, so the resizer is stopped and awaited before the array it uses is cleared:

logSizeTracker?.Stop(wait: true);

Notes on why this is safe:

• base.Dispose() still calls Stop(wait: true) afterwards. Stop compare-exchanges runState from Running, so once the resizer has stopped the second call takes neither the signal path nor the wait loop, and returns immediately.
• The Interlocked.Exchange on objectPages still guards the rest of the method, so the dispose-once behaviour is unchanged. Stop is safe to reach on a second Dispose() for the same reason as above.
• logSizeTracker is internal on AllocatorBase, so no accessibility change was needed.

I did not add a test. The failure is a shutdown race, and the observable outcome is Environment.FailFast, which terminates the test host rather than failing an assertion, so a test that reproduces it would take the run down with it rather than report. What is testable is the ordering itself, and after this change that ordering is enforced by construction in the override.

Built Tsavorite.core clean, and ran the in-memory allocator tests (Name~NativeInMem, 11 passed / 3 skipped) to check for fallout on the normal dispose path.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[Nilesh Patil (nileshpatil6)]

Good catch, that is a real problem with the first version and I have pushed a fix.

Stop(wait: true) spins on !IsStopped && !(resizerTask is { IsCompleted: true }), and the resizer only reaches OnStopped() after ResizeIfNeeded returns. If it was already parked in one of the while (waitForEviction && !disposed && ClosedUntilAddress < newHeadAddress) loops in ShiftAddressesWithWait, disposed was still false when my override called Stop, so the release condition the base method's comment relies on was not in place yet. That is exactly the invariant the comment above the base call spells out, and calling Stop from the override skipped the half of it that makes the wait safe.

Fixed as suggested. Both statements now live in one protected helper on AllocatorBase:

protected void StopSizeTrackerForDispose()
{
    disposed = true;
    logSizeTracker?.Stop(wait: true);
}

AllocatorBase.Dispose() calls it in place of the two lines it had, so its ordering is byte-for-byte what it was, and ObjectAllocatorImpl.Dispose() calls the same helper before the Interlocked.Exchange on objectPages. The override now gets disposed = true first as well, so a parked resizer is released rather than waited on.

It is idempotent for the second call from base.Dispose(): disposed is already true, and Stop compare-exchanges runState from Running, so once the resizer has stopped it takes neither the signal path nor the wait loop.

disposed is private volatile on AllocatorBase, so a helper was needed regardless; the derived class could not set it directly.

Rebuilt Tsavorite.core clean and re-ran the in-memory allocator tests (Name~NativeInMem): 11 passed / 3 skipped on both net8.0 and net10.0.

[Ted Hart (TedHartMS)]

I believe you have completed addressing the Copilot comment; please Resolve if so