Address PR apache#1267 review: FileStream ownership + exception-handling cleanup

SharedMapping now owns and deterministically disposes the backing
FileStream (leaveOpen: true) instead of relying on the finalizer; the
MemoryMappedFile only borrows the handle and never disposes the stream
object. Adds internal test seams (MMapIndexInput.Mapping,
SharedMapping.IsFileStreamDisposed) and tests asserting the stream is
disposed on input/slicer dispose.

Fixes the two DisposeWhileHandlingException misuses (CreateAttempt and
MapChunks): dispose via the swallowing overload then rethrow with a bare
throw, so the original stack trace is preserved and there is no
double-throw. Removes the dead catch/dispose guards in OpenInput and
CreateSlicer (the constructors only set fields and cannot throw there)
and documents the mapping-ownership and slice-lifetime contracts.
Corrects the test comment that implied AlreadyClosedException is a
distinct type from ObjectDisposedException.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: 2 people

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

@@ -1444,9 +1444,12 @@ public void TestOpenFullSlice_On3xCfsFile_MatchesOpenInput()
                 "OpenFullSlice bytes must match OpenInput bytes for the same CFS file");
         }
 
-        // OpenFullSlice on a slicer that has been disposed must throw
-        // AlreadyClosedException, not ObjectDisposedException leaking from
-        // the underlying FileStream. Part of the review-item-6 fix.
+        // OpenFullSlice on a slicer that has been disposed must throw the
+        // already-closed exception. In Lucene.NET, AlreadyClosedException.Create()
+        // is a factory that returns an ObjectDisposedException (there is no
+        // distinct AlreadyClosedException type), so this test cannot and does not
+        // distinguish the two by type; it asserts via IsAlreadyClosedException(),
+        // which matches the ObjectDisposedException that Create() produces.
         [Test, LuceneNetSpecific]
         public void TestOpenFullSlice_AfterDispose_ThrowsAlreadyClosed()
         {
@@ -1492,6 +1495,71 @@ public void TestDisposeIndexInput()
             File.Delete(fileName);
         }
 
+        // LUCENENET specific: PR #1267 review item. MemoryMappedFile.CreateFromFile
+        // borrows the file handle from the FileStream we pass in but never disposes
+        // the FileStream object itself. SharedMapping therefore owns that FileStream
+        // and must dispose it deterministically on Dispose; otherwise the stream (a
+        // finalizable object holding the file handle) is left to the finalizer. We
+        // assert the invariant directly through internal members rather than by
+        // probing the OS, because the borrowed handle is released either way (the MMF
+        // closes it) and so the leak is not observable as a deletion or exclusive-open
+        // failure. IsFileStreamDisposed reports whether the owned FileStream was disposed.
+        // Note TestDisposeIndexInput above uses a zero-length file, which takes the
+        // early-return path that never calls CreateFromFile and owns no FileStream.
+        [Test, LuceneNetSpecific]
+        public void TestDisposeDisposesBackingFileStream_NonEmptyFile()
+        {
+            const string name = "bytes";
+            var dir = CreateTempDir("testDisposeDisposesBackingFileStream");
+
+            using MMapDirectory mmapDir = new MMapDirectory(dir);
+            using (var output = mmapDir.CreateOutput(name, NewIOContext(Random)))
+            {
+                output.WriteInt64(0x0123456789ABCDEFL);
+            }
+
+            var input = (MMapDirectory.MMapIndexInput)mmapDir.OpenInput(name, NewIOContext(Random));
+            var mapping = input.Mapping;
+            Assert.IsFalse(mapping.IsFileStreamDisposed,
+                "backing FileStream must still be open while the input is open");
+
+            input.Dispose();
+
+            Assert.IsTrue(mapping.IsFileStreamDisposed,
+                "disposing the root input must deterministically dispose the mapping's backing FileStream");
+        }
+
+        // LUCENENET specific: PR #1267 review item. The slicer (CreateSlicer) owns its
+        // own SharedMapping; disposing the slicer must dispose that mapping's backing
+        // FileStream just as disposing a root input does. This is the OpenFullSlice /
+        // 3.x CFS path the reviewer called out.
+        [Test, LuceneNetSpecific]
+        public void TestDisposeSlicerDisposesBackingFileStream_NonEmptyFile()
+        {
+            const string name = "bytes";
+            var dir = CreateTempDir("testDisposeSlicerDisposesBackingFileStream");
+
+            using MMapDirectory mmapDir = new MMapDirectory(dir);
+            using (var output = mmapDir.CreateOutput(name, NewIOContext(Random)))
+            {
+                output.WriteInt64(0x0123456789ABCDEFL);
+            }
+
+            var slicer = mmapDir.CreateSlicer(name, NewIOContext(Random));
+#pragma warning disable 612, 618
+            var full = (MMapDirectory.MMapIndexInput)slicer.OpenFullSlice();
+#pragma warning restore 612, 618
+            var mapping = full.Mapping;
+            Assert.IsFalse(mapping.IsFileStreamDisposed,
+                "backing FileStream must still be open while the slicer is open");
+
+            full.Dispose();
+            slicer.Dispose();
+
+            Assert.IsTrue(mapping.IsFileStreamDisposed,
+                "disposing the slicer must deterministically dispose the mapping's backing FileStream");
+        }
+
         // LUCENENET specific: tests written to investigate the concern raised
         // in PR #1267 (review comment r3137038502) that the per-file shared
         // mapping cache, keyed only by file name with a fixed Length captured

