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

Implement read_offset and write_offset #35704

Merged
merged 7 commits into from
Oct 15, 2016
Merged

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Aug 16, 2016

These functions allow to read from and write to a file from multiple
threads without changing the per-file cursor, avoiding the race between
the seek and the read.

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@sfackler sfackler added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 16, 2016
@@ -348,6 +348,18 @@ impl File {
inner: self.inner.duplicate()?
})
}

/// Reads a number of bytes starting from a given offset.
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 specifically an offset from the start of the file, not the current cursor position, right? Could you add that to the docs?

@alexcrichton
Copy link
Member

Can you also be sure to add tests for this? Especially with how it relates to the "current file pointer" and such

@tbu-
Copy link
Contributor Author

tbu- commented Aug 22, 2016

Added docs and tests.

@alexcrichton
Copy link
Member

Discussed during @rust-lang/libs triage today the conclusion was that we probably want to call these {read,write}_at but beyond that they're good to go!

Could you also file a tracking issue and update the tracking issue here?

@tbu-
Copy link
Contributor Author

tbu- commented Aug 23, 2016

Done.

@sfackler
Copy link
Member

@bors r+ 4e48924

eddyb added a commit to eddyb/rust that referenced this pull request Aug 23, 2016
Implement `read_offset` and `write_offset`

These functions allow to read from and write to a file from multiple
threads without changing the per-file cursor, avoiding the race between
the seek and the read.
eddyb added a commit to eddyb/rust that referenced this pull request Aug 23, 2016
Implement `read_offset` and `write_offset`

These functions allow to read from and write to a file from multiple
threads without changing the per-file cursor, avoiding the race between
the seek and the read.
@bors
Copy link
Contributor

bors commented Aug 23, 2016

⌛ Testing commit 4e48924 with merge 6ba6e27...

@bors
Copy link
Contributor

bors commented Aug 23, 2016

💔 Test failed - auto-linux-64-cross-freebsd

@eddyb
Copy link
Member

eddyb commented Aug 25, 2016

@bors retry

@bors
Copy link
Contributor

bors commented Aug 25, 2016

⌛ Testing commit 4e48924 with merge 2d35fe1...

@bors
Copy link
Contributor

bors commented Aug 25, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@tbu-
Copy link
Contributor Author

tbu- commented Aug 25, 2016

This failure is definitely my fault.

@tbu-
Copy link
Contributor Author

tbu- commented Aug 25, 2016

@alexcrichton New situation, the Windows version of this differs from the Unix one: WriteFile updates the file pointer, pwrite doesn't.

Do we still want this and document the difference between platforms?

I don't see a way to emulate either platform's behavior on the other one atomically (atomicity is what this function is about).

@sfackler
Copy link
Member

