doc: AcceptedAlert::write doesn't necessarily write all bytes

[jsha]

The docs for AcceptedAlert::write() say "Send the alert to the client." Under the hood it calls ChunkVecBuffer::write_to, and returns the usize.

The problem is, &mut dyn Write is allowed to return short writes. So it's possible that write_to writes less than all of the ChunkVecBuffer's contents.

I think the correct user behavior is to call AcceptedAlert::write() in a loop until it returns Ok(0) or an error. Is that right? If so, examples/src/bin/server_acceptor.rs should be updated. And the docs should say "Send alert bytes to the client. Call this repeatedly until it returns Ok(0) or an error, to ensure all bytes are sent."

[djc]

Fair point, we've been noticing this stuff in tokio-rustls: rustls/tokio-rustls#48.

[ctz]

Yeah we should fix the example code to deal with short writes for good form.

Though, in practice, there is really zero chance I think that that would ever happen with a real TCP sockets -- the server has written no data, so the write window is at least the MTU (the smallest of which is 68 bytes, and easily fits one alert). Though perhaps it's possible for protocols that upgrade to TLS from another protocol, like SMTP STARTTLS.

[cpu]

Yeah we should fix the example code to deal with short writes for good form.

Here's a quick patch for the doc update & example's usage of AcceptedAlert::write: #1868

It might make sense to do a more systematic evaluation of all the examples + writes as a follow up to make sure we address this holistically.

[jbr]

Maybe an AcceptedAlert::write_all(&mut io) -> io::Result<()> would improve ergonomics for blocking usage?

[jsha]

Good point @jbr - or we could change the behavior of write() to do write_all(), since as @ctz says this would never happen with a real TCP socket. So we would pay the price of possibly blocking an async executor in a very unlikely situation for the convenience of users not having to loop over the result.

[jbr]

I don't think blocking would be the concern since the underlying io is set to non-blocking and so would return ErrorKind::WouldBlock and presumably early-return from the loop. This probably would be fine, since the async wrapper could do something roughly like the following:

return match alert.write(io) {
  Err(e) if e.kind() == ErrorKind::WouldBlock => Poll::Pending,
  other => Poll::Ready(other)
}

The loop in write would likely never continue in async usage because even a successful write is WouldBlock.

[cpu]

Maybe an AcceptedAlert::write_all(&mut io) -> io::Result<()> would improve ergonomics for blocking usage?

That's a good suggestion, thanks. I added a commit for this in #1868 and switched the acceptor example update commit to use it.