src/Lucene.Net/Store/MMapDirectory.cs

@@ -194,31 +194,24 @@ public override IndexInput OpenInput(string name, IOContext context)
             // Matches upstream Java (openInput creates a new FileChannel +
             // fc.map()) and ensures Length reflects the file's current size.
             SharedMapping mapping = SharedMapping.Create(file, chunkSizePower);
-            try
-            {
-                return new MMapIndexInput($"MMapIndexInput(path=\"{file}\")", ownsMapping: true, mapping, 0, mapping.Length, chunkSizePower);
-            }
-            catch
-            {
-                mapping.Dispose();
-                throw;
-            }
+            // Ownership of the mapping transfers to the returned root
+            // MMapIndexInput (ownsMapping: true); the caller of OpenInput is
+            // responsible for disposing that input, which disposes the mapping.
+            // The constructor only sets fields and cannot throw here, so no
+            // catch/dispose guard is needed.
+            return new MMapIndexInput($"MMapIndexInput(path=\"{file}\")", ownsMapping: true, mapping, 0, mapping.Length, chunkSizePower);
         }
 
         public override IndexInputSlicer CreateSlicer(string name, IOContext context)
         {
             EnsureOpen();
             var file = Path.Combine(Directory.FullName, name);
             SharedMapping mapping = SharedMapping.Create(file, chunkSizePower);
-            try
-            {
-                return new IndexInputSlicerAnonymousClass(this, file, mapping);
-            }
-            catch
-            {
-                mapping.Dispose();
-                throw;
-            }
+            // Ownership of the mapping transfers to the returned slicer; the
+            // caller of CreateSlicer is responsible for disposing that slicer,
+            // which disposes the mapping. The constructor only sets fields and
+            // cannot throw here, so no catch/dispose guard is needed.
+            return new IndexInputSlicerAnonymousClass(this, file, mapping);
         }
 
         private sealed class IndexInputSlicerAnonymousClass : IndexInputSlicer
@@ -246,6 +239,15 @@ public IndexInputSlicerAnonymousClass(MMapDirectory outerInstance, string file,
                 this.mapping = mapping;
             }
 
