Skip to content

Commit 899ec1f

Browse files
paulirwinclaude
andcommitted
Pass capacity: 0 to MemoryMappedFile.CreateFromFile, drop the #1090 retry loop
MemoryMappedFile.CreateFromFile accepts capacity: 0 to mean "size the mapping from the file's current length on disk." The framework does its own stat as part of the mapping creation, so there is no caller-side capacity for the file size to disagree with — the race window that #1090 was about is closed at the API boundary. This also lets us drop the FileStream we were holding alongside SharedMapping. The only thing it was being used for was capturing fc.Length to feed into our retry loop, plus being kept alive so the mapping had a handle. The path-based CreateFromFile overload opens its own handle and disposes it with the MemoryMappedFile, so we no longer need our own. Removes the retry loop in CreateMemoryMappedFile, the s_capacityRetryCount and s_maxCapacityAttemptsObserved test-observability counters, and the fileStream field on SharedMapping. Rewrites TestOpenInputConcurrentFileExtension_Issue1090 from a "race fired and was retried" assertion into a "OpenInput succeeds under concurrent file extension" smoke test. With capacity: 0 the original race no longer reaches the framework, so 8700 OpenInput iterations over 15s observed zero retries before the change. The test now shortens to a 5s window and simply asserts that every OpenInput completes cleanly while another thread extends/truncates the file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 62ef06f commit 899ec1f

2 files changed

Lines changed: 47 additions & 112 deletions

File tree

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

Lines changed: 17 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -496,16 +496,17 @@ private void AssertChunking(Random random, int chunkSize)
496496

