Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/6.0.1] Support async operations on unseekable files #61410

Merged
merged 1 commit into from
Nov 12, 2021

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Nov 10, 2021

This is a manual backport of #58434, the problem has been recently discussed for #60931 which has not been merged to 6.0 as it was simply too late.

Customer Impact

This PR actually fixes two bugs.

The first one is 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.

The second bug is #58383. To tell the long story short, File.ReadAllBytes*(string path, ...) methods were assuming that path can't point to a non-seekable file like named pipe.

Testing

The fix was verified using tests added in the PR. It has been in main for a month (#58434) and we have already been doing that for sync file handles (the issue was originally discovered and fixed by @stephentoub):

private static NativeOverlapped GetNativeOverlappedForSyncHandle(SafeFileHandle handle, long fileOffset)
{
Debug.Assert(!handle.IsAsync);
NativeOverlapped result = default;
if (handle.CanSeek)
{
result.OffsetLow = unchecked((int)fileOffset);
result.OffsetHigh = (int)(fileOffset >> 32);
}

Risk

Very low. :shipit:

Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@ghost
Copy link

ghost commented Nov 10, 2021

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

This PR actually fixes two bugs.

The first one is 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.

The second bug is #58383. To tell the long story short, File.ReadAllBytes*(string path, ...) methods were assuming that path can't point to a non-seekable file like named pipe.

Testing

The fix was verified using tests added in the PR. It has been in main for a month (#58434) and we have already been doing that for sync file handles (the issue was originally discovered and fixed by @stephentoub):

private static NativeOverlapped GetNativeOverlappedForSyncHandle(SafeFileHandle handle, long fileOffset)
{
Debug.Assert(!handle.IsAsync);
NativeOverlapped result = default;
if (handle.CanSeek)
{
result.OffsetLow = unchecked((int)fileOffset);
result.OffsetHigh = (int)(fileOffset >> 32);
}

Risk

Very low. :shipit:

Author: adamsitnik
Assignees: -
Labels:

area-System.IO

Milestone: 6.0.1

@danmoseley danmoseley added the Servicing-consider Issue for next servicing release review label Nov 10, 2021
@jamshedd jamshedd added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 11, 2021
@jamshedd
Copy link
Member

Approved for Dec - 6.0.1

@danmoseley
Copy link
Member

@adamsitnik approved for 6.0.1, but if it's feasible to ask the customer to validate (presumably by picking up current main) that would be ideal.

@danmoseley danmoseley closed this Nov 11, 2021
@danmoseley danmoseley reopened this Nov 11, 2021
@Anipik Anipik merged commit 278057b into dotnet:release/6.0 Nov 12, 2021
@adamsitnik adamsitnik deleted the fixNonSeekableFiles branch November 12, 2021 17:36
@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants