Skip to content

Commit e720bd8

Browse files
paulirwinclaude
andcommitted
Use SafeHandle refcount as the chunk drain barrier in MMapDirectory
Replace the hand-rolled per-chunk "rent" state machine (packed state/CLOSED_BIT/RENT_INC, TryAcquire/Release/Close/ReleaseNative CAS loops) with the BCL's own SafeMemoryMappedViewHandle refcount: a per-chunk-crossing AcquirePointer/ReleasePointer is the drain barrier, and accessor.Dispose() defers the actual unmap until all references drain and fails a later crossing fast (ObjectDisposedException -> AlreadyClosedException). This is the .NET-native equivalent of the custom rent, correct by construction, and apache#1151-safe because the reference is taken once per chunk crossing, not per read (a per-read refcount touch is the ~218x concurrency cliff apache#1151 is about). The cross-thread Dispose deferral is still required: a disposer must not release a read reference the acquiring reader may be mid-dereference of, or Chunk.Close could unmap the view under it (AVE). A same-thread Dispose releases directly; a cross-thread Dispose hands the reference to a finalizable StrandedReadRefReleaser that releases it once the input is unreachable. This is proven necessary by TestConcurrentSliceReadVsSlicerDispose, which AVEs without it. Tests: replace the finalizer-reclamation test with deterministic cross-thread-dispose and clone-dispose leak tests that assert chunk views and the backing FileStream are released synchronously (no GC.Collect/WaitForPendingFinalizers). The clone-dispose test closes the gap NightOwl888 hit (apache#1267): MockDirectoryWrapper's open-files gate does not track clones, so a clone leaking its view on its own Dispose would not fail that gate but would keep the file mapped (blocking overwrite/ delete on Windows). All 4 nightly concurrent race tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 483b4bc commit e720bd8

2 files changed

Lines changed: 303 additions & 312 deletions

File tree

src/Lucene.Net.Tests/Store/TestMultiMMap.cs

Lines changed: 109 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -2122,93 +2122,138 @@ public void TestCrossThreadCloneDisposeWhileReading()
21222122
}
21232123

