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

File.ReadAllBytes* should support non-seekable files #58383

Closed
2 tasks
Tracked by #64596
adamsitnik opened this issue Aug 30, 2021 · 7 comments · Fixed by #58434
Closed
2 tasks
Tracked by #64596

File.ReadAllBytes* should support non-seekable files #58383

adamsitnik opened this issue Aug 30, 2021 · 7 comments · Fixed by #58434
Assignees
Labels
area-System.IO help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@adamsitnik
Copy link
Member

As of today, File.WriteAllBytes* APIs support non-seekable files, but File.ReadAllBytes* don't.

The following call to FileStream.Length is going to throw:

The funny thing is that we already almost support it:

else if (fileLength == 0)
{
#if !MS_IO_REDIST
// Some file systems (e.g. procfs on Linux) return 0 for length even when there's content.
// Thus we need to assume 0 doesn't mean empty.
return ReadAllBytesUnknownLength(fs);
#endif
}

We need:

  • test that shows the broken scenario
  • a fix
@adamsitnik adamsitnik added area-System.IO help wanted [up-for-grabs] Good issue for external contributors labels Aug 30, 2021
@adamsitnik adamsitnik added this to the 7.0.0 milestone Aug 30, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 30, 2021
@ghost
Copy link

ghost commented Aug 30, 2021

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

Issue Details

As of today, File.WriteAllBytes* APIs support non-seekable files, but File.ReadAllBytes* don't.

The following call to FileStream.Length is going to throw:

The funny thing is that we already almost support it:

else if (fileLength == 0)
{
#if !MS_IO_REDIST
// Some file systems (e.g. procfs on Linux) return 0 for length even when there's content.
// Thus we need to assume 0 doesn't mean empty.
return ReadAllBytesUnknownLength(fs);
#endif
}

We need:

  • test that shows the broken scenario
  • a fix
Author: adamsitnik
Assignees: -
Labels:

area-System.IO, up-for-grabs

Milestone: 7.0.0

@adamsitnik adamsitnik removed the untriaged New issue has not been triaged by the area owner label Aug 30, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 31, 2021
@lateapexearlyspeed
Copy link
Contributor

Hi @adamsitnik could you please help on question:
how to create non-seekable readable FileStream in Windows by calling path based new FileStream(path, FileMode.Open, FileAccess.Read, FileShare.Read, bufferSize: 1, FileOptions.SequentialScan) which is used in File.ReadAllBytes() ?
I can only create non-seekable FileStream by creating namedPipe then new FileStream(new SafeFileHandle(namedPipeServer.SafePipeHandle.DangerousGetHandle(), true), FileAccess.Read) which is code from test

@adamsitnik
Copy link
Member Author

@lateapexearlyspeed Appologies for the delay, I've missed GH notificaiton for your question.

You can take a look at the tests that I am trying to add in #58506 :

string pipeName = FileSystemTest.GetNamedPipeServerStreamName();
string pipePath = Path.GetFullPath($@"\\.\pipe\{pipeName}");
using (var server = new NamedPipeServerStream(pipeName, PipeDirection.InOut))
using (SafeFileHandle clientHandle = File.OpenHandle(pipePath, FileMode.Open, access, FileShare.None, FileOptions.Asynchronous))

in your case you should be able to create a FileStream from given path. Sth like this:

string pipeName = FileSystemTest.GetNamedPipeServerStreamName();
string pipePath = Path.GetFullPath($@"\\.\pipe\{pipeName}");

using (var server = new NamedPipeServerStream(pipeName, PipeDirection.InOut))
using (FileStream fs = new FileStream(pipePath, FileMode.Open, access, FileShare.None, FileOptions.Asynchronous))

@lateapexearlyspeed
Copy link
Contributor

@adamsitnik Thanks for help, test cases finished in PR, sync version test for File.ReadAllBytes() passed but async version test for File.ReadAllBytesAsync() failed, with exception:

System.IO.IOException : The parameter is incorrect. : '.\pipe\a0b5cd9498c54bb4a6ea30fd57edf8d8'
at System.IO.File.InternalReadAllBytesUnknownLengthAsync(FileStream fs, CancellationToken cancellationToken) in //src/libraries/System.Private.CoreLib/src/System/IO/File.cs:line 812
at System.IO.Tests.File_ReadWriteAllBytesAsync.ReadAllBytesAsync_NonSeekableFileStream() in /
/src/libraries/System.IO.FileSystem/tests/File/ReadWriteAllBytesAsync.cs:line 202
--- End of stack trace from previous location ---

which can be reproduced by following call:

            string pipeName = FileSystemTest.GetNamedPipeServerStreamName();
            string pipePath = Path.GetFullPath($@"\\.\pipe\{pipeName}");

            var namedPipeWriterStream = new NamedPipeServerStream(pipeName, PipeDirection.Out);
            var fs = new FileStream(pipePath, FileMode.Open, FileAccess.Read, FileShare.Read, bufferSize: 1, FileOptions.Asynchronous | FileOptions.SequentialScan);
            await fs.ReadAsync(new byte[3].AsMemory()); // Here will throw same exception as test case throws

Not sure is there issue inside FileStream for async operation ?

@adamsitnik
Copy link
Member Author

Not sure is there issue inside FileStream for async operation ?

Most likely. I am going to build your fork and try to repro it locally

@adamsitnik
Copy link
Member Author

It turned out that the official docs were not telling the exact truth.

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

I've added an handle.CanSeek condition to your PR (#58434) and the tests are passing now.

cc @stephentoub

@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Sep 14, 2021
@lateapexearlyspeed
Copy link
Contributor

Hi @adamsitnik I marked PR as ready for review now.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants