Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse Framed buffers with new codec #717

Closed
1tgr opened this issue Oct 21, 2018 · 10 comments
Closed

Reuse Framed buffers with new codec #717

1tgr opened this issue Oct 21, 2018 · 10 comments
Labels
C-enhancement Category: A PR with an enhancement or bugfix.

Comments

@1tgr
Copy link

1tgr commented Oct 21, 2018

I'm attempting to use multiple codecs on a single stream: my protocol (WebSockets) has an initial handshake before the data frames are processed. I have two codecs for this, one for the handshake and one for the data frames.

After the handshake codec is done, I'd like to reuse the read and write buffers from the Framed with a new codec. In the deprecated tokio_io::codec codebase I can do this by calling Framed::from_parts(old_framed.into_parts(), new_codec). But in #394 the codec instance was moved inside the FramedParts struct and Framed::from_parts can no longer take the new_codec parameter.

How should I be doing this in terms of tokio_codec functions?

@1tgr
Copy link
Author

1tgr commented Oct 21, 2018

I have this workaround:

fn set_codec<T: AsyncRead + AsyncWrite, C1, C2: Encoder + Decoder>(framed: Framed<T, C1>, codec: C2) -> Framed<T, C2> {
    let parts1 = framed.into_parts();
    let mut parts2 = Framed::new(parts1.io, codec).into_parts();
    parts2.read_buf = parts1.read_buf;
    parts2.write_buf = parts1.write_buf;
    Framed::from_parts(parts2)
}

But it seems like Framed<T, U> needs a set_codec method that returns Framed<T, U2>, or likewise on FramedParts<T, U>.

@carllerche
Copy link
Member

An interesting case. It feels pretty specialized, but also that it should be better supported.

We could possibly add fn from_framed(framed, codec) that takes a framed instance and a new codec.

Thoughts?

@1tgr
Copy link
Author

1tgr commented Nov 19, 2018

Yep, from_framed(framed, codec) would do the job.

Here is where I ended up using my workaround:
https://github.com/1tgr/rust-websocket-lite/blob/d3594c08124fcabd03ebb87a086e811e2b925ac1/src/client.rs#L187-L194

@carllerche
Copy link
Member

One issue @seanmonstar raises is that you might want to take ownership of data form the first codec and move it into the second codec. My proposed API would not solve that.

@carllerche
Copy link
Member

We could add map_codec(|old_codec| build_new_codec(old_codec)) instead.

@carllerche carllerche added this to the v0.2 milestone Jun 25, 2019
@carllerche carllerche removed this from the v0.2 milestone Dec 21, 2019
@carllerche
Copy link
Member

I think the solution for this will be to switch to using AsyncBufRead instead of a custom solution. That said, the trait needs to be tweaked to support our use case.

@carllerche carllerche added the C-enhancement Category: A PR with an enhancement or bugfix. label Dec 21, 2019
@bschwind
Copy link

I think the solution for this will be to switch to using AsyncBufRead instead of a custom solution.

@carllerche can you expand a bit on how the AsyncBufRead trait will solve this issue?

@carllerche
Copy link
Member

Refs: #2428

A BufReader will contain the buffer as well as the stream. This type can be removed from the codec and passed to a new one.

I am going to close this issue as it is relatively low priority and is blocked on #2428. However, once #2428 happens (soonish), anyone should feel free to attempt this work.

@saiintbrisson
Copy link
Contributor

saiintbrisson commented Jan 26, 2022

As #2428 and #2896 have been put aside, are there any other blockers for this implementation? If not, I'd be willing to work on it.

@Darksonn
Copy link
Contributor

I'm not aware of any blockers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: A PR with an enhancement or bugfix.
Projects
None yet
Development

No branches or pull requests

5 participants