Write::write_all erroring when encountering Ok(0) interacts poorly with the contract of Write::write

[marshallpierce]

The documentation of write (emphasis added) specifies that returning Ok(0) is a valid thing to do, albeit with a possible implementation-specific "this writer is dead" meaning:

A return value of 0 typically means that the underlying object is no longer able to accept bytes and will likely not be able to in the future as well, or that the buffer provided is empty.

Despite the above, Write::write_all's current implementation returns an error when an underlying write returns Ok(0), effectively making this a forbidden return value for implementations of Write. However, in cases where a Write is transforming or buffering data, it can be useful to return Ok(0) to avoid violating this other, more firmly worded, requirement of write:

A call to write represents at most one attempt to write to any wrapped object.

I came across this issue while working on a streaming base64 encoder Write for the base64 crate, and subsequently found other cases where these features have clashed (hat tip to jebrosen on IRC).

In the case of base64, suppose a user constructs a base64 writer with an internal buffer that delegates to, say, stdout. The user calls write with a 6-byte input, which is base64 encoded into 8 bytes in the internal buffer, and subsequently written to the delegate (stdout). The delegate writes 5 bytes, and the remaining 3 bytes are kept in the buffer. On the next call to write from the user, I want to consume no input (and therefore return Ok(0)), but rather simply try again to write the remaining 3 bytes to the delegate. (Even with a different implementation that does consume as much input as can be encoded and still fit within the buffer, eventually that buffer will fill, and we're back to returning Ok(0).)

The same issue affects flate2:

// miniz isn't guaranteed to actually write any of the buffer provided,
// it may be in a flushing mode where it's just giving us data before
// we're actually giving it any data. We don't want to spuriously return
// `Ok(0)` when possible as it will cause calls to write_all() to fail.
// As a result we execute this in a loop to ensure that we try our
// darndest to write the data.

That violates the "at most one write" part of write, but it could be avoided if returning Ok(0) wasn't going to break write_all.

BufWriter in the std lib has a similar problem:

impl<W: Write> Write for BufWriter<W> {
    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
        if self.buf.len() + buf.len() > self.buf.capacity() {
            self.flush_buf()?;
        }
        if buf.len() >= self.buf.capacity() {
            self.panicked = true;
            let r = self.inner.as_mut().unwrap().write(buf);
            self.panicked = false;
            r
        } else {
            Write::write(&mut self.buf, buf)
        }
    }
...

If BufWriter::write is called when its buffer is nonempty with an input that won't fit in the buffer, the second if leads to two calls to the delegate writer for one BufWriter::write call, which violates the contract of write. Similarly, returning Ok(0) there would solve the issue: the caller could then retry with the same buffer, which would be passed to the delegate writer untouched.

No doubt there are complexities I don't see yet, but given that write_all's documentation doesn't say anything about erroring on Ok(0), is it feasible to simply continue looping instead of erroring on Ok(0)? If the concept of "writer exhaustion" the "typically..." clause in write's docs is referring to needs to be represented, could that not be an ErrorKind?

[SimonSapin]

@rust-lang/libs, any thoughts?

[sfackler]

A call to write represents at most one attempt to write to any wrapped object.

This requirement seems at best annoying and at worst unenforceable. For example, I have no idea if OpenSSL's SSL_write behaves this way not. I could in theory add some kind of buffering layer between OpenSSL and the downstream Write implementation but that really does not seem worth it. I think I'd probably advocate for removing that language in favor of something that's a bit softer.

[marshallpierce]

AFAICT, the at-most-one requirement is necessary for this other part to be true in practice, though:

If an error is returned then no bytes in the buffer were written to this writer.

And that's a pretty useful property.

[sfackler]

That is also not a thing that you can guarantee when e.g. wrapping third party libraries.

[marshallpierce]

I agree; I just think it's a good thing to strive for. But either way, I don't think we have to cross that bridge if we change the (undocumented) behavior that write_all bails on Ok(0).

[sfackler]

That will break the use of write_all with all writers which return Ok(0).

[alexcrichton]

IIRC handling Ok(0) was intended to handle some weird cases of pipes on some OS somewhere where a write to a closed pipe/socket didn't return an error but rather returned Ok(0). 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". This is already the case in flate2 where write only goes until something succeeds. It's a special case because of a wonky C library API and just the requirements of the protocol it's implementing.

[marshallpierce]

I'm not sure I follow -- what would the middle ground be between "one write" and "like write_all", and how would that obviate the need to return Ok(0) when some intermediate buffer can't be written fast enough yet it's not an error?

[alexcrichton]

To me the middle ground is "if you can, make sure write isn't the same as write_all", and that's basically it. There's not really a guarantee we can provide here, it's just a suggestion for implementors which is intended to make their lives easier in the majority of situations.

[marshallpierce]

What would my implementation choice be, then, that doesn't return Ok(0) but isn't the same as write_all and still upholds the if an error occurred, no write took place part?

Or, to look at it from another angle: how widespread is the usage of weird OS/pipe interactions vs the usage of types that end up in unintuitive, contrary-to-docs behavior because of write_all's current handling of Ok(0)?

[alexcrichton]

I believe base64 is the same case as flate2. Some internal buffering is required and writes into the adapter don't match up with writes to the other end. Writes of just a few bytes in gzip can generate megabytes of output, so often the input needs to be blocked while that happens.

[marshallpierce]

But doesn't flate2's way imply that it can return an error and have written some data? Any time we retry anything that could be the case...?

[jebrosen]

Here's my interpretation of this so far:

• Write::write states that Ok(0) typically means "cannot be further written to".
  • This can be returned, for example, when attempting to write "past" the end of a slice or into an empty slice.
  • The provided implementation of Write::write_all considers Ok(0) with outstanding data to be written as an error condition.
• Write::write states that it "represents at most one attempt to write to any wrapped object"
  • Definitely violated by BufWriter -- it may flush, which does about the same thing as a write_all to the inner buffer. Also violated by flate2 and probably others for similar reasons.
• Write::write states that "If an error is returned then no bytes in the buffer were written to this writer."
  • Not violated by BufWriter, possibly only because of my own interpretation of the sentence. If the first flush_all raised an error, no bytes in the [new] buffer could have been written. If the [new] buffer is larger than BufWriter's internal buffer, then the return from BufWriter<W>::write is whatever W::write returns, so BufWriter is still compliant as long as W is. Otherwise, it writes to its internal buffer which should never error anyway.
    • I'm still working through the implementation in flate2, but I think similar reasoning applies.
  • My take on this requirement is that "retrying" a failed write by writing the same buffer again is always supposed to be safe with no risk of writing duplicate data, which I think is true of BufWriter and possibly for flate2. They might write something to the underlying writer and return an error, but in that case they do not write or stash "the buffer" referred to in the requirement.

If I've interpreted the requirements correctly, "represents at most one attempt to write to any wrapped object" is already ignored by several Write implementations and is not actually necessary to enforce "If an error is returned then no bytes in the [current incoming] buffer were written to this writer."

[alexcrichton]

@jebrosen everything you mentioned sounds accurate to me, and I think supports the idea that we should just relax the language here as it isn't really intended to be so strict anyway.

@marshallpierce I don't believe that's the case for flate2, it shouldn't be possible to return an error and have written some data.

[marshallpierce]

So, it seems that one way to go here would be to remove the "just write once" restriction, which would have the effect of causing write_all-style logic to be potentially anywhere in your chain of writers. It's certainly possible to do, but that seems like a shame to me. It also doubles down on the messy interpretation of Ok(0).

What if instead we tackle the other end of the problem, which is that Ok(0) is in sort of a no man's land? If there are a few circumstances in which it causes a problem, why not handle those cases individually and map Ok(0)-that's-really-an-error to an actual Err just for them? Then Ok(0) would be free to represent what it looks like it represents: "not an error, but none of your input was written".

[alexcrichton]

I think that's in the vein of where we want to go, yes, but I don't think that we need to remove such a clause entirely. I think we just need to say something in the spirit of "you should strive to make write not equivalent to write_all".

I don't think we're in a position to change the meaning of Ok(0), that I suspect would be a bit too subtle of a breaking change.

[marshallpierce]

So, is the upshot then that this is the suggested path for base64...?

• On write(), write as many times as needed to flush the internal buffer, then encode the input buffer into the internal buffer and write as much as possible of it in one write() call to the delegate writer
• If an error occurs at any point, return it

That means, for instance, that some data can be written successfully followed by an error (data from the internal buffer, not from the input buffer passed to write()), which seems wrong to me.

[jebrosen]

That means, for instance, that some data can be written successfully followed by an error (data from the internal buffer, not from the input buffer passed to write()), which seems wrong to me.

I don't think multiple calls to write are avoidable. For example in the case of a hypothetical wrapped Write that always only consumes 1 byte (and returns 1) every time write is called, pushing buffered data to the inner writer has to happen somewhere -- either when data is initially produced by the outer Write or on a later call to write when the internal buffer fills.

[Lucretiel]

That means, for instance, that some data can be written successfully followed by an error (data from the internal buffer, not from the input buffer passed to write()), which seems wrong to me.

I think it's okay if the user-facing contract of write is maintained. The most important part of that (to me) is that there are two outcomes:

• Ok(n > 0): n bytes were "consumed" from the user input, and no errors were returned from anywhere in the Write stack. This doesn't mean that the bytes reached any specific point, only that no errors have occurred yet. These bytes are to be truncated from the front of user input before subsequent calls to write.
• Err(err): Something went wrong somewhere, and no bytes were consumed. The writer is still in a valid state such that subsequent calls to write may be safely attempted, with the same data, without duplicated data being written anywhere (though obviously if something is wrong (ie, pipe closed), these writes may also fail).

The key here is that the Writer is always in a determinate state, with regard to what data may be provided to a subsequent call to write.

This allows us to relax the "at most one write" language while still having a formal distinction from write_all: you can call write multiple times (ie, to flush a full internal buffer), but if any user bytes are consumed, this must be reported to the user as soon as possible; at the very least, before any operations that may return an io::Error or other failure.

This also allows us to make the contract more strict in the other direction: because Ok(0) is interpreted as EOF, the write call is obligated to continue trying to make progress until it is able to consume at least 1 user byte from this write (or an error occurs or genuine EOF condition is reached).

This is distinguished from write_all, which only has the contract that "either 100% of the write succeeded or there was an error and an unknown number of bytes were consumed." There isn't any safe way to retry a write_all call, because you have no way of knowing how much data was written.

[Blub]

IMHO a sane writer should return an error when it is already at the end. This would be in line with eg. a block device (a typical size-limited object to write to...). Ok(0) doesn't really make any sense to me. If you can't write anything yet, return EWOULDBLOCK (or, well, block), if you can't write anything because of some error, return the error. But don't just do nothing (or purely some unrelated internal stuff) and return 0, that makes no sense.
So in that sense, treating Ok(0) as an error condition is acceptable, because it's something that shouldn't happen anyway. (And yes, IMO the slice impl could just as well be changed)

[Lucretiel]

Related to @Blub's point: another crucial property of this design is that it means that looped writes (a common pattern) may not loop forever. If Ok(0) was a valid result (that is, if it simply meant the device "successfully" wrote 0 bytes and is not in an eof state), then it would be acceptable to continue to retry those writes, looping forever without (necessarily) making progress. Ok(0) must in some sense signal "will not ever be accepting more bytes", because it must cause write loops to terminate.

[m-ou-se]

We discussed this issue in the libs api meeting just now, and agreed that the current behaviour of write_all for Ok(0) is the right one.

is it feasible to simply continue looping instead of erroring on Ok(0)?

Nope. That could result in an infinite loop if write() keeps resulting in Ok(0). E.g. a os/hardware buffer that has no space left. Busy-looping until there is space again (which might take a long time, or might even never happen) is not something we can do. We also can't just return from write_all without writing all data. So the only thing we can do with Ok(0) is to return an error.