Skip to content

Commit 3a46c36

Browse files
paulirwinclaude
andcommitted
Address review feedback on WindowsDirectory/NativeUnixDirectory port (#1342)
Per NightOwl888's review of #1346: - Surface IOException.HResult on the native-error paths so NativeFSLock (which inspects HResult to detect share/lock violations) keeps working: - errno-derived throws (NativePosixUtil.NewIOException/posix_fadvise; NativeUnixDirectory PWrite/FTruncate/FileSize) pass errno into the IOException(message, hresult) ctor. On Unix the BCL surfaces the raw errno as HResult (a FileStream share violation gives HResult == EWOULDBLOCK), so errno-as-HResult matches the convention. - Win32-derived throws (WindowsDirectory OpenFile/Read/Length) use Marshal.GetHRForLastWin32Error(). - Exception-wrap sites (WindowsDirectory.ReadInternal; NativeUnixDirectory Refill/Length/Clone) preserve the original HResult. Uses the IOException(message, hresult) ctor where possible because the HResult setter is protected on net462/netstandard2.0; the RuntimeException wrappers guard the setter with #if for those TFMs. - Implement NativeUnixIndexOutput.Checksum via a running CRC32 (like FSIndexOutput) instead of throwing NotSupportedException. The base-suite TestChecksum now passes against NativeUnixDirectory. - Move the LuceneNetSpecific test files into the Support folder: Store/Test{NativeUnix,Windows}Directory.cs -> Support/Store/... Not changed: the optional Put/Get micro-optimizations (Unsafe.CopyBlockUnaligned / ref byte), which the reviewer flagged as "benchmark to be sure it is worth it"; the current Span.CopyTo already lowers to memmove. Verified: builds on net10/net8/netstandard2.0/net462; TestNativeUnix/TestWindows pass 24/24 on macOS arm64 and 23/23 on Linux aarch64. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent df10293 commit 3a46c36

5 files changed

Lines changed: 63 additions & 13 deletions

File tree

src/Lucene.Net.Misc/Store/NativePosixUtil.cs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,9 @@ public static void Advise(SafeFileHandle fd, long offset, long len, int advise)
159159
if (code != 0)
160160
{
161161
// LUCENENET: upstream throws RuntimeException; we use IOException as this is an I/O failure.
162-
throw new IOException("posix_fadvise failed code=" + code);
162+
// posix_fadvise returns the error number directly (it does not set errno); surface it as
163+
// the HResult to match the errno-as-HResult convention used elsewhere here.
164+
throw new IOException("posix_fadvise failed code=" + code, code);
163165
}
164166
}
165167

@@ -188,7 +190,10 @@ private static IOException NewIOException(string operation, string filename, int
188190
string where = filename is null ? operation : $"{operation} {filename}";
189191
// On Unix with SetLastError, GetLastWin32Error() returns errno. Win32Exception's message
190192
// is not meaningful on Unix, so include the raw errno for diagnosis.
191-
return new IOException($"{where} failed (errno {errno})");
193+
// LUCENENET: surface the errno as the IOException's HResult. On Unix the BCL sets HResult to
194+
// the raw errno (e.g. a FileStream share violation surfaces HResult == EWOULDBLOCK), and
195+
// NativeFSLock inspects IOException.HResult, so we match that convention here.
196+
return new IOException($"{where} failed (errno {errno})", errno);
192197
}
193198

194199
internal static void EnsureUnix()

src/Lucene.Net.Misc/Store/NativeUnixDirectory.cs

Lines changed: 41 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using Lucene.Net.Support;
12
using Lucene.Net.Util;
23
using Microsoft.Win32.SafeHandles;
34
using System;
@@ -186,15 +187,20 @@ private static void PWrite(SafeFileHandle fd, IntPtr buffer, int length, long po
186187
long written = (long)NativeMethods.pwrite(Fd(fd), buffer, (nuint)length, pos);
187188
if (written < 0)
188189
{
189-
throw new IOException($"pwrite failed (errno {Marshal.GetLastWin32Error()})");
190+
int errno = Marshal.GetLastWin32Error();
191+
// LUCENENET: surface errno as the HResult (see NativePosixUtil.NewIOException) so callers
192+
// such as NativeFSLock, which inspect IOException.HResult, still see the underlying error.
193+
throw new IOException($"pwrite failed (errno {errno})", errno);
190194
}
191195
}
192196