497497
// LUCENENET: Regression test for GitHub #1090. A background thread
498498
// extends a file on disk while the foreground thread repeatedly
499-
// opens it with MMapDirectory.OpenInput. Before the fix, the
500-
// file's length could grow between the caller capturing fc.Length
501-
// and MemoryMappedFile.CreateFromFile performing its internal
502-
// stat, causing ArgumentOutOfRangeException (paramName="capacity")
503-
// with the message "The capacity may not be smaller than the
504-
// file size."
505-
// NonParallelizable: the retry-path assertion reads static counters on
506-
// MMapDirectory, so any other test exercising MMapDirectory in parallel
507-
// could skew the observed retry count.
508-
[Test, LuceneNetSpecific, Slow, NonParallelizable]
499+
// opens it with MMapDirectory.OpenInput. The original failure
500+
// mode was ArgumentOutOfRangeException(paramName="capacity")
501+
// from MemoryMappedFile.CreateFromFile, because we were passing
502+
// a caller-computed capacity (fc.Length) that could be smaller
503+
// than the actual file size by the time the framework did its
504+
// internal stat. The current design passes capacity: 0, which
505+
// tells the framework to size the mapping from the file's
506+
// current on-disk length atomically — there is no caller-side
507+
// capacity to disagree with the file. This test asserts that
508+
// OpenInput continues to succeed under concurrent file extension.
509+
[Test, LuceneNetSpecific, Slow]
509510
public void TestOpenInputConcurrentFileExtension_Issue1090()
510511
{
511512
var dir = CreateTempDir("testOpenInputConcurrentFileExtension");
@@ -549,50 +550,25 @@ public void TestOpenInputConcurrentFileExtension_Issue1090()
549550
{ IsBackground = true, Name = "mmap-issue1090-extender" };
550551
writer.Start();
551552

552-
// Snapshot counters so this test's assertion is not affected by
553-
// any earlier test's activity on MMapDirectory.
554-
long baselineRetries = Interlocked.Read(ref MMapDirectory.s_capacityRetryCount);
555-
556553
try
557554
{
558555
var sw = Stopwatch.StartNew();
559556
int iterations = 0;
560-
// Keep stretching the window until either the race fires or we
561-
// hit a hard deadline. On most machines this takes < 1 second.
562-
const int maxSeconds = 15;
557+
// Stress OpenInput while the background thread extends/truncates the
558+
// file. Each call must succeed cleanly. A short window is plenty:
559+
// before the capacity:0 fix this race fired in well under a second.
560+
const int maxSeconds = 5;
563561
while (sw.Elapsed < TimeSpan.FromSeconds(maxSeconds))
564562
{
565563
using (var _ = mmapDir.OpenInput(name, NewIOContext(Random)))
566564
{
567-
// Just open and dispose; the race occurs during construction.
565+
// Just open and dispose; the race occurred during construction.
568566
}
569567
iterations++;
570-
571-
if (Interlocked.Read(ref MMapDirectory.s_capacityRetryCount) > baselineRetries)
572-
{
573-
break; // race reproduced and handled by the retry loop
574-
}
575568
}
576569

577-
long retries = Interlocked.Read(ref MMapDirectory.s_capacityRetryCount) - baselineRetries;
578-
int maxAttempts = Volatile.Read(ref MMapDirectory.s_maxCapacityAttemptsObserved);
579-
580-
// Surface what was observed for diagnostics when run with -v normal.
581570
TestContext.Progress.WriteLine(
582-
$"TestOpenInputConcurrentFileExtension: iterations={iterations}, retries={retries}, maxAttemptsObserved={maxAttempts}");
583-
584-
// The real check: the race must have fired and our retry loop
585-
// must have swallowed it. Without the fix, the exception
586-
// escapes OpenInput and the test fails with ArgumentOutOfRangeException
587-
// (as seen in #1090). If the race never fires during this run
588-
// (timing-dependent), mark the test inconclusive rather than
589-
// silently passing — we haven't actually exercised the fix.
590-
if (retries == 0)
591-
{
592-
NUnit.Framework.Assert.Inconclusive(
593-
$"The concurrent-extension race was not reproduced within {maxSeconds}s " +
594-
$"({iterations} OpenInput iterations). The fix was therefore not exercised on this run.");
595-
}
571+
$"TestOpenInputConcurrentFileExtension: completed {iterations} OpenInput calls in {sw.Elapsed.TotalSeconds:F1}s");
596572
}
597573
finally
598574
{

src/Lucene.Net/Store/MMapDirectory.cs

Lines changed: 30 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -66,14 +66,6 @@ public class MMapDirectory : FSDirectory
6666

6767
private readonly int chunkSizePower;
6868

69-
// LUCENENET specific: test-only counters for the capacity-retry
70-
// path in Map() — see #1090. Internal (exposed via InternalsVisibleTo
71-
// to the test assemblies) so regression tests can assert that the
72-
// race was actually exercised during a run, and to gather data on how
73-
// many retries are typically needed. Not intended for production use.
74-
internal static long s_capacityRetryCount;
75-
internal static int s_maxCapacityAttemptsObserved;
76-
7769
/// <summary>
7870
/// Create a new <see cref="MMapDirectory"/> for the named location.
7971
/// </summary>
@@ -762,38 +754,55 @@ internal sealed unsafe class SharedMapping : IDisposable
762754
/// Note that this can be null in the edge case of a zero-length mapping.
763755
/// </summary>
764756
private readonly MemoryMappedFile? memoryMappedFile;
765-
private readonly FileStream fileStream;
766757
private int disposed;
767758

768-
private SharedMapping(MemoryMappedFile? mmf, FileStream fs, Chunk[] chunks, long length)
759+
private SharedMapping(MemoryMappedFile? mmf, Chunk[] chunks, long length)
769760
{
770761
this.memoryMappedFile = mmf;
771-
this.fileStream = fs;
772762
this.Chunks = chunks;
773763
this.Length = length;
774764
}
775765

776766
internal static SharedMapping Create(string file, int chunkSizePower)
777767
{
778-
// MemoryMappedFile uses only the file handle and bypasses
779-
// the FileStream buffer, so bufferSize: 1 avoids allocating
780-
// a 4 KiB buffer that would immediately be discarded.
781-
var fs = new FileStream(file, FileMode.Open, FileAccess.Read, FileShare.ReadWrite,
782-
bufferSize: 1, FileOptions.RandomAccess | FileOptions.Asynchronous);
768+
// We don't track a separate FileStream: the path-based
769+
// CreateFromFile overload opens its own handle and
770+
// disposes it with the MemoryMappedFile. We capture the
771+
// file length once via FileInfo for our own snapshot
772+
// (used as the slice/range upper bound). Any divergence
773+
// between this snapshot and the framework's internal
774+
// stat — e.g. the file growing in between, formerly the
775+
// #1090 race — is harmless: the mmap itself is sized by
776+
// the framework's own stat, and our `length` is treated
777+
// as a snapshot at open time (matching upstream Java's
778+
// fc.size() snapshot semantics).
779+
long length = new FileInfo(file).Length;
780+
if (length == 0)
781+
{
782+
return new SharedMapping(mmf: null, chunks: Array.Empty<Chunk>(), length: 0);
783+
}
784+
783785
MemoryMappedFile? mmf = null;
784786
Chunk[]? chunks = null;
785787
try
786788
{
787-
long length = fs.Length;
788-
mmf = CreateMemoryMappedFile(fs, length);
789+
// capacity: 0 -> the framework uses the file's
790+
// current size on disk, atomically with mapping
791+
// creation. This eliminates the #1090 race window
792+
// we previously had to retry around.
793+
mmf = MemoryMappedFile.CreateFromFile(
794+
path: file,
795+
mode: FileMode.Open,
796+
mapName: null,
797+
capacity: 0,
798+
access: MemoryMappedFileAccess.Read);
789799
chunks = MapChunks(mmf, 0, length, chunkSizePower);
790-
return new SharedMapping(mmf, fs, chunks, length);
800+
return new SharedMapping(mmf, chunks, length);
791801
}
792802
catch
793803
{
794804
DisposeChunks(chunks);
795805
mmf?.Dispose();
796-
IOUtils.DisposeWhileHandlingException(fs);
797806
throw;
798807
}
799808
}
@@ -805,63 +814,13 @@ public void Dispose()
805814
{
806815
if (Interlocked.CompareExchange(ref disposed, 1, 0) != 0) return;
807816
DisposeChunks(Chunks);
808-
IOUtils.DisposeWhileHandlingException(memoryMappedFile, fileStream);
817+
IOUtils.DisposeWhileHandlingException(memoryMappedFile);
809818
}
810819

811820
internal Chunk[] Chunks { get; }
812821

813822
internal long Length { get; }
814823

815-
private static MemoryMappedFile? CreateMemoryMappedFile(FileStream fc, long requiredCapacity)
816-
{
817-
if (requiredCapacity <= 0)
818-
{
819-
return null;
820-
}
821-
822-
// LUCENENET specific BEGIN: retry on capacity race (#1090).
823-
// MemoryMappedFile.CreateFromFile performs an internal stat
824-
// and throws ArgumentOutOfRangeException("capacity") if the
825-
// on-disk file size exceeds the requested capacity. When
826-
// another process/thread is appending to this file, the file
827-
// can grow between when we capture fc.Length and when
828-
// CreateFromFile reads the size.
829-
long capacity = Math.Max(requiredCapacity, fc.Length);
830-
const int maxAttempts = 5;
831-
int attempt = 0;
832-
while (true)
833-
{
834-
try
835-
{
836-
var mmf = MemoryMappedFile.CreateFromFile(
837-
fileStream: fc,
838-
mapName: null,
839-
capacity: capacity,
840-
access: MemoryMappedFileAccess.Read,
841-
#if FEATURE_MEMORYMAPPEDFILESECURITY
842-
memoryMappedFileSecurity: null,
843-
#endif
844-
inheritability: HandleInheritability.Inheritable,
845-
leaveOpen: true); // We dispose the FileStream explicitly.
846-
int attemptsTaken = attempt + 1;
847-
int prior;
848-
do
849-
{
850-
prior = Volatile.Read(ref s_maxCapacityAttemptsObserved);
851-
if (attemptsTaken <= prior) break;
852-
} while (Interlocked.CompareExchange(ref s_maxCapacityAttemptsObserved, attemptsTaken, prior) != prior);
853-
return mmf;
854-
}
855-
catch (ArgumentOutOfRangeException e) when (e.ParamName == "capacity" && attempt < maxAttempts - 1)
856-
{
857-
Interlocked.Increment(ref s_capacityRetryCount);
858-
capacity = Math.Max(capacity, fc.Length);
859-
attempt++;
860-
}
861-
}
862-
// LUCENENET specific END
863-
}
864-
865824
private static Chunk[] MapChunks(MemoryMappedFile? mmf, long offset, long length, int chunkSizePower)
866825
{
867826
if (length == 0 || mmf == null)

0 commit comments

Comments
 (0)