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

Update BufWriter example to include call to flush() #63410

Merged
merged 3 commits into from
Sep 1, 2019

Conversation

johnterickson
Copy link
Contributor

I was playing with a writing a Huffman encoder/decoder and was getting weird corruptions and truncations. I finally realized it was was because BufWriter was swallowing write errors 😬. I've found Rust to generally be explicit and err on the safe side, so I definitely found this unintuitive and not "rustic".

https://twitter.com/johnterickson/status/1159514988123312128

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 9, 2019
src/libstd/io/buffered.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Aug 9, 2019

@bors rollup

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-09T14:01:21.3488780Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-09T14:01:21.3678565Z ##[command]git config gc.auto 0
2019-08-09T14:01:21.3763634Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-09T14:01:21.3814433Z ##[command]git config --get-all http.proxy
2019-08-09T14:01:21.3963911Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63410/merge:refs/remotes/pull/63410/merge
---
2019-08-09T14:01:56.4839570Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-09T14:01:56.4839601Z 
2019-08-09T14:01:56.4839818Z   git checkout -b <new-branch-name>
2019-08-09T14:01:56.4839849Z 
2019-08-09T14:01:56.4839917Z HEAD is now at dc5009d6a Merge a801a8ff4833c5e69bb3ab4881ffa1462636c9a5 into 813a3a5d4b2be4e2101faa73a067da02a704a598
2019-08-09T14:01:56.5054491Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-09T14:01:56.5061814Z ==============================================================================
2019-08-09T14:01:56.5061890Z Task         : Bash
2019-08-09T14:01:56.5061935Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-09T14:08:15.2569430Z    Compiling serde_json v1.0.40
2019-08-09T14:08:19.7642873Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-08-09T14:08:28.8692642Z     Finished release [optimized] target(s) in 1m 33s
2019-08-09T14:08:28.8770780Z tidy check
2019-08-09T14:08:29.4050233Z tidy error: /checkout/src/libstd/io/buffered.rs:368: trailing whitespace
2019-08-09T14:08:29.4050644Z tidy error: /checkout/src/libstd/io/buffered.rs:401: trailing whitespace
2019-08-09T14:08:31.3139368Z some tidy checks failed
2019-08-09T14:08:31.3140129Z 
2019-08-09T14:08:31.3140129Z 
2019-08-09T14:08:31.3141147Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-08-09T14:08:31.3141319Z 
2019-08-09T14:08:31.3141345Z 
2019-08-09T14:08:31.3141394Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-08-09T14:08:31.3141464Z Build completed unsuccessfully in 0:01:37
2019-08-09T14:08:31.3141464Z Build completed unsuccessfully in 0:01:37
2019-08-09T14:08:33.2944453Z ##[error]Bash exited with code '1'.
2019-08-09T14:08:33.2985989Z ##[section]Starting: Checkout
2019-08-09T14:08:33.2987641Z ==============================================================================
2019-08-09T14:08:33.2987853Z Task         : Get sources
2019-08-09T14:08:33.2987921Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Freaky
Copy link
Contributor

Freaky commented Aug 9, 2019

There's a similar issue with Rust's handling of close(2). As per the Linux man page:

A careful programmer will check the return value of close(), since it
is quite possible that errors on a previous write(2) operation are
reported only on the final close() that releases the open file
description. Failing to check the return value when closing a file
may lead to silent loss of data. This can especially be observed
with NFS and with disk quota.

A careful programmer who wants to know about I/O errors may precede
close() with a call to fsync(2).

Drop ignores errors in close(2), and we don't have the Close trait yet, but we can do into_inner()?.sync_all().

@joshtriplett
Copy link
Member

By all means, we should check for errors in close, but under no circumstances should the normal file APIs generate syncs that the programmer didn't request. Higher-level file or database APIs may wish to, but low-level file APIs should not. Write buffering is a feature.

@Freaky
Copy link
Contributor

Freaky commented Aug 12, 2019

Nobody suggested that, I'm just saying this is still misleading:

Calling ['flush'] ensures that the buffer is empty and all errors have been observed.

People shouldn't have the wrong idea about what flush does - if you care to observe all errors you need to sync.

@johnterickson
Copy link
Contributor Author

@Freaky @joshtriplett I updated the wording based on your comments, but frankly I'm a little out of my element here. I'm more familiar with Windows I/O - which has its own set of idiosyncrasies 🤣

Bottom line, feel free to propose verbiage.

@Alexendoo
Copy link
Member

Ping from triage, any updates? @joshtriplett

@hdhoang
Copy link
Contributor

hdhoang commented Aug 30, 2019

second ping from triage, nominating another reviewer. @Centril could you review this? Thanks

@joshtriplett
Copy link
Member

Sorry for the delayed response; had a busy week.

This looks reasonable to me. One nit: please fold the commit removing trailing whitespace into the commit that added that whitespace. With that change, r=me.

@johnterickson
Copy link
Contributor Author

Thanks, @joshtriplett - I rebased off master, squashing those two commits.

@joshtriplett
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 31, 2019

📌 Commit 1b94610 has been approved by joshtriplett

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 1, 2019
Update BufWriter example to include call to flush()

I was playing with a writing a Huffman encoder/decoder and was getting weird corruptions and truncations.  I finally realized it was was because `BufWriter` was swallowing write errors 😬.  I've found Rust to generally be explicit and err on the safe side, so I definitely found this unintuitive and not "rustic".

https://twitter.com/johnterickson/status/1159514988123312128
bors added a commit that referenced this pull request Sep 1, 2019
Rollup of 4 pull requests

Successful merges:

 - #63410 (Update BufWriter example to include call to flush())
 - #64032 (rustdoc use -Ccodegen-units=1 by default for test compile)
 - #64039 (Update sync condvar doc style)
 - #64042 (Fix word repetition in str documentation)

Failed merges:

r? @ghost
Centril added a commit to Centril/rust that referenced this pull request Sep 1, 2019
Update BufWriter example to include call to flush()

I was playing with a writing a Huffman encoder/decoder and was getting weird corruptions and truncations.  I finally realized it was was because `BufWriter` was swallowing write errors 😬.  I've found Rust to generally be explicit and err on the safe side, so I definitely found this unintuitive and not "rustic".

https://twitter.com/johnterickson/status/1159514988123312128
bors added a commit that referenced this pull request Sep 1, 2019
Rollup of 5 pull requests

Successful merges:

 - #63410 (Update BufWriter example to include call to flush())
 - #64029 (Account for rounding errors when deciding the diagnostic boundaries)
 - #64032 (rustdoc use -Ccodegen-units=1 by default for test compile)
 - #64039 (Update sync condvar doc style)
 - #64042 (Fix word repetition in str documentation)

Failed merges:

r? @ghost
@bors bors merged commit 1b94610 into rust-lang:master Sep 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants