OSFileStreamStrategy Seek Error is Confusing and not helpful.

[ProbablePrime]

Description

Hello, Apologies if I'm not doing anything correct here but let's get started!

I'm working on a complex system that processes files into more optimal formats for compression and storage, a bug in this code caused it to try to seek to a negative offset position within: OSFileStreamStrategy.Seek

When this occurs, FileStreamHelpers.ThrowInvalidArgument(_fileHandle); occurs.

Which throws a really unhelpful message: The parameter is incorrect. : <file path>.

This caused me to debug around thinking that my file stream was wrong, or that my input file was a corrupt image etc. When in actuality the image is fine, some code elsewhere was just trying to seek negatively.

Reproduction Steps

Attempt to seek a `OSFileStreamStrategy with a negative resultant value.

Seek(-1, SeekOrigin.Begin); should achieve this.

Expected behavior

A better exception message, would be very helpful for developers to more quickly comprehend what's going on.

Actual behavior

Explained in description

Regression?

I am not sure, there is a comment in the area: " // keep throwing the same exception we did when seek was causing actual offset change
" that implies there was a decision point at some point in time.

Known Workarounds

No response

Configuration

.NET 9, Windows 10, x64

Other information

No response

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

[KalleOlaviNiemitalo]

The exception throwing was changed in #49975 and again in #55191. Before those, I think the exception would have been thrown in this code path:

1. WindowsFileStreamStrategy.Seek:

   runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs

   Line 172 in 411c8a1

   long pos = SeekCore(_fileHandle, offset, origin);

2. WindowsFileStreamStrategy.SeekCore:

   runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/WindowsFileStreamStrategy.cs

   Line 192 in 411c8a1

   return _filePosition = FileStreamHelpers.Seek(fileHandle, _path, offset, origin, closeInvalidHandle);

3. FileStreamHelpers.SeekCore, where I presume SetFilePointerEx would fail with ERROR_INVALID_PARAMETER:

   runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.Windows.cs

   Lines 207 to 215 in 411c8a1

   	if (!Interop.Kernel32.SetFilePointerEx(handle, offset, out long ret, (uint)origin))
   	{
   	if (closeInvalidHandle)
   	{
   	throw Win32Marshal.GetExceptionForWin32Error(GetLastWin32ErrorAndDisposeHandleIfInvalid(handle), path);
   	}
   	else
   	{
   	throw Win32Marshal.GetExceptionForLastWin32Error(path);

4. Win32Marshal.GetExceptionForWin32Error:

   runtime/src/libraries/Common/src/System/IO/Win32Marshal.cs

   Lines 59 to 63 in 411c8a1

   	case Interop.Errors.ERROR_INVALID_PARAMETER:
   	default:
   	return new IOException(
   	string.IsNullOrEmpty(path) ? GetMessage(errorCode) : $"{GetMessage(errorCode)} : '{path}'",
   	MakeHRFromErrorCode(errorCode));

I don't see anything directly related in the PR comments, although #49975 (comment) requested not changing the type of the exception thrown by AsyncWindowsFileStreamStrategy.WriteAsyncInternalCore.

Changing OSFileStreamStrategy.Seek to throw ArgumentException instead of IOException would be considered a breaking change under the breaking change rules for exceptions. However, changing the Exception.Message string is explicitly allowed, and I think setting an ArgumentException as Exception.InnerException would also be allowed.

[ProbablePrime]

Thanks for doing the research, I took a look through it and can more clearly see what's going on with the Call flow here. I do see the issue with the exception types.

My hope here is for a more helpful message, It could have saved me about an hour of debugging. So I'm happy to see some possibilities.

[adamsitnik]

Hello @ProbablePrime

@KalleOlaviNiemitalo is right, we can't change the exception type (it would be a breaking change), but we can change the exception message.

I am going to mark it as help-wanted. Please feel free to send a PR with a fix.

Thanks!

[KalleOlaviNiemitalo]

I presume SetFilePointerEx would fail with ERROR_INVALID_PARAMETER

On .NET Framework 4.8.1, FileStream.Seek calls SetFilePointer, which actually fails with ERROR_NEGATIVE_SEEK = 131; Exception.HResult is then 0x80070083. This same error occurs if you do stream.Seek(long.MaxValue, SeekOrigin.Current) twice; Windows does not have a separate error code for file position overflow.

On .NET 6.0 and .NET 9.0, Exception.HResult is instead 0x80070057 = HRESULT_FROM_WIN32(ERROR_INVALID_PARAMETER).

So if the Windows implementation of FileStreamHelpers.ThrowInvalidArgument were changed to use ERROR_NEGATIVE_SEEK rather than ERROR_INVALID_PARAMETER, then Exception.Message would become easier to understand, and Exception.HResult would become more descriptive and match .NET Framework. In this case, FileStreamHelpers.ThrowInvalidArgument should be renamed to better indicate that it is only used for invalid seek. However, this change would not help on Linux, which does not have a dedicated error code for invalid seek.