[release/6.0] for non-seekable file handles, the OVERLAPPED offset must be set to 0

[adamsitnik]

Customer Impact

Fixes a bug reported by customer via email, where an async read operation performed on a FileStream opened for non-seekable device file was throwing an System.IO.IOException: "The parameter is incorrect".

It turns out that ReadFile doc was wrong:

For an hFile that does not support byte offsets, Offset and OffsetHigh are ignored.

And OVERLAPPED doc was right:

This member is nonzero only when performing I/O requests on a seeking device that supports the concept of an offset (also referred to as a file pointer mechanism), such as a file. Otherwise, this member must be zero.

Testing

We already had a test for that, but it's part of Outerloop and that is why the regression was missed:

      System.IO.Tests.DeviceInterfaceTests.DeviceInterfaceCanBeOpenedForAsyncIO [FAIL]
        System.IO.IOException : The parameter is incorrect. : '\\?\hid#vid_0b0e&pid_24c8&mi_03&col02#b&92ae56&0&0001#{4d1e55b2-f16f-11cf-88cb-001111000030}'
        Stack Trace:
          D:\projects\runtime\src\libraries\System.IO.FileSystem\tests\FileStream\FileStreamConformanceTests.Windows.cs(209,0): at System.IO.Tests.DeviceInterfaceTests.DeviceInterfaceCanBeOpenedForAsyncIO()
          --- End of stack trace from previous location ---
    Finished:    System.IO.FileSystem.Tests

The test was used to ensure that it's passing now.

Risk

Very low.

[ghost]

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

Issue Details

---

Customer Impact

Fixes a bug reported by customer via email, where an async read operation performed on a FileStream opened for non-seekable device file was throwing an System.IO.IOException: "The parameter is incorrect".

It turns out that ReadFile doc was wrong:

For an hFile that does not support byte offsets, Offset and OffsetHigh are ignored.

And OVERLAPPED doc was right:

This member is nonzero only when performing I/O requests on a seeking device that supports the concept of an offset (also referred to as a file pointer mechanism), such as a file. Otherwise, this member must be zero.

Testing

We already had a test for that, but it's part of Outerloop and that is why the regression was missed:

      System.IO.Tests.DeviceInterfaceTests.DeviceInterfaceCanBeOpenedForAsyncIO [FAIL]
        System.IO.IOException : The parameter is incorrect. : '\\?\hid#vid_0b0e&pid_24c8&mi_03&col02#b&92ae56&0&0001#{4d1e55b2-f16f-11cf-88cb-001111000030}'
        Stack Trace:
          D:\projects\runtime\src\libraries\System.IO.FileSystem\tests\FileStream\FileStreamConformanceTests.Windows.cs(209,0): at System.IO.Tests.DeviceInterfaceTests.DeviceInterfaceCanBeOpenedForAsyncIO()
          --- End of stack trace from previous location ---
    Finished:    System.IO.FileSystem.Tests

The test was used to ensure that it's passing now.

Risk

Very low.

Author:	adamsitnik
Assignees:	-
Labels:	Servicing-consider, area-System.IO
Milestone:	6.0.0

[stephentoub]

We already had a test for that, but it's part of Outerloop and that is why the regression was missed

When did it regress? Does this mean we have a gap in our outerloop story? I'd have expected to have an issue opened by now about it, unless the regression was super recent.

[adamsitnik]

When did it regress?

On 1st of August (RC2): e176fdf#diff-6a552da0cf26a4ac2ec6b2c4b314249db089d46519e1af7efc21940a2acce450

Does this mean we have a gap our outerloop story?

Before we started the rewrite, we basically had 0 tests that were testing async file IO for non-seekable files opened for async IO. We have added some, the work and other gaps are tracked here: #54495

One of the reasons why it's hard to test is the lack of ability to easily copy ThreadPoolBoundHandle from an alread initialized async pipe handle (that needs to call some pipe-specific sys-calls to connect and start a server) to SafeFileHandle: #28585 (comment)

I'd have expected to have an issue opened by now about it, unless the regression was super recent.

Until #54676 it was impossible to open such files using FileStream(path), users had to open the handle on their own and pass to the FileStream ctor that accepts handles. I would say it's a very niche scenario.

[stephentoub]

Thanks, but what I'm trying to understand is, you said:
"We already had a test for that, but it's part of Outerloop and that is why the regression was missed"
If we already had a test for that, and that test was added 4 months ago, and this regressed 2months ago, and that test has been failing since, why didn't we know about this issue? We're supposed to have regular outerloop runs that generate issues if things fail.

This PR isn't adding or enabling any tests, which suggests we're expecting existing tests to cover this, but we were unaware of any failures in such tests.

[adamsitnik]

If we already had a test for that, and that test was added 4 months ago, and that test has been failing, why didn't we know about this issue? We're supposed to have regular outerloop runs that generate issues if things fail.

Thanks for the explanation and apologies for the confusion.

The test tries to find a device that we can access for reading, but if there are no such devices, it does nothing:

runtime/src/libraries/System.IO.FileSystem/tests/FileStream/FileStreamConformanceTests.Windows.cs

Lines 192 to 195 in a7a9448

	if (fileStream is null)
	{
	// it's OK to not have any such devices available
	// this test is just best effort

So most likely our CI machines have no such devices available and we need better test(s).

[adamsitnik]

This PR isn't adding or enabling any tests, which suggests we're expecting existing tests to cover this, but we were unaware of any failures in such tests.

Good point, I'll am going to add a test for async pipes tomorrow.

[danmoseley]

ReadFile doc was wrong:

Should we also offer a PR to the ReadFile doc?

Also, is there any value here in reaching out to Windows IO dev to confirm the behavior is expected by them, and won't change? edit: never mind, it's clearly safe to change from an arbitrary value to zero.

[jeffhandley]

There was an email thread about this PR. We will close this PR, submit a fix for this issue in main, and consider porting this fix to release/6.0 in a servicing release. Before committing this into release/6.0 though, we need successful test evidence off main, including current and new coverage.

[danmoseley]

@PriyaPurkayastha this is a known issue in 6.0.0 that we expect to fix in 6.0.1. Do we document such things?

[PriyaPurkayastha]

@PriyaPurkayastha this is a known issue in 6.0.0 that we expect to fix in 6.0.1. Do we document such things?

Yes, we should document it as a Known issue and https://github.com/dotnet/core/blob/main/release-notes/6.0/known-issues.md is the location that @rbhanda uses.

[danmoseley]

@jeffhandley this came up in tactics and was approved for 6.0.1. So up to you guys whether you reuse this PR (+test) or create another. I know you want to try it in main first, and the 6.0.1. submit window isn't open for at least a week anyway.