That seems like a pretty significant platform difference. :(

@alexcrichton
Copy link
Member

Yeah that seems significant enough that we wouldn't provide a cross platform API for this. Perhaps a platform-specific one in std::os::*::fs, though.

@ranma42
Copy link
Contributor

ranma42 commented Aug 29, 2016

@tbu- my understanding of the Windows API is that it would be possible to avoid the update of the file pointer if the file had been opened with the flag FILE_FLAG_OVERLAPPED. This seems to be required anyway if multiple threads are accessing the file concurrently (without this flag, the operations would happen sequentially).

Would it be acceptable to only allow read_offset and write_offset on OVERLAPPED files on Windows?

@arthurprs
Copy link
Contributor

@ranma42 do we have a way to open files with OVERLAPPED flag using just the stdlib?

@ranma42
Copy link
Contributor

ranma42 commented Sep 4, 2016

@arthurprs Right now the OVERLAPPED flag is available through the Windows-specific extension for OpenOption: #27720

This is already used in the stdlib to open pipes as overlapped files:

opts.attributes(c::FILE_FLAG_OVERLAPPED);

@tbu-
Copy link
Contributor Author

tbu- commented Oct 12, 2016

Lints should be the last stage before tests, fixed.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 12, 2016

📌 Commit 744aecf has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 12, 2016

⌛ Testing commit 744aecf with merge 2481c23...

@bors
Copy link
Contributor

bors commented Oct 12, 2016

💔 Test failed - auto-linux-64-x-android-t

@tbu-
Copy link
Contributor Author

tbu- commented Oct 12, 2016

It seems the signature of ftruncate in libc is wrong, or the wrong signature is being used here (I removed the local definition of ftruncate. What is the preferred course of action?

Is off_t always i64 on Android and we don't need this fallback? Is it only sometimes i64, and we should only use this signature when it's not? Or is the definition in libc completely wrong, and it's always i32?

@alexcrichton
Copy link
Member

IIRC it's because this is an older version of Android compatibility. I think newer versions changed the definition of ftruncate (e.g. the ABI changed). I forget the exact details here though. For now seems fine to just leave it.

@tbu-
Copy link
Contributor Author

tbu- commented Oct 14, 2016

I changed it to only look for {ftruncate,pread,pwrite}64 on 32 bit systems, on 64 bit systems, off_t should be i64.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 14, 2016

📌 Commit 94aa08b has been approved by alexcrichton

bors added a commit that referenced this pull request Oct 14, 2016
Implement `read_offset` and `write_offset`

These functions allow to read from and write to a file from multiple
threads without changing the per-file cursor, avoiding the race between
the seek and the read.
@bors
Copy link
Contributor

bors commented Oct 14, 2016

⌛ Testing commit 94aa08b with merge 8c09454...

@bors
Copy link
Contributor

bors commented Oct 14, 2016

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 14, 2016

📌 Commit 1554993 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 15, 2016

⌛ Testing commit 1554993 with merge e1b6777...

bors added a commit that referenced this pull request Oct 15, 2016
Implement `read_offset` and `write_offset`

These functions allow to read from and write to a file from multiple
threads without changing the per-file cursor, avoiding the race between
the seek and the read.
@bors bors merged commit 1554993 into rust-lang:master Oct 15, 2016
@tbu-
Copy link
Contributor Author

tbu- commented Oct 15, 2016

Finally! Thanks for your patience, @alexcrichton.

@tbu-
Copy link
Contributor Author

tbu- commented Oct 28, 2016

Test.

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 28, 2024
Currently the documentation of `FileExt::seek_write` on Windows
indicates that writes beyond the end of the file leave intermediate
bytes uninitialized. This commentary dates back to the original
inclusion of these functions in rust-lang#35704 (wow blast from the past!). At
the time the functionality here was implemented using `WriteFile`, but
nowadays the `NtWriteFile` method is used instead. The documentation for
`NtWriteFile` explicitly states:

> If Length and ByteOffset specify a write operation past the current
> end-of-file mark, NtWriteFile automatically extends the file and updates
> the end-of-file mark; any bytes that are not explicitly written between
> such old and new end-of-file marks are defined to be zero.

This commentary has had a downstream impact in the `system-interface`
crate where it tries to handle this by explicitly writing zeros, but I
don't believe that's necessary any more. I'm sending a PR upstream here
to avoid future confusion and codify that zeros are written in the
intermediate bytes matching what Windows currently provides.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jan 30, 2024
…rite-docs, r=ChrisDenton

std: Update documentation of seek_write on Windows

Currently the documentation of `FileExt::seek_write` on Windows indicates that writes beyond the end of the file leave intermediate bytes uninitialized. This commentary dates back to the original inclusion of these functions in rust-lang#35704 (wow blast from the past!). At the time the functionality here was implemented using `WriteFile`, but nowadays the `NtWriteFile` method is used instead. The documentation for `NtWriteFile` explicitly states:

> If Length and ByteOffset specify a write operation past the current
> end-of-file mark, NtWriteFile automatically extends the file and updates
> the end-of-file mark; any bytes that are not explicitly written between
> such old and new end-of-file marks are defined to be zero.

This commentary has had a downstream impact in the `system-interface` crate where it tries to handle this by explicitly writing zeros, but I don't believe that's necessary any more. I'm sending a PR upstream here to avoid future confusion and codify that zeros are written in the intermediate bytes matching what Windows currently provides.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2024
Rollup merge of rust-lang#120452 - alexcrichton:update-windows-seek-write-docs, r=ChrisDenton

std: Update documentation of seek_write on Windows

Currently the documentation of `FileExt::seek_write` on Windows indicates that writes beyond the end of the file leave intermediate bytes uninitialized. This commentary dates back to the original inclusion of these functions in rust-lang#35704 (wow blast from the past!). At the time the functionality here was implemented using `WriteFile`, but nowadays the `NtWriteFile` method is used instead. The documentation for `NtWriteFile` explicitly states:

> If Length and ByteOffset specify a write operation past the current
> end-of-file mark, NtWriteFile automatically extends the file and updates
> the end-of-file mark; any bytes that are not explicitly written between
> such old and new end-of-file marks are defined to be zero.

This commentary has had a downstream impact in the `system-interface` crate where it tries to handle this by explicitly writing zeros, but I don't believe that's necessary any more. I'm sending a PR upstream here to avoid future confusion and codify that zeros are written in the intermediate bytes matching what Windows currently provides.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants