diff --git a/src/Lucene.Net.Tests/Index/TestIndexWriterOnJRECrash.cs b/src/Lucene.Net.Tests/Index/TestIndexWriterOnJRECrash.cs index 37695bbf78..ab8afb82b7 100644 --- a/src/Lucene.Net.Tests/Index/TestIndexWriterOnJRECrash.cs +++ b/src/Lucene.Net.Tests/Index/TestIndexWriterOnJRECrash.cs @@ -4,6 +4,7 @@ using NUnit.Framework; using RandomizedTesting.Generators; using System; +using System.Collections.Generic; using System.Diagnostics; using System.Globalization; using System.IO; @@ -11,6 +12,8 @@ using System.Net; using System.Net.Sockets; using System.Reflection; +using System.Runtime.InteropServices; +using System.Text; using System.Threading; using BaseDirectoryWrapper = Lucene.Net.Store.BaseDirectoryWrapper; using Assert = Lucene.Net.TestFramework.Assert; @@ -47,12 +50,15 @@ public class TestIndexWriterOnJRECrash : TestNRTThreads [Test] [Slow] - [AwaitsFix] public override void TestNRTThreads_Mem() { //if we are not the fork if (!SystemProperties.GetPropertyAsBoolean("tests:crashmode", false)) { + // LUCENENET: Bail out early (Inconclusive) if the current platform can't launch the fork, + // rather than hanging in WaitForProcessId waiting for a process that will never start. + EnsureForkPlatformSupported(); + // try up to 10 times to create an index for (int i = 0; i < 10; i++) { @@ -74,12 +80,17 @@ public override void TestNRTThreads_Mem() // Note this is the vstest.console process we are tracking here. p = ForkTest(tempDir.FullName, port); - TextWriter childOut = BeginOutput(p, out ThreadJob stdOutPumper, out ThreadJob stdErrPumper); + // LUCENENET: Capture STDERR so we can report it if the fork fails to start or + // exits with a non-zero exit code. + StringBuilder stdErrCapture = new StringBuilder(); + TextWriter childOut = BeginOutput(p, stdErrCapture, out ThreadJob stdOutPumper, out ThreadJob stdErrPumper); // LUCENENET: Note that ForkTest() creates the vstest.console.exe process. // This spawns testhost.exe, which runs our test. We wait until // the process starts and transmits its own PID so we know who to kill later. - int processIdToKill = WaitForProcessId(listener); + // If the fork exits before connecting (e.g. a build/launch error), this throws + // with the fork's exit code and STDERR rather than hanging indefinitely. + int processIdToKill = WaitForProcessId(listener, p, stdErrCapture); // Setup a time to crash the forked thread int crashTime = TestUtil.NextInt32(Random, 4000, 5000); // LUCENENET: Adjusted these up by 1 second to give our tests some more time to spin up @@ -162,31 +173,46 @@ public Process ForkTest(string tempDir, int port) //get the folder that's in string theDirectory = Path.GetDirectoryName(testAssemblyPath); + // LUCENENET: Only constrain the target platform when running as x86. Since .NET 8, an x86 + // `dotnet test` fork will not run unless the 32-bit SDK is installed separately (the 64-bit + // SDK is no longer sufficient), so we verify it up front and skip the test if it is missing + // rather than hanging forever in WaitForProcessId. For x64/ARM64 we leave the platform off and + // let vstest auto-detect a compatible host, which avoids a similar "Could not find 'dotnet' + // host for the 'X64' architecture" hang on ARM64 hosts. + var arguments = new List + { + // LUCENENET NOTE: dotnet test doesn't need the --no-build flag since we are passing the DLL path in + "test", testAssemblyPath, + "--framework", GetTargetFramework(), + "--filter", nameof(TestIndexWriterOnJRECrash), + "--logger:\"console;verbosity=normal\"", + "--", + }; + + string targetPlatform = GetTargetPlatform(); + if (targetPlatform != null) + { + arguments.Add($"RunConfiguration.TargetPlatform={targetPlatform}"); + } + + // LUCENENET NOTE: Since in our CI environment we create a lucene.testsettings.json file + // for all tests, we need to pass some of these settings as test run parameters to override + // for this process. These are read as system properties on the inside of the application. + arguments.Add(TestRunParameter("assert", "true")); + arguments.Add(TestRunParameter("tests:seed", SeedUtils.FormatSeed(Random.NextInt64()))); + arguments.Add(TestRunParameter("tests:culture", Thread.CurrentThread.CurrentCulture.Name)); + arguments.Add(TestRunParameter("tests:crashmode", "true")); + // passing NIGHTLY to this test makes it run for much longer, easier to catch it in the act... + arguments.Add(TestRunParameter("tests:nightly", "true")); + arguments.Add(TestRunParameter("tempDir", tempDir)); + // This port is for passing the process ID of the fork back to the original test so it can kill it. + arguments.Add(TestRunParameter("tests:crashtestport", port.ToString(CultureInfo.InvariantCulture))); + // Set up the process to run the console app ProcessStartInfo startInfo = new ProcessStartInfo { FileName = "dotnet", - Arguments = string.Join(" ", new[] { - // LUCENENET NOTE: dotnet test doesn't need the --no-build flag since we are passing the DLL path in - "test", testAssemblyPath, - "--framework", GetTargetFramework(), - "--filter", nameof(TestIndexWriterOnJRECrash), - "--logger:\"console;verbosity=normal\"", - "--", - $"RunConfiguration.TargetPlatform={GetTargetPlatform()}", - // LUCENENET NOTE: Since in our CI environment we create a lucene.testsettings.json file - // for all tests, we need to pass some of these settings as test run parameters to override - // for this process. These are read as system properties on the inside of the application. - TestRunParameter("assert", "true"), - TestRunParameter("tests:seed", SeedUtils.FormatSeed(Random.NextInt64())), - TestRunParameter("tests:culture", Thread.CurrentThread.CurrentCulture.Name), - TestRunParameter("tests:crashmode", "true"), - // passing NIGHTLY to this test makes it run for much longer, easier to catch it in the act... - TestRunParameter("tests:nightly", "true"), - TestRunParameter("tempDir", tempDir), - // This port is for passing the process ID of the fork back to the original test so it can kill it. - TestRunParameter("tests:crashtestport", port.ToString(CultureInfo.InvariantCulture)), - }), + Arguments = string.Join(" ", arguments), WorkingDirectory = theDirectory, RedirectStandardOutput = true, RedirectStandardError = true, @@ -215,12 +241,13 @@ private static string Escape(string value) private const string BackSlash = "\\"; private const string Space = " "; - private static TextWriter BeginOutput(Process p, out ThreadJob stdOutPumper, out ThreadJob stdErrPumper) + private static TextWriter BeginOutput(Process p, StringBuilder stdErrCapture, out ThreadJob stdOutPumper, out ThreadJob stdErrPumper) { // We pump everything to stderr. TextWriter childOut = Console.Error; - stdOutPumper = ThreadPumper.Start(p.StandardOutput, childOut); - stdErrPumper = ThreadPumper.Start(p.StandardError, childOut); + stdOutPumper = ThreadPumper.Start(p.StandardOutput, childOut, capture: null); + // LUCENENET: Capture the fork's STDERR so it can be surfaced if the fork fails. + stdErrPumper = ThreadPumper.Start(p.StandardError, childOut, capture: stdErrCapture); if (Verbose) childOut.WriteLine(">>> Begin subprocess output"); return childOut; } @@ -243,7 +270,48 @@ private string GetTargetFramework() private static string GetTargetPlatform() { - return Environment.Is64BitProcess ? "x64" : "x86"; + // LUCENENET: Only x86 needs an explicit target platform. The forked vstest can otherwise + // auto-detect a compatible dotnet host for the current architecture; forcing a platform on + // x64/ARM64 risks the fork failing with "Could not find 'dotnet' host for the '' + // architecture" (which left the parent blocked forever in WaitForProcessId, e.g. on ARM64 + // hosts), so we return null to leave RunConfiguration.TargetPlatform unset for those. + // + // For x86 we must verify the 32-bit SDK is actually installed: since .NET 8, an x86 + // `dotnet test` fork will not run unless the 32-bit SDK is installed separately. If it is + // missing, EnsureForkPlatformSupported() makes the test inconclusive rather than letting it + // hang waiting for a fork that can never start. + if (RuntimeInformation.ProcessArchitecture == Architecture.X86) + { + return "x86"; + } + + return null; + } + + // LUCENENET: Verify the runtime/SDK needed to launch the fork on the current platform is present, + // marking the test Inconclusive (rather than hanging) when it is not. Currently this only applies + // to x86, where the 32-bit .NET SDK must be installed separately since .NET 8. + private static void EnsureForkPlatformSupported() + { + if (RuntimeInformation.ProcessArchitecture != Architecture.X86) + { + return; + } + + // The x86 SDK installs under "Program Files (x86)\dotnet". On 64-bit Windows this path exists + // only when the 32-bit SDK has been installed in addition to the 64-bit one. + string programFilesX86 = Environment.GetFolderPath(Environment.SpecialFolder.ProgramFilesX86); + string x86DotnetExe = string.IsNullOrEmpty(programFilesX86) + ? null + : Path.Combine(programFilesX86, "dotnet", "dotnet.exe"); + + if (x86DotnetExe is null || !File.Exists(x86DotnetExe)) + { + NUnit.Framework.Assert.Inconclusive( + "The 32-bit .NET SDK is required to run this test on x86 but was not found at '" + + (x86DotnetExe ?? "") + "'. Since .NET 8, the 32-bit SDK must be installed " + + "separately from the 64-bit SDK to fork an x86 `dotnet test` process."); + } } #endregion @@ -251,9 +319,11 @@ private static string GetTargetPlatform() /// A pipe thread. It'd be nice to reuse guava's implementation for this... internal static class ThreadPumper { - public static ThreadJob Start(TextReader from, TextWriter to) + // LUCENENET: capture is an optional buffer that accumulates the piped text (e.g. STDERR) so it + // can be reported when the fork fails, independent of Verbose. + public static ThreadJob Start(TextReader from, TextWriter to, StringBuilder capture) { - ThreadJob t = new ThreadPumperAnonymousClass(from, to); + ThreadJob t = new ThreadPumperAnonymousClass(from, to, capture); t.Start(); return t; } @@ -262,11 +332,13 @@ private sealed class ThreadPumperAnonymousClass : ThreadJob { private readonly TextReader from; private readonly TextWriter to; + private readonly StringBuilder capture; - public ThreadPumperAnonymousClass(TextReader from, TextWriter to) + public ThreadPumperAnonymousClass(TextReader from, TextWriter to, StringBuilder capture) { this.from = from; this.to = to; + this.capture = capture; } public override void Run() @@ -277,6 +349,13 @@ public override void Run() int len; while ((len = from.Read(buffer, 0, buffer.Length)) > 0) { + if (capture != null) + { + lock (capture) + { + capture.Append(buffer, 0, len); + } + } if (Verbose) { to.Write(buffer, 0, len); @@ -315,25 +394,39 @@ public virtual bool CheckIndexes(FileSystemInfo file) { if (file is DirectoryInfo directoryInfo) { - BaseDirectoryWrapper dir = NewFSDirectory(directoryInfo); - dir.CheckIndexOnDispose = false; // don't double-checkindex - if (DirectoryReader.IndexExists(dir)) + BaseDirectoryWrapper dir = null; + Exception priorE = null; + try { - if (Verbose) + dir = NewFSDirectory(directoryInfo); + dir.CheckIndexOnDispose = false; // don't double-checkindex + if (DirectoryReader.IndexExists(dir)) { - Console.Error.WriteLine("Checking index: " + file); - } - // LUCENE-4738: if we crashed while writing first - // commit it's possible index will be corrupt (by - // design we don't try to be smart about this case - // since that too risky): - if (SegmentInfos.GetLastCommitGeneration(dir) > 1) - { - TestUtil.CheckIndex(dir); + if (Verbose) + { + Console.Error.WriteLine("Checking index: " + file); + } + // LUCENE-4738: if we crashed while writing first + // commit it's possible index will be corrupt (by + // design we don't try to be smart about this case + // since that too risky): + if (SegmentInfos.GetLastCommitGeneration(dir) > 1) + { + TestUtil.CheckIndex(dir); + } + return true; } - return true; } - dir.Dispose(); + catch (Exception e) + { + priorE = e; + throw; + } + finally + { + IOUtils.DisposeWhileHandlingException(priorE, dir); + } + foreach (DirectoryInfo f in directoryInfo.EnumerateDirectories()) { if (CheckIndexes(f)) @@ -354,15 +447,58 @@ private TcpListener SetupSocketListener() } // LUCENENET: Wait for our test to spin up and send its process ID so we can kill it. - private int WaitForProcessId(TcpListener listener) + // Rather than blocking forever in AcceptTcpClient(), poll for either an incoming connection or the + // fork exiting. If the fork exits before connecting (e.g. it failed to build or launch), hard-fail + // with its exit code and captured STDERR so the test reports the cause instead of hanging. + private int WaitForProcessId(TcpListener listener, Process fork, StringBuilder stdErrCapture) { - using var client = listener.AcceptTcpClient(); + IAsyncResult acceptResult = listener.BeginAcceptTcpClient(null, null); + + // The fork has to build the test project before it can run, so allow a generous window. + const int TimeoutMs = 120_000; + int waited = 0; + const int PollMs = 100; + while (!acceptResult.AsyncWaitHandle.WaitOne(PollMs)) + { + waited += PollMs; + if (fork.HasExited) + { + FailForkStartup(fork, stdErrCapture, "The forked process exited before connecting back."); + } + if (waited >= TimeoutMs) + { + FailForkStartup(fork, stdErrCapture, + $"Timed out after {TimeoutMs / 1000} seconds waiting for the forked process to connect back."); + } + } + + using var client = listener.EndAcceptTcpClient(acceptResult); using var stream = client.GetStream(); // Directly read the process ID as a 32-bit integer using var reader = new BinaryReader(stream); return reader.ReadInt32(); } + // LUCENENET: Hard-fail with the fork's exit code and captured STDERR so failures are diagnosable + // instead of presenting as a hang. + private static void FailForkStartup(Process fork, StringBuilder stdErrCapture, string message) + { + string exitCode = fork.HasExited + ? fork.ExitCode.ToString(CultureInfo.InvariantCulture) + : "(still running)"; + string stdErr; + lock (stdErrCapture) + { + stdErr = stdErrCapture.ToString(); + } + if (stdErr.Length == 0) + { + stdErr = "(no STDERR captured)"; + } + + Assert.Fail($"{message} Fork exit code: {exitCode}.{Environment.NewLine}Fork STDERR:{Environment.NewLine}{stdErr}"); + } + private void SendProcessId(int processId, int port) { using var client = new TcpClient("127.0.0.1", port); diff --git a/src/Lucene.Net.Tests/Store/TestDirectory.cs b/src/Lucene.Net.Tests/Store/TestDirectory.cs index 8b355ebd8b..a2ada8ddbd 100644 --- a/src/Lucene.Net.Tests/Store/TestDirectory.cs +++ b/src/Lucene.Net.Tests/Store/TestDirectory.cs @@ -438,6 +438,41 @@ public virtual void TestFsyncDoesntCreateNewFiles() Assert.AreEqual(0, fsdir.ListAll().Length); } + [Test] + [LuceneNetSpecific] + public virtual void TestFSIndexOutputFlushesBeforeMarkedStale() + { + DirectoryInfo path = CreateTempDir("flushBeforeMarkedStale"); + using var dir = new FlushObservingSimpleFSDirectory(path); + + byte[] bytes = new byte[1]; + Random.NextBytes(bytes); + + using (IndexOutput output = dir.CreateOutput("afile", NewIOContext(Random))) + { + output.WriteBytes(bytes, bytes.Length); + } + + Assert.AreEqual(bytes.Length, dir.LengthObservedOnIndexOutputClosed, + "FSDirectory must flush FileStream's managed buffer before marking a file stale for fsync."); + } + + private sealed class FlushObservingSimpleFSDirectory : SimpleFSDirectory + { + public FlushObservingSimpleFSDirectory(DirectoryInfo path) + : base(path) + { + } + + public long LengthObservedOnIndexOutputClosed { get; private set; } = -1; + + protected override void OnIndexOutputClosed(FSIndexOutput io) + { + LengthObservedOnIndexOutputClosed = new FileInfo(Path.Combine(m_directory.FullName, io.name)).Length; + base.OnIndexOutputClosed(io); + } + } + [Test] [Slow] [LuceneNetSpecific] diff --git a/src/Lucene.Net/Store/FSDirectory.cs b/src/Lucene.Net/Store/FSDirectory.cs index 0164e44fc7..8598eb88fc 100644 --- a/src/Lucene.Net/Store/FSDirectory.cs +++ b/src/Lucene.Net/Store/FSDirectory.cs @@ -510,6 +510,7 @@ protected class FSIndexOutput : BufferedIndexOutput private readonly FileStream file; #pragma warning restore CA2213 // Disposable fields should be disposed private volatile bool isOpen; // remember if the file is open, so that we don't try to close it more than once + private int disposed; // LUCENENET specific: 0 = open, 1 = disposed. Used with Interlocked to make Dispose run-once. private readonly CRC32 crc = new CRC32(); public FSIndexOutput(FSDirectory parent, string name) @@ -574,13 +575,29 @@ public override void Flush() /// protected override void Dispose(bool disposing) { - if (disposing) + // LUCENENET specific: Ensure dispose only runs once, even if two threads race here. + // A plain isOpen check is not enough: the read is atomic, but two threads could both + // observe true and both run the dispose logic. Atomically transition disposed 0 -> 1 + // so only the first thread proceeds. + if (disposing && 0 == Interlocked.CompareExchange(ref disposed, 1, 0)) { - parent.OnIndexOutputClosed(this); - // only close the file if it has not been closed yet - if (isOpen) + Exception priorE = null; // LUCENENET: No need to cast to IOException + + UninterruptableMonitor.Enter(parent.m_syncLock); + try { - Exception priorE = null; // LUCENENET: No need to cast to IOException + // LUCENENET: FileStream has a managed buffer, so a write is not guaranteed to + // have reached the OS until the buffer is flushed. (Java's RandomAccessFile.write() + // is unbuffered, so upstream has no such buffer and no flush race here.) If we add + // this file to m_staleFiles (via OnIndexOutputClosed) before flushing that buffer, + // another thread can fsync the file through a separate handle, remove the stale + // marker, and only then does this FileStream flush its buffer to the OS. That leaves + // bytes written after the fsync, which can make a committed index non-durable after + // a process or machine crash. + // + // Keep the FileStream flush and stale-file bookkeeping atomic with FSDirectory.Sync() + // (which also takes m_syncLock) so a file is never considered synced until all managed + // FileStream buffers have at least reached the OS. try { file.Flush(flushToDisk: false); @@ -589,11 +606,14 @@ protected override void Dispose(bool disposing) { priorE = ioe; } - finally - { - isOpen = false; - IOUtils.DisposeWhileHandlingException(priorE, file); - } + + parent.OnIndexOutputClosed(this); + } + finally + { + UninterruptableMonitor.Exit(parent.m_syncLock); + isOpen = false; + IOUtils.DisposeWhileHandlingException(priorE, file); } } //base.Dispose(disposing); // LUCENENET: No need to call base class, we are not using the functionality of BufferedIndexOutput