Decoder::decode() says an Err informs Framed that the stream should be terminated, but Framed doesn't terminate

[lilyball]

Version
tokio-util v0.6.7

Platform
Darwin My-Computer 20.5.0 Darwin Kernel Version 20.5.0: Sat May 8 05:10:33 PDT 2021; root:xnu-7195.121.3~9/RELEASE_X86_64 x86_64

Description
tokio_util::codec::FramedRead will happily continue trying to decode a stream after the associated tokio_util::codec::Decoder implementation returns Err from Decoder::decode() or Decoder::decode_eof(). This happens in spite of the documentation on Decoder::decode():

Finally, if the bytes in the buffer are malformed then an error is returned indicating why. This informs Framed that the stream is now corrupt and should be terminated.

(decode_eof() does not document the semantics of returning an error but it's safe to assume it's the same as decode()).

Looking at <FramedImpl as Stream>::poll_next(), it returns the Err to its caller without modifying its state:

tokio/tokio-util/src/codec/framed_impl.rs

Lines 168 to 179 in 2087f3e

	let frame = pinned.codec.decode_eof(&mut state.buffer)?;
	if frame.is_none() {
	state.is_readable = false; // prepare pausing -> paused
	}
	// implicit pausing -> pausing or pausing -> paused
	return Poll::Ready(frame.map(Ok));
	}
	// framing
	trace!("attempting to decode a frame");
	if let Some(frame) = pinned.codec.decode(&mut state.buffer)? {

This means that if the decode() method returns an Err without consuming any bytes from the buffer, a simple loop like the following will go into a tight infinite loop:

while let Some(result) = framed.next().await {
    process(result);
}

And if a Decoder impl can return recoverable errors, and does so from the decode() method instead of moving them into the Item, then that lends itself to the following loop:

while let Some(result) = framed.next().await {
    match result {
        Ok(val) => process(val),
        Err(e) => warn!(log, "error decoding stream"; "error" => %e),
    }
}

It's really easy to write this loop, especially if you haven't carefully read all of the associated documentation, and you won't realize what happened until you hit a nonrecoverable error.

If Framed actually did what the Decoder::decode() documentation suggests and moved to a terminal state upon encountering an Err, then these loops would instantly go from an infinite loop to one that logs an error and terminates. This would also strongly encourage Decoder impls that can return recoverable errors to make that distinction properly in its return type.

Optionally, if someone is using a non-conforming Decoder impl that mixes recoverable and non-recoverable errors together and the calling code wants to intelligently handle that, Framed could offer a method to recover from the terminal state.

This behavior change should probably be considered a breaking change, but I could make the argument that it's a bugfix (makes behavior conform to documentation) and that it's an important safety improvement for existing code (turns infinite loops into finite ones).

[Darksonn]

It seems reasonable that it should return None after an error.

[bIgBV]

@Darksonn I can take a look at this issue.

[Darksonn]

Fixed in #4166