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

Clarify shared file handler behavior of File::try_clone. #47958

Merged
merged 1 commit into from
Feb 5, 2018

Conversation

frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Feb 2, 2018

Fixes #46578.

@rust-highfive
Copy link
Collaborator

r? @aidanhs

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

src/libstd/fs.rs Outdated
/// references. Both handles will read and write with the same cursor
/// position.
/// Modifying one `File` (e.g. reading, writing, seeking) will result in the
/// underlying file handle of the other `File` also adjusting.
Copy link
Member Author

Choose a reason for hiding this comment

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

not a big fan of my wording here or in the previous section. lemme know if y'all have better suggestions

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something along the lines of:

This means that both File instance shares the same cursor; reads, writes and seeks will affect both instances simultaneously.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! Latest force push incorporates some of that wording

@abonander
Copy link
Contributor

I just realized this could be a bad footgun for someone using a cloned handle with BufReader<File>; the buffer won't know to flush and re-read after a seek or modification. Should we warn the reader about this?

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 2, 2018
@frewsxcv
Copy link
Member Author

frewsxcv commented Feb 3, 2018

I just realized this could be a bad footgun for someone using a cloned handle with BufReader; the buffer won't know to flush and re-read after a seek or modification. Should we warn the reader about this?

@abonander I think that might be a good idea. Any suggestions on how to word this?

@abonander
Copy link
Contributor

Something like

Note: BufReader Data Corruption

Using a cloned handle with BufReader could cause data corruption when the other handle is read from, seeked or written to, as the buffer won't know that the data it contains is stale.

--

However, I think this merits some discussion as BufReader could instead remember the cursor position from its previous read and check it against the current cursor position before yielding data out of the buffer. However, this would mean a syscall on every fill_buf() invocation (?). (Also if the other handle is read or written and then seeked back to the original position then the buffer wouldn't know the difference anyway.)

@abonander
Copy link
Contributor

We already have a warning about a similar footgun with BufReader.get_mut() so I don't think this requires special treatment.

@frewsxcv
Copy link
Member Author

frewsxcv commented Feb 3, 2018

Okay so it sounds like we might want to open a separate issue for that then? What do you think?

@aidanhs
Copy link
Member

aidanhs commented Feb 3, 2018

@bors r+ rollup

This looks good to me, and it sounds like the bufreader thing may need more discussion elsewhere.

@bors
Copy link
Contributor

bors commented Feb 3, 2018

📌 Commit d597da3 has been approved by aidanhs

@bors
Copy link
Contributor

bors commented Feb 3, 2018

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Feb 3, 2018

📌 Commit d597da3 has been approved by aidanhs

kennytm added a commit to kennytm/rust that referenced this pull request Feb 3, 2018
Clarify shared file handler behavior of File::try_clone.

Fixes rust-lang#46578.
bors added a commit that referenced this pull request Feb 3, 2018
Rollup of 9 pull requests

- Successful merges: #47753, #47862, #47877, #47896, #47912, #47944, #47947, #47978, #47958
- Failed merges:
kennytm added a commit to kennytm/rust that referenced this pull request Feb 4, 2018
Clarify shared file handler behavior of File::try_clone.

Fixes rust-lang#46578.
kennytm added a commit to kennytm/rust that referenced this pull request Feb 4, 2018
Clarify shared file handler behavior of File::try_clone.

Fixes rust-lang#46578.
bors added a commit that referenced this pull request Feb 4, 2018
Rollup of 10 pull requests

- Successful merges: #47862, #47877, #47896, #47912, #47947, #47958, #47978, #47996, #47999, #47892
- Failed merges:
bors added a commit that referenced this pull request Feb 4, 2018
Rollup of 10 pull requests

- Successful merges: #47862, #47877, #47896, #47912, #47947, #47958, #47978, #47996, #47999, #47892
- Failed merges:
@bors bors merged commit d597da3 into rust-lang:master Feb 5, 2018
@frewsxcv frewsxcv deleted the frewsxcv-try-clone branch February 11, 2018 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants