io: soften ‘at most one write attempt’ requirement in io::Write::write

[mina86]

At the moment, documentation of std::io::Write::write indicates that
call to it ‘represents at most one attempt to write to any wrapped
object’. It seems that such wording was put there to contrast it with
pre-1.0 interface which attempted to write all the data (it has since
been changed in RFC 517).

However, the requirement puts unnecessary constraints and may
complicate adaptors which perform non-trivial transformations on the
data. For example, they may maintain an internal buffer which needs
to be written out before the write method accepts more data. It might
be natural to code the method such that it flushes the buffer and then
grabs another chunk of user data. With the current wording in the
documentation, the adaptor would be forced to return Ok(0).

This commit softens the wording such that implementations can choose
code structure which makes most sense for their particular use case.

While at it, elaborate on the meaning of Ok(0) return pointing out
that the write_all methods interprets it as an error.

[rustbot]

r? @Mark-Simulacrum

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

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[mina86]

@rustbot label +T-libs-api -T-libs
cc @alexcrichton

[the8472]

it has since been changed in RFC 517

Which part? searching through the RFC I have found

The biggest change here is to the semantics of write. Instead of repeatedly writing to the underlying IO object until all of buf is written, it attempts a single write and on success returns the number of bytes written.

My understanding is that the at most once guarantee allows transitive atomicity guarantees. If you have a bunch of wrappers (e.g. progress reporting or line-buffering) around File and the underlying OS guarantees atomic append (linux does up to the page size) then you won't get tearing because some intermediate writer misbehaves. This can be important for logging.

Another issue is the return value. Either you get an Ok() with the bytes written or an Err() which indicates the write didn't happen and (for some errors) you can retry it. If some adapter splits the write and writes some bytes and then encounters an error it would have to bubble that up, but then it can't report the bytes. So without this guarantee the caller doesn't know how many bytes it has to retry.

[the8472]

Furthermore, it removes statement that returning Ok(0) means that
the object is likely no longer able to accept data. With adaptor
types this is often untrue and even with system-level file descriptors
I couldn’t find documentation suggesting that write may return zero in
such cases.

The default impl for write_all turns this into an error, so this would be a breaking change.

rust/library/std/src/io/mod.rs

Lines 1539 to 1548 in cef633d

	#[stable(feature = "rust1", since = "1.0.0")]
	fn write_all(&mut self, mut buf: &[u8]) -> Result<()> {
	while !buf.is_empty() {
	match self.write(buf) {
	Ok(0) => {
	return Err(error::const_io_error!(
	ErrorKind::WriteZero,
	"failed to write whole buffer",
	));
	}

Prior discussion: #56889 (comment)

[mina86]

The biggest change here is to the semantics of write. Instead of
repeatedly writing to the underlying IO object until all of buf is
written, it attempts a single write and on success returns the
number of bytes written.

Yep, that’s the part I was referring to.

My understanding is that the at most once guarantee allows
transitive atomicity guarantees. If you have a bunch of wrappers
around File and the underlying OS guarantees atomic append then
you won't get tearing because some intermediate writer
misbehaves.

But that’s the problem of those intermediate writers. If Linux
guarantees atomic append up to a page, I can easily write an adaptor
conforming to the Write interface which buffers up to two pages.
Requirement of at most one write does not guarantee that atomicity
properties of wrapped objects are preserved.

If some adapter splits the write and writes some bytes and then
encounters an error it would have to bubble that up, but then it
can't report the bytes.

Yes, that adaptor would need to store the error, return
Ok(bytes_written) and return Err(stored_error) on next write. For
some writers that may make sense. The ‘at most once’ guarantee is not
necessary to adhere to other requirements of the Write trait. [edit:
To be clear, I’m not saying writers should loop over input, I’m just
pointing out that ‘at most once’ is not required to uphold other
requirements of the trait.]

Prior discussion: #56889 (comment)

Interesting. I actually stumbled upon this working on base64 as well.
Didn’t notice that Marshall has already brought this up.

I wasn’t aware about the write_all issue so perhaps changing wording
around Ok(0) isn’t appropriate, but then I argue even more strongly
that ‘at most once’ requirement shouldn’t be there. Essentially, I’m
agreeing with @alexcrichton here:

The "at most one write" language I think should be toned down to
a general guideline, as it's basically just saying "please don't
make write equivalent to write_all".

I’ve updated the commit to not change meaning of Ok(0).

[the8472]

I think it's a libs-api issue since it changes Trait guarantees.

r? libs-api

[the8472]

I have looked through some previous PR discussions around BufWriter/LineWriter/stdout buffering by @Lucretiel (some of which didn't land). The main aim there was to minimize tearing of written slices, but not a hard guarantee that 1 write call = 1 syscall.

#78551 (comment) argues

It does only try to write the newly submitted data at most once. flush_buf only targets old data.

I think this is somewhat of a middle ground. The new data results in at most one write to the underlying. The old data already "paid" by a previous write call.

---

Some general edge-cases:

• polled IO where the caller needs ErrorKind::WouldBlock to arm epoll&co., storing the error in the writer and only returning it on the next write attempt could be surprising
• writes to procfs/sysfs which must be performed in a single syscall. A user may want to println!() into a procfile and use bufwriter to coalesce the writes into a single one
• atomic appends
• sockets / nagle's algorithm (write-read is fine, write-write-read is bad)

---

• writers that are buffering may flush out previously-acknowledged-and-buffered data at the beginning of a write call. if that errors the error must be propagated immediately
• they should (best-effort) avoid tearing a written slice.
  • Which means if they're flushing the previously buffered data they should avoid topping up the buffer with a few bytes from the incoming write before flushing it
    • unless the data can be written in one go
      • small write and just-so fits in the buffer
      • is_write_vectored() == true -> write_vectored. avoids nagle
    • sad case: no vectored writes -> buffer flush + new write -> write-write-read 😢

-> If we ignore previously written data that's being flushed now then 1 outer write = at most one inner write

[mina86]

• polled IO where the caller needs ErrorKind::WouldBlock to arm epoll&co., storing the error in the writer and only returning it on the next write attempt could be surprising
• writes to procfs/sysfs which must be performed in a single syscall. A user may want to println!() into a procfile and use bufwriter to coalesce the writes into a single one
• atomic appends
• sockets / nagle's algorithm (write-read is fine, write-write-read is bad)

None of those work with current Write trait. A Write implementation can store error, write only half of the input or buffer until it has 69KB to write. All of those are valid and will break one of the listed edge cases. This PR doesn’t change anything in that regard. Softening the ‘at most one write’ requirement doesn’t make things worse for the listed cases while at the same time simplifies implementation of whole class of writers (and by simplifies I mean turn them from arduous to straightforward).

[writers] should (best-effort) avoid tearing a written slice.

I don’t think I’d even agree with that phrasing. This is certainly use-case dependent and giving strong preference for particular use-case seems questionable.

[joshtriplett]

We talked about this in the @rust-lang/libs-api meeting today.

BufWriter already behaves like this, and we don't probably want to change that (because doing so would add extra copying into the buffer), so we should document the expectation. Given that:

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[a1phyr]

I think that the change about returning should be reverted, at least for now.

Returning Ok(0) on write used to mean EOF, which is perfectly fine. I don't understand why would the interface change about this.
If writing is not possible now but will be in the future and blocking is not appropriate, returning Err(WouldBlock) is the thing to do.

It is the right thing for write_all to error with Err(WriteZero) on Ok(0), because EOF was reached before the function could do what it was instructed. Now there are two possible errors to check if write_all failed because the writer is full: WriteZero and StorageFull.

Moreover, this proposes StorageFull, which is meant for the underlying storage (eg a disk) more than a writer itself. Also, StorageFull is unstable, which mean most users cannot even respect the new contract.

Additionally, this changes the interface without changing any implementation in std, which continues to return Ok(0) on EOF. Doing so would be a change that actually breaks code so probably won't be accepted. So we end up with std not implementing properly its own traits.

[the8472]

Please open a new issue on that. Most people won't notice discussion on a merged PR.

[joshtriplett]

@a1phyr Thanks for catching this! #114897