-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Refactor codec::length_delimited
#575
Refactor codec::length_delimited
#575
Conversation
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
4d03a89
to
d6cd03b
Compare
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Issue #500's todo list also includes adding tests and a fix for #497; I'm going to start on that in a subsequent PR. Edit: Actually, having looked at #497, I think this change may fix it as well, provided the same issue doesn't exist in |
I'll have to review this in more depth when I'm fresh, however after an initial scan, this seems like an improvement. I wonder why it was originally done the other way. Do you have any thoughts about this? Do you see any disadvantages to this route? |
I'm not sure what the original motivation behind the prior implementation was. I did a little digging, and it looks like the I can't think of any obvious technical disadvantages to this change, but it's certainly possible I've overlooked something. |
Ok, I found the main difference. The original implementation is able to take advantage of vectored writes to avoid having to perform a large amount of data copying. That said, the win of this is particular implementation is debatable. I originally wrote this to support HTTP/2.0, but ended up not using this codec. Additionally, the current implementation is not ideal for small frames. So, I am not necessarily opposed to merging this refactor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts I started writing up.
src/length_delimited.rs
Outdated
#[derive(Debug)] | ||
struct Decoder { | ||
pub struct Codec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this will be the main type, maybe we should name it LengthDelimited
. This way, when used, it will be Framed<T, LengthDelimited>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the name be LengthDelimitedCodec
if we're matching existing codecs?
src/length_delimited.rs
Outdated
pub struct FramedRead<T> { | ||
inner: codec::FramedRead<T, Decoder>, | ||
} | ||
pub type FramedRead<T> = codec::FramedRead<T, Codec>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid this type def for now. We can always add them later if needed.
src/length_delimited.rs
Outdated
/// See [module level] documentation for more detail. | ||
/// | ||
/// [module level]: index.html | ||
pub type FramedWrite<T> = codec::FramedWrite<T, Codec>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@carllerche I'm not sure what's the right way to deal with the issue around vectored writes, as that is a potentially significant change in behaviour. However, I believe I've addressed all the other requested changes, in case we still want to go through with merging this branch despite the vectored writes issue. |
If @carllerche is cool with merging this sans optimization, seems worth it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with merging it.
Motivation
The current
tokio::codec::length_delimited
module provides its ownFramed
,FramedRead
, andFramedWrite
types which provide the samefunctionality as the types of the same name in the
codec
module.However, it's not currently possible to use the
codec::Framed{Read, Write}
types with the length-delimited codec; instead, the separate
types in the
length_delimited
module must be used.Solution
This branch unifies
length_delimited
with the rest oftokio::codec
,by replacing the private
length_delimited::Decoder
type with a publiclength_delimited::Codec
that implements bothcodec::Encoder
andcodec::Decoder
. It may now be used with theFramed
,FramedRead
,and
FramedWrite
types intokio_codec
. This branch also updates theAPI documentation and tests to reflect these changes.
Refs: #500