embedded_io's WriteAllError

[adamgreig]

Currently, embedded_io::Write::write() returns Ok(0) to indicate "did not write any bytes and cannot write any more", for example due to a closed socket or EOF or other situation that is not "an error" but nevertheless means no more data can ever be written.

The implementation of Write::write_all() must detect this situation and return early, otherwise it would block forever trying to write more data. To signal this condition, write_all() returns Result<(), WriteAllError<E>>, where WriteAllError<E> is an enum that can indicate either that the sink returned Ok(0) or that it returned some other error E, specific to the writer type.

This makes it awkward to write middleware which needs to call write_all(), as each middleware layer then needs to implement their own wrapping error type rather than passing up the error type used by the underlying writer.

We discussed this in the wg meeting on 2023-08-29 and concluded the best option is to change the documentation for write() to prohibit returning Ok(0) and instead signal the "no more data can be written" condition as an error instead. This allows write_all() to return Result<(), E> directly.

The current behaviour matches std::io, but in std the returned error is Boxed which avoids the wrapping problem we encounter. By changing write()'s behaviour we diverge from std, but it is still possible to maintain the implementations of embedded_io for std traits.

If anyone has any further thoughts on the change, please discuss here; otherwise if anyone would like to prepare a PR implementing this change please go ahead.

[Dominaezzz]

This makes it awkward to write middleware which needs to call write_all(), as each middleware layer then needs to implement their own wrapping error type rather than passing up the error type used by the underlying writer.

Initially I wasn't sure what you meant by needing to make a wrapping error type, since write_all calling an underlying write_all can just forward the error as is, which is what I have done in some middleware I've written as well. However after reading the chat/meeting I now see you're trying to call write_all in flush, which make the problem more obvious.

For reference this is the implementation of flush for the BufferedWrite struct.

    async fn flush(&mut self) -> Result<(), Self::Error> {
        if self.pos > 0 {
            self.inner.write_all(&self.buf[..self.pos]).await?;
            self.pos = 0;
        }

        self.inner.flush().await
    }

I'm going to say I think the use of write_all there is incorrect, or more specifically the WriteZero error variant should be ignored.

If I have an implementation of embedded_io::Write for an SD Card, several calls to write will eventually return Ok(0) since the SD Card has a finite capacity. If I wrapped this with the BufferedWrite struct, I think it would be incorrect for the BufferedWrite struct to accept more bytes than the SD Card can take.

EDIT: I just looked at the docs for the Write and it looks like none of this matters? Looks like returning Ok(0) is already not allowed.

    /// Implementations should never return `Ok(0)` when `buf.len() != 0`. Situations where the writer is not
    /// able to accept more bytes and likely never will are better indicated with errors.

[MabezDev]

I think this can be closed now, as https://github.com/rust-embedded/embedded-hal/pull/505 has been merged.