193197
private static void FTruncate(SafeFileHandle fd, long length)
194198
{
195199
if (NativeMethods.ftruncate(Fd(fd), length) < 0)
196200
{
197-
throw new IOException($"ftruncate failed (errno {Marshal.GetLastWin32Error()})");
201+
int errno = Marshal.GetLastWin32Error();
202+
// LUCENENET: surface errno as the HResult (see PWrite).
203+
throw new IOException($"ftruncate failed (errno {errno})", errno);
198204
}
199205
}
200206

@@ -205,7 +211,9 @@ private static long FileSize(SafeFileHandle fd)
205211
long size = NativeMethods.lseek(Fd(fd), 0, NativeMethods.SEEK_END);
206212
if (size < 0)
207213
{
208-
throw new IOException($"lseek failed (errno {Marshal.GetLastWin32Error()})");
214+
int errno = Marshal.GetLastWin32Error();
215+
// LUCENENET: surface errno as the HResult (see PWrite).
216+
throw new IOException($"lseek failed (errno {errno})", errno);
209217
}
210218
return size;
211219
}
@@ -286,6 +294,9 @@ private sealed class NativeUnixIndexOutput : IndexOutput
286294
private readonly AlignedByteBuffer buffer;
287295
private readonly SafeFileHandle fd;
288296
private readonly int bufferSize;
297+
// LUCENENET specific: track a running CRC32 over the bytes written (in logical order), so
298+
// Checksum can return it like FSIndexOutput does. Upstream left getChecksum() unsupported.
299+
private readonly CRC32 crc = new CRC32();
289300

290301
private int bufferPos;
291302
private long filePos;
@@ -302,6 +313,7 @@ public NativeUnixIndexOutput(string path, int bufferSize)
302313

303314
public override void WriteByte(byte b)
304315
{
316+
crc.Update(b);
305317
buffer.Put(b);
306318
if (++bufferPos == bufferSize)
307319
{
@@ -311,6 +323,7 @@ public override void WriteByte(byte b)
311323

312324
public override void WriteBytes(ReadOnlySpan<byte> source)
313325
{
326+
crc.Update(source);
314327
int offset = 0;
315328
int toWrite = source.Length;
316329
while (true)
@@ -393,7 +406,9 @@ public override long Length
393406
set { /* not supported: length is managed internally */ }
394407
}
395408

396-
public override long Checksum => throw new NotSupportedException("this directory currently does not work at all!");
409+
// LUCENENET specific: upstream left getChecksum() unsupported; we maintain a running CRC32
410+
// over the written bytes (like FSIndexOutput) so the checksum footer can be written.
411+
public override long Checksum => crc.Value;
397412

398413
protected override void Dispose(bool disposing)
399414
{
@@ -494,7 +509,16 @@ public override long Length
494509
}
495510
catch (Exception ioe) when (ioe.IsIOException())
496511
{
497-
throw RuntimeException.Create("IOException during Length: " + this, ioe);
512+
// LUCENENET: upstream wraps in a RuntimeException here (IndexInput.Length cannot
513+
// throw a checked IOException in Java). Preserve the original HResult so callers
514+
// that inspect it (e.g. NativeFSLock) still see the underlying error. The HResult
515+
// setter is only public on modern TFMs; on net462/netstandard2.0 it is protected,
516+
// so the detail is retained in the message there instead.
517+
Exception wrapped = RuntimeException.Create("IOException during Length: " + this, ioe);
518+
#if !(NETSTANDARD2_0 || NET462)
519+
wrapped.HResult = ioe.HResult;
520+
#endif
521+
throw wrapped;
498522
}
499523
}
500524
}
@@ -524,7 +548,10 @@ private void Refill()
524548
}
525549
catch (Exception ioe) when (ioe.IsIOException())
526550
{
527-
throw new IOException(ioe.Message + ": " + this, ioe);
551+
// LUCENENET: surface the original HResult (via the ctor that accepts it, which is
552+
// public on all target frameworks) so callers such as NativeFSLock still see the
553+
// underlying error. The inner detail is retained in the message.
554+
throw new IOException(ioe.Message + ": " + this, ioe.HResult);
528555
}
529556
// Upstream used FileChannel.read(), which returns -1 at EOF. Here Pread() wraps libc
530557
// pread(), which returns 0 at EOF (a genuine error already threw above), so a refill that
@@ -567,7 +594,14 @@ public override object Clone()
567594
}
568595
catch (Exception ioe) when (ioe.IsIOException())
569596
{
570-
throw RuntimeException.Create("IOException during clone: " + this, ioe);
597+
// LUCENENET: as in Length above, upstream wraps in a RuntimeException (Clone cannot
598+
// throw a checked IOException in Java). Preserve the original HResult where the setter
599+
// is public (modern TFMs); on net462/netstandard2.0 the detail stays in the message.
600+
Exception wrapped = RuntimeException.Create("IOException during clone: " + this, ioe);
601+
#if !(NETSTANDARD2_0 || NET462)
602+
wrapped.HResult = ioe.HResult;
603+
#endif
604+
throw wrapped;
571605
}
572606
}
573607

src/Lucene.Net.Misc/Store/WindowsDirectory.cs

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,11 @@ protected override void ReadInternal(Span<byte> b)
157157
}
158158
catch (Exception ioe) when (ioe.IsIOException())
159159
{
160-
throw new IOException(ioe.Message + ": " + this, ioe);
160+
// LUCENENET: surface the original HResult (via the ctor that accepts it, which is
161+
// public on all target frameworks) so callers such as NativeFSLock, which inspect
162+
// IOException.HResult, still see the underlying error. The inner detail is retained
163+
// in the message.
164+
throw new IOException(ioe.Message + ": " + this, ioe.HResult);
161165
}
162166

163167
if (bytesRead != b.Length)
@@ -208,10 +212,13 @@ private static SafeFileHandle OpenFile(string filename)
208212
NativeMethods.FILE_FLAG_RANDOM_ACCESS,
209213
IntPtr.Zero);
210214
int lastError = Marshal.GetLastWin32Error();
215+
// LUCENENET: surface the Win32 error as the IOException's HResult (matching what the BCL
216+
// sets on a FileStream IOException) so callers such as NativeFSLock can recognize it.
217+
int hresult = Marshal.GetHRForLastWin32Error();
211218

212219
if (handle == NativeMethods.INVALID_HANDLE_VALUE)
213220
{
214-
throw new IOException("Could not open file " + filename + ": " + new Win32Exception(lastError).Message);
221+
throw new IOException("Could not open file " + filename + ": " + new Win32Exception(lastError).Message, hresult);
215222
}
216223

217224
return new SafeFileHandle(handle, ownsHandle: true);
@@ -239,7 +246,9 @@ private static unsafe int Read(SafeFileHandle fd, Span<byte> bytes, long pos)
239246

240247
if (!success)
241248
{
242-
throw new IOException(new Win32Exception(Marshal.GetLastWin32Error()).Message);
249+
int lastError = Marshal.GetLastWin32Error();
250+
// LUCENENET: surface the Win32 error as the IOException's HResult (see OpenFile).
251+
throw new IOException(new Win32Exception(lastError).Message, Marshal.GetHRForLastWin32Error());
243252
}
244253

245254
return numRead;
@@ -251,7 +260,9 @@ private static long Length(SafeFileHandle fd)
251260
{
252261
if (!NativeMethods.GetFileInformationByHandle(fd, out NativeMethods.BY_HANDLE_FILE_INFORMATION info))
253262
{
254-
throw new IOException(new Win32Exception(Marshal.GetLastWin32Error()).Message);
263+
int lastError = Marshal.GetLastWin32Error();
264+
// LUCENENET: surface the Win32 error as the IOException's HResult (see OpenFile).
265+
throw new IOException(new Win32Exception(lastError).Message, Marshal.GetHRForLastWin32Error());
255266
}
256267

257268
return ((long)info.nFileSizeHigh << 0x20) | info.nFileSizeLow;

src/Lucene.Net.Tests.Misc/Store/TestNativeUnixDirectory.cs renamed to src/Lucene.Net.Tests.Misc/Support/Store/TestNativeUnixDirectory.cs

File renamed without changes.

src/Lucene.Net.Tests.Misc/Store/TestWindowsDirectory.cs renamed to src/Lucene.Net.Tests.Misc/Support/Store/TestWindowsDirectory.cs

File renamed without changes.

0 commit comments

Comments
 (0)