21242124
[Test, LuceneNetSpecific]
2125-
public void TestStrandedChunkRentReclaimedByFinalizer()
2125+
public void TestCrossThreadCloneDisposeReleasesReadRefDeterministically()
21262126
{
2127-
// Regression (#1013): when an MMapIndexInput is disposed on a
2128-
// different thread than the one holding its current chunk rent
2129-
// (e.g. slicer.Dispose disposing slices other threads were
2130-
// reading), Dispose intentionally does NOT release the rent on the
2131-
// disposing thread - that could unmap a view under a live reader.
2132-
// The reader normally releases the rent itself on its next read.
2133-
// But if the reader stops reading without disposing, the rent would
2134-
// be stranded: without a backstop it would leak the chunk's
2135-
// MemoryMappedViewAccessor (and its mapped address space) forever.
2136-
// The fix hands the stranded rent to a finalizable releaser tied to
2137-
// the input's lifetime, so the rent is reclaimed once the input
2138-
// becomes unreachable. This test asserts the chunk's native
2139-
// resources ARE reclaimed after GC + finalization.
2140-
using var mmapDir = new MMapDirectory(CreateTempDir("strandedRent"), null, 1 << 4);
2141-
// 16-byte chunks, multi-chunk file so a chunk rent is taken.
2127+
// Regression (#1013): a clone that acquired its per-chunk-crossing
2128+
// read reference on one thread, then is disposed on a DIFFERENT
2129+
// thread (e.g. a slicer cascade disposing slices other threads had
2130+
// read), releases that reference ON THE DISPOSING THREAD -
2131+
// deterministically, with no GC/finalizer dependency.
2132+
// SafeBuffer.ReleasePointer is thread-safe, and the runtime defers
2133+
// the actual unmap until all references drain, so this is AVE-safe.
2134+
// Because the reference is gone the moment Dispose returns, the
2135+
// owner's subsequent Dispose unmaps every chunk immediately.
2136+
//
2137+
// This test deliberately does NOT call GC.Collect /
2138+
// WaitForPendingFinalizers anywhere: if cleanup were still
2139+
// finalizer-dependent, the final assertion would fail.
2140+
using var mmapDir = new MMapDirectory(CreateTempDir("crossThreadDispose"), null, 1 << 4);
2141+
// 16-byte chunks, multi-chunk file so a chunk read reference is taken.
21422142
WriteFile(mmapDir, "f", 256);
21432143
var parent = (MMapDirectory.MMapIndexInput)mmapDir.OpenInput("f", NewIOContext(Random));
21442144
MMapDirectory.Chunk[] chunks = parent.Mapping.Chunks;
21452145
Assert.IsTrue(chunks.Length > 1, "expected a multi-chunk mapping");
21462146

2147-
// Strand a rent on a clone of `parent` (see StrandRentOnClone). The
2148-
// helper is static and holds no reference that escapes back here, so
2149-
// the clone becomes unreachable as soon as it returns and is eligible
2150-
// for finalization. It strands the rent on chunk 0.
2151-
StrandRentOnClone(parent);
2152-
MMapDirectory.Chunk stranded = chunks[0];
2153-
Assert.IsFalse(stranded.IsNativeReleased,
2154-
"rent should still be outstanding before finalization - " +
2155-
"the cross-thread Dispose must not release it");
2156-
2157-
// Drain the finalizer queue. The releaser's finalizer releases the
2158-
// stranded rent, dropping inFlight to 0. The mapping is NOT disposed
2159-
// yet (parent still holds it open), so the chunk is not Closed and
2160-
// ReleaseNative does not run here - it only runs at the terminal
2161-
// (closed=1, inFlight=0) transition. We retry the drain a few times
2162-
// because exactly when the clone is collected and its finalizer
2163-
// pumped is not deterministic.
2164-
for (int i = 0; i < 10 && !stranded.IsNativeReleasedOrZeroRent; i++)
2165-
{
2166-
GC.Collect();
2167-
GC.WaitForPendingFinalizers();
2168-
}
2169-
Assert.IsTrue(stranded.IsNativeReleasedOrZeroRent,
2170-
"the stranded rent must be released by the releaser's finalizer");
2171-
2172-
// Now close the mapping. With the stranded rent released by the
2173-
// finalizer above, Close observes inFlight == 0 and reclaims the
2174-
// accessor. Had the finalizer NOT released the rent, Close would set
2175-
// the closed bit but defer ReleaseNative forever (inFlight stuck at
2176-
// 1) - i.e. the original leak.
2147+
// A clone reads chunk 0's first byte on a dedicated thread (so the
2148+
// read reference is acquired on a thread other than this one), then
2149+
// this thread disposes the clone - the cross-thread Dispose path.
2150+
DisposeCloneCrossThread(parent);
2151+
2152+
// The clone's read reference was released by its own (cross-thread)
2153+
// Dispose, synchronously. Disposing the root now closes (disposes)
2154+
// every chunk accessor; with no references outstanding the runtime
2155+
// unmaps them immediately, with no finalizer step.
21772156
parent.Dispose();
21782157

2179-
Assert.IsTrue(stranded.IsNativeReleased,
2180-
"stranded chunk rent must be reclaimed after finalization + " +
2181-
"mapping close; otherwise the MemoryMappedViewAccessor leaks");
2158+
foreach (var c in chunks)
2159+
{
2160+
Assert.IsTrue(c.IsNativeReleased,
2161+
"every chunk accessor must be closed synchronously by the " +
2162+
"root's Dispose; a stranded cross-thread read reference would " +
2163+
"have deferred this to the finalizer");
2164+
}
2165+
Assert.IsTrue(parent.Mapping.IsFileStreamDisposed,
2166+
"the backing FileStream must be disposed deterministically too");
21822167
}
21832168

2184-
// Strands chunk 0's rent on a fresh clone of `parent`: a dedicated
2185-
// reader thread acquires the rent (by reading one byte), then THIS
2186-
// (different) thread disposes the clone. Because rent-owner thread !=
2187-
// disposer thread, Dispose hands the rent to a finalizable releaser
2188-
// instead of releasing it. Static and self-contained so the clone (and
2189-
// the reader thread that ran against it) are unreachable once this
2190-
// returns, making the clone - and the releaser it references - eligible
2191-
// for finalization.
2169+
// Reads chunk 0's first byte on a fresh clone of `parent` from a
2170+
// dedicated reader thread (acquiring the read reference on THAT thread),
2171+
// then disposes the clone from THIS thread - exercising the cross-thread
2172+
// Dispose path. Static and self-contained so the clone and reader thread
2173+
// are unreachable once this returns.
21922174
[System.Runtime.CompilerServices.MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining)]
2193-
private static void StrandRentOnClone(MMapDirectory.MMapIndexInput parent)
2175+
private static void DisposeCloneCrossThread(MMapDirectory.MMapIndexInput parent)
21942176
{
21952177
IndexInput clone = (IndexInput)parent.Clone();
2196-
// Acquire the rent on a different thread via a parameterized start so
2197-
// the clone is NOT captured into a closure that the Thread object
2198-
// would keep alive. The Thread is local and gone after this returns.
2178+
// Acquire the read reference on a different thread via a parameterized
2179+
// start so the clone is NOT captured into a closure that the Thread
2180+
// object would keep alive. The Thread is local and gone after return.
21992181
var reader = new System.Threading.Thread(static state =>
22002182
{
22012183
var c = (IndexInput)state!;
22022184
c.Seek(0);
2203-
c.ReadByte(); // acquires chunk 0's rent on THIS thread
2185+
c.ReadByte(); // acquires chunk 0's read reference on THIS thread
22042186
});
22052187
reader.Start(clone);
22062188
reader.Join();
2207-
// Reader has exited holding chunk 0's rent. Dispose from this thread
2208-
// (cross-thread relative to the rent owner): the rent is handed off.
2189+
// Reader has exited holding chunk 0's read reference. Dispose from
2190+
// this thread (cross-thread relative to the acquirer): the reference
2191+
// is released here, deterministically.
22092192
clone.Dispose();
22102193
}
22112194

