std: Fix partial writes in LineWriter

[alexcrichton]

Previously the LineWriter could successfully write some bytes but then fail to
report that it has done so. Additionally, an erroneous flush after a successful
write was permanently ignored. This commit fixes these two issues by (a)
maintaining a need_flush flag to indicate whether a flush should be the first
operation in LineWriter::write and (b) avoiding returning an error once some
bytes have been successfully written.

Closes #37807

[rust-highfive]

r? @brson

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

[Stebalien]

Previously the LineWriter could successfully write some bytes but then fail to
report that it has done so.

Previously as in before #32107 (or am I missing something)?

[Stebalien]

Really, we should probably avoid flushing unless the last character written was a newline (that's kind of the point of this wrapper). Unfortunately, that could lead to a pathological error swallowing case if we keep making small, partial writes that never finish on a newline.

(Currently, we can end up flushing on partial writes even if we haven't written the newline yet.)

[alexcrichton]

@Stebalien oh I meant previously as in just before this commit. I think this was likely accidental but if this write fails then we didn't report the successfully written bytes earlier.

Really, we should probably avoid flushing unless the last character written was a newline

Isn't that essentially what happens here? After a successful write of data which may contain a newline we guarantee a flush afterwards?

[alexcrichton]

Er well so there's two conditions after the partial write:

• Did we write all the data?
• Did the flush succeed?

If the former is false then we may have written a newline (we don't know unless we search). Do you think in the case of a partial write we should search for a newline to see if we should actually flush? It seems like a rare enough case that somewhat eager flushing would also be fine...

[Stebalien]

@Stebalien oh I meant previously as in just before this commit. I think this was likely accidental but if this write fails then we didn't report the successfully written bytes earlier.

head -> desk: I completely missed that.

If the former is false then we may have written a newline (we don't know unless we search). Do you think in the case of a partial write we should search for a newline to see if we should actually flush? It seems like a rare enough case that somewhat eager flushing would also be fine...

After thinking about it, your way is probably best. The only way to get a partial write from a BufWriter is if we try to write more than the inner buffer can hold in one go (so, we won't end up flushing for small writes). In that case, the BufWriter will write directly to the wrapped writer (bypassing the buffer). A subsequent flush won't flush anything out of the internal buffer (it will have been flushed before the direct write) and instead just call flush on the wrapped writer. In the normal case where the wrapped writer is some kind of os stream (e.g. a file or socket), the flush will be a no-op.

TL;DR:

1. The extra flush will only happen for very large writes.
2. It will usually be a no-op (the buffer will already have been automatically flushed by the BufWriter).

[alexcrichton]

Makes sense to me!

[brson]

@bors r+

It's a clever patch. Nice work.

[bors]

📌 Commit ecc6010 has been approved by brson

[bors]

⌛ Testing commit ecc6010 with merge d86cf13...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-cargotest, auto-linux-64-cross-freebsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild
Approved by: brson
Pushing d86cf13 to master...