tokio-uitls: from_parts() for FramedWrite and FramedRead

[Mrreadiness]

Implement from_parts() for FramedWrite and FramedRead.

Motivation

Thanks to #7566, FramedWrite and FramedRead can be transformed into FramedParts, but there is no way to create them back from parts, or to transform Framed into FramedRead or FramedWrite without losing progress in buffers.

My original problem was splitting Framed into both FramedRead and FramedWrite, to swap the codec for one of them with map_codec. I wish we could have something like into_split for Framed:

impl<T, U> Framed<T, U>
where
    T: AsyncRead + AsyncWrite,
    U: Clone,
{
    pub fn into_split(self) -> (FramedRead<ReadHalf<T>, U>, FramedWrite<WriteHalf<T>, U>) {
        let (r, w) = split(self.inner.inner);
        let read = FramedRead::from_parts(FramedParts {
            io: r,
            codec: self.inner.codec.clone(),
            read_buf: self.inner.state.read.buffer,
            write_buf: BytesMut::new(),
            _priv: (),
        });
        let write = FramedWrite::from_parts(FramedParts {
            io: w,
            codec: self.inner.codec,
            read_buf: BytesMut::new(),
            write_buf: self.inner.state.write.buffer,
            _priv: (),
        });
        return (read, write);
    }
}

But cloning of the codec doesn't feel right here. So, with from_parts, a user will have more control over the reader and writer reconstruction.

Solution

Implement from_parts() for FramedWrite and FramedRead, which builds those structures from FramedParts and preserves internal buffers.

[ADD-SP]

Would mind adding some tests for both from_parts and into_parts?

[Mrreadiness]

@ADD-SP Thanks! I added tests as well.

I also noticed that FramedParts::new() forces the creation of parts with at least an Encoder codec. But it doesn't make much sense, because it could be used for Decoder without Encoder. Looks like it's just a historical mistake, and there was an attempt to remove this limitation (#5280), but it's a breaking change, so it was reverted.
That's why I added an alternative constructor: FramedParts::new_parts.

So, solving the task of splitting up Framed will look like:

let framed = Framed::from_parts(parts);
let FramedParts {
    io,
    read_buf,
    write_buf,
    ..
} = framed.into_parts();
let (io_read, io_write) = tokio::io::split(parts.io);
let mut read_parts = FramedParts::new_parts(io_read, Codec::new());
read_parts.read_buf = read_buf;
let framed_read = FramedRead::from_parts(read_parts);
let mut write_parts = FramedParts::new_parts(io_write, Codec::new());
write_parts.write_buf = write_buf;
let framed_write = FramedWrite::from_parts(read_parts);

It's far from perfect, but at least doable now.
If you see a better way to do such a split or even an ergonomic way to add it as a method of Framed, I will be happy to implement it.

[ADD-SP]

Thanks for the context! I'm considering deprecating the tokio_util::codec::FramedParts::new. What do you think? What should the new constructor be called? new_parts? new2? new_v2?

[Mrreadiness]

I end up with new_parts. Sounds like not a great naming situation anyway.

[martin-g]

Please add a comment that this must be the same as the non-public constant at https://github.com/Mrreadiness/tokio/blob/52fcebcaea1e7f6930d1f21e84b59e36ae22d725/tokio-util/src/codec/framed_impl.rs#L25

[martin-g]

This loses the eof and has_errored flags - https://github.com/Mrreadiness/tokio/blob/52fcebcaea1e7f6930d1f21e84b59e36ae22d725/tokio-util/src/codec/framed_impl.rs#L76-L77
Is this OK ?

[Mrreadiness]

I replicated the same behaviour as in Framed.
As I understood from the state machine, it's okay.

has_error - used in 2 cases:

1. Error from "previous" Decoder. A user already saw this error and intentionally created a new Framed (probably with another Decoder) to try to decode again.
2. Error from underlying IO. I'm not 100% sure, but I suppose we get the same error with the next read attempt. A user saw this IO error as well.

For eof, we should restore this knowledge with the next call to the underlying IO here. As I understood from AsyncRead docs, the expected behavior after reaching EOF is Ok(Ready) with 0 new bytes for all subsequent read attempts.

[Mrreadiness]

I just realised that these 2 flags are related to IO, and the user can change parts.io. So we don't have any other way to get this knowledge back, but to interact with IO itself.

[martin-g]

What should the new constructor be called? new_parts? new2? new_v2?

How about new_unchecked() ?!
To make it more clear that U: Encoder<I> is not required.

[ADD-SP]

How about new_unchecked() ?!

@martin-g The _unchecked suffix is usually for unsafe function.