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

Add support for non-seekable files to RandomAccess #58506

Closed
wants to merge 7 commits into from

Conversation

adamsitnik
Copy link
Member

fixes #58381

@ghost
Copy link

ghost commented Sep 1, 2021

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

Issue Details

fixes #58381

Author: adamsitnik
Assignees: -
Labels:

area-System.IO

Milestone: -

@adamsitnik adamsitnik added this to the 7.0.0 milestone Sep 2, 2021
@@ -92,7 +117,7 @@ internal static unsafe void WriteAtOffset(SafeFileHandle handle, ReadOnlySpan<by
// the function to be used by FileStream for all the same situations.
int bytesWritten = handle.CanSeek ?
Interop.Sys.PWrite(handle, bufPtr, GetNumberOfBytesToWrite(buffer.Length), fileOffset) :
Interop.Sys.Write(handle, bufPtr, GetNumberOfBytesToWrite(buffer.Length));
Interop.Sys.Write(handle, bufPtr, buffer.Length);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub I've removed the call to GetNumberOfBytesToWrite which as you know is used only for testing partial writes in DEBUG.

With pipes|sockets, every read operation is blocked until there is some data available. Even if it's a zero byte read to an empty buffer.
I wanted to test the scenario where we write X bytes to the pipe and try to read Y bytes from it, where Y > X and ensure that the read returns X bytes.
With partial writes emulation provided by GetNumberOfBytesToWrite, the read was returning X / 2 bytes (because this is how much we have written). I could perform more reads using exact length, but in reality we usually don't know how many bytes are available in the pipe|socket. I could not read until read returns 0 because when there is no data available, the read is blocked and does not return 0 ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's trading off testing one thing for another thing. It's not clear to me that's the right trade off, but if you think so, ok.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep in mind that we are still testing partial writes for seekable files. I just don't think that we can test non-seekable files using the same approach, as they just block the read when there is no data available.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't think that we can test non-seekable files using the same approach, as they just block the read when there is no data available.

If the write and read are issued concurrently, it should be fine.

I could perform more reads using exact length

That's how reading generally works, issuing repeated reads until all data expected is consumed.

I wanted to test the scenario where we write X bytes to the pipe and try to read Y bytes from it, where Y > X and ensure that the read returns X bytes.

What guarantees it'll return exactly X bytes at the OS level?

@jeffhandley
Copy link
Member

@adamsitnik It looks like this PR is back in your court with review feedback that still needs to be addressed/resolved.

@adamsitnik adamsitnik closed this Oct 12, 2021
@stephentoub
Copy link
Member

Why is this being closed?

@dotnet dotnet locked as resolved and limited conversation to collaborators Nov 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relax RandomAccess type requirements, make all Read*|Write* methods work with non-seekable files
3 participants