+            // Returns a slice the CALLER must dispose. The slice does not own the
+            // mapping (ownsMapping: false); it is also tracked in issuedSlices so
+            // that disposing the slicer cascades to any slices the caller left
+            // open. Note a slice can outlive a Dispose of outerInstance (the
+            // MMapDirectory): we deliberately do not thread outerInstance into the
+            // slice to re-check on every read, because reads against a disposed
+            // mapping already fail fast with AlreadyClosedException via the
+            // mapping's closed flag and per-chunk rent. EnsureOpen here only guards
+            // the act of opening a new slice.
             public override IndexInput OpenSlice(string sliceDescription, long offset, long length)
             {
                 outerInstance.EnsureOpen();
@@ -279,8 +281,13 @@ public override IndexInput OpenSlice(string sliceDescription, long offset, long
             public override IndexInput OpenFullSlice()
             {
                 outerInstance.EnsureOpen();
-                // The shared mapping's Length was captured at creation time,
-                // so we don't need to touch any FileStream here.
+                // A full slice is just a slice over the whole mapping. It shares
+                // the slicer's single SharedMapping (same MemoryMappedFile and
+                // FileStream) rather than opening a second mapping, and it is
+                // tracked in issuedSlices like any other slice, so disposing the
+                // slicer disposes it and the one backing FileStream. The mapping's
+                // Length was captured at creation time, so we touch no FileStream
+                // here.
                 return OpenSlice("full-slice", 0, mapping.Length);
             }
 
@@ -398,6 +405,11 @@ internal sealed unsafe class MMapIndexInput : IndexInput
             // on Dispose. Slices and clones do not own it.
             private readonly SharedMapping mapping;
 
+            // LUCENENET specific (PR #1267): for testing only. Exposes the shared
+            // mapping so a test can assert that disposing a root input
+            // deterministically disposes the mapping's backing FileStream.
+            internal SharedMapping Mapping => mapping;
+
             // The window into the shared mapping that this IndexInput sees.
             // For OpenInput this is [0, mapping.Length); for OpenSlice it is
             // the requested slice range. All offsets in the cached chunk
@@ -830,11 +842,34 @@ internal sealed unsafe class SharedMapping : IDisposable
             /// Note that this can be null in the edge case of a zero-length mapping.
             /// </summary>
             private readonly MemoryMappedFile? memoryMappedFile;
+
+            /// <summary>
+            /// The <see cref="FileStream"/> backing <see cref="memoryMappedFile"/>.
+            /// We pass this stream to
+            /// <see cref="MemoryMappedFile.CreateFromFile(FileStream, string?, long, MemoryMappedFileAccess, HandleInheritability, bool)"/>
+            /// with <c>leaveOpen: true</c>, so the mapping borrows the file handle
+            /// but never disposes the <see cref="FileStream"/> object. This mapping
+            /// owns it and disposes it in <see cref="Dispose"/> so the stream (a
+            /// finalizable object holding the file handle) is released
+            /// deterministically rather than left to the finalizer. Null for the
+            /// zero-length edge case (no mapping is created).
+            /// </summary>
+            private readonly FileStream? fileStream;
             private int disposed;
 
-            private SharedMapping(MemoryMappedFile? mmf, Chunk[] chunks, long length)
+            // LUCENENET specific (PR #1267): for testing only. True once Dispose has
+            // run and the owned FileStream (if any) has been disposed. Lets a test
+            // assert that the mapping releases its FileStream deterministically
+            // instead of leaking the object to finalization. Always true for the
+            // zero-length edge case, which owns no FileStream.
+            internal bool IsFileStreamDisposed =>
+                Volatile.Read(ref disposed) != 0 &&
+                (fileStream is null || !fileStream.CanRead);
+
+            private SharedMapping(MemoryMappedFile? mmf, FileStream? fileStream, Chunk[] chunks, long length)
             {
                 this.memoryMappedFile = mmf;
+                this.fileStream = fileStream;
                 this.Chunks = chunks;
                 this.Length = length;
             }
@@ -910,7 +945,6 @@ private static SharedMapping CreateAttempt(string file, int chunkSizePower)
                     bufferSize: 1, FileOptions.RandomAccess);
                 MemoryMappedFile? mmf = null;
                 Chunk[]? chunks = null;
-                Exception? priorException = null;
                 try
                 {
                     long length = fs.Length;
@@ -925,7 +959,7 @@ private static SharedMapping CreateAttempt(string file, int chunkSizePower)
                         // (it would be misleading — we successfully built a
                         // zero-length mapping).
                         IOUtils.DisposeWhileHandlingException(fs);
-                        return new SharedMapping(mmf: null, chunks: Array.Empty<Chunk>(), length: 0);
+                        return new SharedMapping(mmf: null, fileStream: null, chunks: Array.Empty<Chunk>(), length: 0);
                     }
 
                     // capacity: 0 -> the framework sizes the mapping
@@ -935,9 +969,14 @@ private static SharedMapping CreateAttempt(string file, int chunkSizePower)
                     // the length is re-read across the defaulting and
                     // validation steps, which is why Create wraps this
                     // call in a retry loop.
-                    // leaveOpen: false -> the MMF takes ownership of
-                    // the FileStream and disposes it on its own
-                    // Dispose, so we don't need to track it ourselves.
+                    // leaveOpen: true -> the MMF borrows fs's file handle
+                    // but does not close it; SharedMapping owns the
+                    // FileStream and disposes it (which closes the handle)
+                    // in Dispose. Note that even with leaveOpen: false the
+                    // MMF would close only the handle, never the FileStream
+                    // object itself, so we must track fs either way; using
+                    // leaveOpen: true keeps a single, unambiguous owner of
+                    // the handle and avoids a redundant handle close.
                     mmf = MemoryMappedFile.CreateFromFile(
                         fileStream: fs,
                         mapName: null,
@@ -947,30 +986,28 @@ private static SharedMapping CreateAttempt(string file, int chunkSizePower)
                         memoryMappedFileSecurity: null,
 #endif
                         inheritability: HandleInheritability.None,
-                        leaveOpen: false);
+                        leaveOpen: true);
                     chunks = MapChunks(mmf, 0, length, chunkSizePower);
-                    return new SharedMapping(mmf, chunks, length);
+                    return new SharedMapping(mmf, fs, chunks, length);
                 }
                 catch (Exception e) when (e.IsThrowable())
                 {
-                    priorException = e;
+                    // Cleanup must not mask e. DisposeChunks swallows internally,
+                    // so chunk teardown is safe. We dispose mmf/fs through the
+                    // swallowing overload of DisposeWhileHandlingException (the one
+                    // with no Exception parameter), which suppresses any Dispose
+                    // failure, and then rethrow e with a bare `throw;`. A bare
+                    // rethrow preserves e's original stack trace, and using the
+                    // swallowing overload (rather than the priorException overload,
+                    // which would ALSO throw) avoids a confusing double-throw.
+                    // With leaveOpen: true we always own fs (the MMF never disposes
+                    // it), so dispose both the mmf (if it was created) and fs. mmf
+                    // first so the mapping is torn down before the backing handle
+                    // is closed.
+                    DisposeChunks(chunks);
+                    IOUtils.DisposeWhileHandlingException(mmf, fs);
                     throw;
                 }
-                finally
-                {
-                    if (priorException != null)
-                    {
-                        // Cleanup must not mask priorException. DisposeChunks
-                        // swallows internally, so chunk teardown is safe.
-                        // For the mmf/fs the priorException overload attaches
-                        // any Dispose failure as a suppressed exception and
-                        // rethrows the original.
-                        // mmf owns fs once CreateFromFile returned (leaveOpen:
-                        // false); if mmf is null, fs ownership is still ours.
-                        DisposeChunks(chunks);
-                        IOUtils.DisposeWhileHandlingException(priorException, (IDisposable?)mmf ?? fs);
-                    }
-                }
             }
 
             /// <summary>
@@ -980,7 +1017,11 @@ public void Dispose()
             {
                 if (Interlocked.CompareExchange(ref disposed, 1, 0) != 0) return;
                 DisposeChunks(Chunks);
-                IOUtils.DisposeWhileHandlingException(memoryMappedFile);
+                // Tear down the mapping before closing the backing handle, then
+                // dispose the FileStream we own (the MMF was created with
+                // leaveOpen: true and never disposes it). fileStream is null for
+                // the zero-length edge case, which the overload tolerates.
+                IOUtils.DisposeWhileHandlingException(memoryMappedFile, fileStream);
             }
 
             internal Chunk[] Chunks { get; }
@@ -1019,27 +1060,22 @@ private static Chunk[] MapChunks(MemoryMappedFile? mmf, long offset, long length
 
                         MemoryMappedViewAccessor accessor = mmf.CreateViewAccessor(chunkOffset, thisChunkLen, MemoryMappedFileAccess.Read);
                         byte* ptr = null;
-                        Exception? acquireException = null;
                         try
                         {
                             accessor.SafeMemoryMappedViewHandle.AcquirePointer(ref ptr);
                         }
                         catch (Exception e) when (e.IsThrowable())
                         {
                             // Don't let accessor.Dispose() mask the original
-                            // AcquirePointer failure — route through the
-                            // priorException overload so any Dispose throw
-                            // becomes a suppressed exception on the original.
-                            acquireException = e;
+                            // AcquirePointer failure: dispose the accessor through the
+                            // swallowing overload (suppressing any Dispose failure),
+                            // then rethrow e with a bare `throw;` that preserves its
+                            // original stack trace. The throw skips the Chunk
+                            // construction below and propagates to the outer catch,
+                            // which disposes the already-built chunks.
+                            IOUtils.DisposeWhileHandlingException(accessor);
                             throw;
                         }
-                        finally
-                        {
-                            if (acquireException != null)
-                            {
-                                IOUtils.DisposeWhileHandlingException(acquireException, accessor);
-                            }
-                        }
                         // The accessor may be mapped at an offset inside the OS page,
                         // in which case PointerOffset is the distance from the
                         // SafeBuffer's base to the first byte of the requested view.