2195+
[Test, LuceneNetSpecific]
2196+
public void TestCloneDisposeReleasesViewsDeterministically()
2197+
{
2198+
// Regression (#1267, NightOwl888 leak concern): a clone holds no
2199+
// native resource of its own beyond a per-chunk-crossing read
2200+
// reference it acquires while reading. Disposing the clone (on its
2201+
// own thread, the normal path) must release that reference
2202+
// deterministically, and disposing the root must then unmap every
2203+
// chunk view and the backing FileStream synchronously - with NO
2204+
// GC/finalizer step. This closes the gap that MockDirectoryWrapper's
2205+
// open-files gate cannot see: it does not track clones, so a clone
2206+
// leaking its view on its own Dispose would not fail that gate, but
2207+
// it WOULD leave the file mapped (on Windows, blocking a later
2208+
// overwrite/delete - exactly NightOwl888's "Cannot overwrite"
2209+
// symptom). This test never calls GC.Collect/WaitForPendingFinalizers.
2210+
using var mmapDir = new MMapDirectory(CreateTempDir("cloneDisposeLeak"), null, 1 << 4);
2211+
// 16-byte chunks, multi-chunk file.
2212+
WriteFile(mmapDir, "f", 256);
2213+
var root = (MMapDirectory.MMapIndexInput)mmapDir.OpenInput("f", NewIOContext(Random));
2214+
MMapDirectory.Chunk[] chunks = root.Mapping.Chunks;
2215+
Assert.IsTrue(chunks.Length > 1, "expected a multi-chunk mapping");
2216+
2217+
// Read through a clone on THIS thread so it acquires (and the cache
2218+
// holds) a per-chunk read reference, then dispose the clone. Reading
2219+
// a few bytes that cross a chunk boundary exercises acquire+release
2220+
// across chunks; the final read leaves a reference cached that the
2221+
// clone's Dispose must release.
2222+
var clone = (IndexInput)root.Clone();
2223+
clone.Seek(0);
2224+
for (int i = 0; i < 32; i++) clone.ReadByte(); // crosses chunk 0 -> chunk 1
2225+
clone.Dispose();
2226+
2227+
// The clone is gone; its read reference was released by its own
2228+
// Dispose. The root still holds the file open, so chunks remain
2229+
// mapped (a clone Dispose must NOT tear down the shared mapping).
2230+
foreach (var c in chunks)
2231+
{
2232+
Assert.IsFalse(c.IsNativeReleased,
2233+
"disposing a clone must NOT unmap the shared chunks the root still owns");
2234+
}
2235+
Assert.IsFalse(root.Mapping.IsFileStreamDisposed,
2236+
"the root still holds the file open after the clone is disposed");
2237+
2238+
// The root can still read - proves the clone's Dispose released its
2239+
// reference cleanly without disturbing the shared mapping.
2240+
root.Seek(0);
2241+
root.ReadByte();
2242+
2243+
// Disposing the root unmaps every chunk and the backing FileStream,
2244+
// synchronously, with no finalizer dependency. If the clone had
2245+
// leaked its read reference, a chunk's view would stay mapped here.
2246+
root.Dispose();
2247+
foreach (var c in chunks)
2248+
{
2249+
Assert.IsTrue(c.IsNativeReleased,
2250+
"disposing the root must unmap every chunk view synchronously; " +
2251+
"a leaked clone read reference would have kept one mapped");
2252+
}
2253+
Assert.IsTrue(root.Mapping.IsFileStreamDisposed,
2254+
"the backing FileStream must be disposed deterministically");
2255+
}
2256+
22122257
[Test, LuceneNetSpecific]
22132258
public void TestSlicedReadInt32AcrossOffsets()
22142259
{

0 commit comments

Comments
 (0)