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: parameterize file position #6381

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MarcusSorealheis
Copy link

@MarcusSorealheis MarcusSorealheis commented Mar 6, 2024

Motivation

Closes: #6374

Solution

  1. We make the std parameter mutable by adding mut before it: mut std: StdFile.
  2. We call std.seek(SeekFrom::Current(0)) to get the current position of the file.
  3. The SeekFrom::Current(0) ensures that the file pointer is not modified, and the method returns the current position.
  4. We use the position returned by seek to initialize the pos field of the Inner struct within the File struct.

By initializing the pos field with the correct position from the StdFile, the File struct created by from_std will maintain the same seek position as the original StdFile. I'm looking into expanding the existing test to validate no regressions introduced.

@MarcusSorealheis MarcusSorealheis marked this pull request as draft March 6, 2024 00:19
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-fs Module: tokio/fs labels Mar 6, 2024
@Darksonn
Copy link
Contributor

Darksonn commented Mar 6, 2024

Thank you. My main concern here is how long do these calls take. We offload all other file operations to spawn_blocking, since they may take a long time. It's possible that this particular syscall never takes a long time, but I don't know ...

That said, I acknowledge that doing this is more difficult if we have to offload it to spawn_blocking. We would probably have to postpone the position lookup until the first operation on the file, since from_std is not async, so we can't do it there.

impl Seek for &'_ MockFile {
impl Seek for MockFile {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to cause a compilation failure. Most likely, you can just undo the change.

Copy link
Author

Choose a reason for hiding this comment

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

My apologies. That was a mistake. I'm currently working through/experimenting offloading to spawn_blocking

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. Hopefully it isn't too complicated to do that. What you're doing now may be preferable if offloading is very complicated.

Copy link
Author

Choose a reason for hiding this comment

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

I will resolve it in just a moment.

tokio/src/fs/file.rs Outdated Show resolved Hide resolved
ohh, good thought. Thank you.

Co-authored-by: Alice Ryhl <aliceryhl@google.com>
@MarcusSorealheis
Copy link
Author

I'm trying to get Clippy working on my rig.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks! Your code looks good. Can you add a test for this? Then I would be happy to merge it.

@Darksonn
Copy link
Contributor

Darksonn commented Sep 9, 2024

Actually, it unfortunately seems like there are a bunch of test failures.

@MarcusSorealheis
Copy link
Author

I will get these tests and failures sorted out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-fs Module: tokio/fs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tokio::fs::File::from_std() assumes current file position is zero
2 participants