codec: change Encoder to take &Item #1746

djc · 2019-11-07T10:55:31Z

Since the encoded item is to be encoded into a given BytesMut, taking ownership is not necessary here.

djc · 2019-11-07T11:40:25Z

As discussed here: tokio-rs/tokio-io#54.

Marwes · 2019-11-07T15:10:08Z

Since this is an input argument, then Item should probably be a type parameter instead of an associated type? If that is the case then the implementer can freely specify reference vs non-reference.

hawkw · 2019-11-07T23:30:06Z

I agree with @Marwes' comment — I think the current design of this trait may be an artifact of older versions of the Sink trait, which had an associated type for their inputs rather than a type parameter...

djc · 2019-11-08T21:44:52Z

So I've changed the Item to be a type parameter instead of an associated type (see the second commit in the series). However, I'm not sure if the second step (allow ownership transfer or just passing a reference) is actually possible because I don't how to express that any lifetime bound is allowed. Here's an example error:

error[E0716]: temporary value dropped while borrowed
   --> tokio-util/tests/length_delimited.rs:399:14
    |
399 |     let io = length_delimited::Builder::new()
    |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary which is freed while still in use
...
405 |         });
    |           - temporary value is freed at the end of this statement
406 |     pin_mut!(io);
    |     ------------- borrow later used here
    |
    = note: consider using a `let` binding to create a longer lived value
    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

This seems to happen because the FramedWrite (for example) gets returned with a lifetime matching the Builder lifetime. I tried to:

use higher-ranked trait bounds to get the compiler to unlink the lifetimes
explicitly used lifetime names to unlink the lifetimes

but neither of these approaches had the desired effect. Any suggestions?

djc · 2019-11-09T14:25:37Z

I did yet another attempt with using AsRef<Item>, but it was pretty ugly. I'm really not sure of the value of allowing both ownership and references to be passed?

Marwes · 2019-11-09T14:52:04Z

tokio-util/src/codec/encoder.rs

@@ -18,5 +18,5 @@ pub trait Encoder {
    /// This method will encode `item` into the byte buffer provided by `dst`.
    /// The `dst` provided is an internal buffer of the `Framed` instance and
    /// will be written out when possible.
-    fn encode(&mut self, item: Self::Item, dst: &mut BytesMut) -> Result<(), Self::Error>;
+    fn encode(&mut self, item: &Item, dst: &mut BytesMut) -> Result<(), Self::Error>;


Didn't think about the lifetime issues :/ I am not able to decide on this obviously but &Item with Item as an argument seems like the way to go 👍

djc · 2019-11-09T15:13:16Z

Do you have some kind of use case where you'd prefer to pass ownership?

Marwes · 2019-11-09T16:02:05Z

No, just thought it would work for both cases.

djc · 2019-11-18T17:01:37Z

Ping! Any further feedback before I spend time on rebasing this?

carllerche · 2019-11-27T18:57:19Z

This seems good to me. I can queue it up to be merged w/ tokio-util 0.3 (which can be sooner than later).

djc · 2019-11-27T20:08:07Z

(Rebase not quite finished, will continue later, pending taiki-e/pin-project#170.)

djc · 2019-11-29T19:49:52Z

It looks like pin-project-lite does not support ?Sized bounds, and I think it's not very obvious how to support these in declarative macros. So I'm not sure how to continue rebasing this.

djc · 2019-12-06T20:57:11Z

@carllerche ping? The question here is if it would be okay to revert to pin-project instead of pin-project-lite for tokio-util in order to make this work.

carllerche · 2019-12-06T21:40:49Z

I would rather avoid either then (use unsafe). pin-project is too heavy at this point IMO.

taiki-e · 2020-01-20T10:50:39Z

(pin-project-lite is now support ?Sized bounds in generic parameters.)

djc · 2020-01-20T14:01:30Z

@taiki-e that's great, thanks!

So I rebased this on top of current master, keeping the use of pin-project-lite intact.

However, iterating on this I question the change going from Encoder<Item = I> to Encoder<I>: it appears to me that in the latter case, the type used for I must be specified explicitly for types like Framed, requiring a third type parameter everywhere (otherwise I get the not constrained by the impl trait, self type, or predicates error for some of the Framed impls).

IMO, the current situation, where the Encoder implementation specifies a single associated type and thus Framed instantiations do not have to repeat the Encoder's item type, is more ergonomic to use. I've pushed a second commit that changes this back, and would like some feedback on the preferable direction before I go through all the example code to fix it up if necessary.

LucioFranco · 2020-01-21T00:18:56Z

I wonder if it makes sense to make Item a generic on the trait which then would allow you to use for<'a>: Encoder<&'a T>. This could enable both scenarios.

djc · 2020-01-21T08:04:19Z

@LucioFranco do you think that would fix the issues from #1746 (comment)? This feels like we're going back and forth. Also, if Item is generic on the trait, don't Framed and friends still have to specify the Encoder's Item type explicitly?

Marwes · 2020-01-21T08:57:54Z

Passing Item just as a type parameter without any & works fine at least https://github.com/Marwes/tokio/tree/type_parameter_encoder .

djc · 2020-01-21T09:04:06Z

But in that approach can I also pass in a &'a MyStruct? Would AsRef<MyStruct> also work for that?

djc · 2020-01-21T09:06:50Z

Also the requirement to parametrize the Sink method calls with the item type seems a little ugly.

Marwes · 2020-01-21T09:11:52Z

But in that approach can I also pass in a &'a MyStruct? Would AsRef also work for that?

Sure. See LinesCodec and BytesCodec which take I: AsRef<_>. &'a MyStruct works with the suggestion in #1746 (comment) E: for<'a> Encoder<&'a MyStruct>

Also the requirement to parametrize the Sink method calls with the item type seems a little ugly.

That is only because start_send/poll_flush is called which do not refer to Item. For normal use Item is passed and everything is figured out.

Marwes · 2020-01-21T09:12:56Z

The main downside is that some Encoder<I> bounds has to be removed to let type inference work without needing to specify Framed::new::<&str> etc.

djc · 2020-01-21T09:33:38Z

The main downside is that some Encoder<I> bounds has to be removed to let type inference work without needing to specify Framed::new::<&str> etc.

How is that a downside, exactly?

Marwes · 2020-01-21T09:46:44Z

If you accidentally pass something that does not implement Encoder then the error message ends up further away from where the actual problem is. Not a huge issue but I had this problem in https://github.com/Marwes/combine and had to stick PhantomData everywhere due to inference problems like this.

It is probably not that big of an issue here but in combine Parser<Input> types are so deeply nested that omitting the bound was enough of an issue that I had to keep it (and keep PhantomData around everywhere to fix the type parameter)

djc · 2020-02-21T14:23:48Z

I think this is ready for an actual review now.

@Marwes thanks for your changes. I've squashed our changes together, but added you as a co-author to the resulting commit.

LucioFranco

Looks good overall just two questions

tokio-util/src/codec/decoder.rs

LucioFranco · 2020-02-21T15:53:50Z

tokio-util/src/codec/length_delimited.rs

@@ -546,12 +543,12 @@ impl Decoder for LengthDelimitedCodec {
    }
 }

-impl Encoder for LengthDelimitedCodec {
-    type Item = Bytes;
+impl<T> Encoder<T> for LengthDelimitedCodec where T: AsRef<[u8]> {


I think it may make sense to keep this Encoder<Bytes>?

Why do you think so? This is strictly more general.

tokio-util/src/codec/bytes_codec.rs

tokio-util/src/codec/length_delimited.rs

LucioFranco

This looks mostly good just a few questions, once we get this merged I'll get a 0.3 release out. Sorry for the delay!

LucioFranco

👍 I want to see LengthDelimitedCodec take a Bytes as well. Then we can get this release out, sorry for the super long delay on all this!

This allows passing references into `Encoder`s, which are going to encode the item into a given `BytesMut` anyway. Co-authored-by: Markus Westerlind <markus.westerlind@distilnetworks.com>

djc force-pushed the encoder-ref branch from 7bf729c to 090b739 Compare November 7, 2019 11:36

djc force-pushed the encoder-ref branch from e2a7a70 to 9ae6a9f Compare November 9, 2019 13:48

Marwes reviewed Nov 9, 2019

View reviewed changes

djc force-pushed the encoder-ref branch 2 times, most recently from 78add5f to 9cf3149 Compare November 27, 2019 20:07

djc force-pushed the encoder-ref branch from 9cf3149 to ccd68e2 Compare January 20, 2020 13:45

djc force-pushed the encoder-ref branch 2 times, most recently from c45296c to 8bd8a83 Compare February 21, 2020 14:22

LucioFranco reviewed Feb 21, 2020

View reviewed changes

djc force-pushed the encoder-ref branch 2 times, most recently from 3aa5a94 to 0a705a8 Compare February 28, 2020 19:53

LucioFranco reviewed Mar 3, 2020

View reviewed changes

tokio-util/src/codec/bytes_codec.rs Outdated Show resolved Hide resolved

LucioFranco reviewed Mar 3, 2020

View reviewed changes

tokio-util/src/codec/length_delimited.rs Outdated Show resolved Hide resolved

LucioFranco reviewed Mar 3, 2020

View reviewed changes

Ignore hanging test on macOS (see tokio-rs#2268)

84b2e78

djc force-pushed the encoder-ref branch from 0a705a8 to c745020 Compare March 4, 2020 19:44

LucioFranco approved these changes Mar 4, 2020

View reviewed changes

djc force-pushed the encoder-ref branch from c745020 to 4d514d9 Compare March 4, 2020 20:07

djc and others added 2 commits March 4, 2020 21:10

codec: change Encoder to take generic Item parameter

df3b880

This allows passing references into `Encoder`s, which are going to encode the item into a given `BytesMut` anyway. Co-authored-by: Markus Westerlind <markus.westerlind@distilnetworks.com>

chore: prepare tokio-util 0.3.0 release

d8ba314

djc force-pushed the encoder-ref branch from 4d514d9 to d8ba314 Compare March 4, 2020 20:10

LucioFranco merged commit 9d4d076 into tokio-rs:master Mar 4, 2020

halzy mentioned this pull request Mar 30, 2020

Upgrade to tokio-util v0.3.x halzy/stream_multiplexer#35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

codec: change Encoder to take &Item #1746

codec: change Encoder to take &Item #1746

djc commented Nov 7, 2019

djc commented Nov 7, 2019

Marwes commented Nov 7, 2019

hawkw commented Nov 7, 2019

djc commented Nov 8, 2019

djc commented Nov 9, 2019

Marwes Nov 9, 2019

djc commented Nov 9, 2019

Marwes commented Nov 9, 2019

djc commented Nov 18, 2019

carllerche commented Nov 27, 2019

djc commented Nov 27, 2019

djc commented Nov 29, 2019

djc commented Dec 6, 2019

carllerche commented Dec 6, 2019

taiki-e commented Jan 20, 2020

djc commented Jan 20, 2020 •

edited

Loading

LucioFranco commented Jan 21, 2020

djc commented Jan 21, 2020

Marwes commented Jan 21, 2020

djc commented Jan 21, 2020

djc commented Jan 21, 2020

Marwes commented Jan 21, 2020

Marwes commented Jan 21, 2020

djc commented Jan 21, 2020

Marwes commented Jan 21, 2020

djc commented Feb 21, 2020

LucioFranco left a comment

LucioFranco Feb 21, 2020

djc Feb 21, 2020

LucioFranco left a comment

LucioFranco left a comment

codec: change Encoder to take &Item #1746

codec: change Encoder to take &Item #1746

Conversation

djc commented Nov 7, 2019

djc commented Nov 7, 2019

Marwes commented Nov 7, 2019

hawkw commented Nov 7, 2019

djc commented Nov 8, 2019

djc commented Nov 9, 2019

Marwes Nov 9, 2019

Choose a reason for hiding this comment

djc commented Nov 9, 2019

Marwes commented Nov 9, 2019

djc commented Nov 18, 2019

carllerche commented Nov 27, 2019

djc commented Nov 27, 2019

djc commented Nov 29, 2019

djc commented Dec 6, 2019

carllerche commented Dec 6, 2019

taiki-e commented Jan 20, 2020

djc commented Jan 20, 2020 • edited Loading

LucioFranco commented Jan 21, 2020

djc commented Jan 21, 2020

Marwes commented Jan 21, 2020

djc commented Jan 21, 2020

djc commented Jan 21, 2020

Marwes commented Jan 21, 2020

Marwes commented Jan 21, 2020

djc commented Jan 21, 2020

Marwes commented Jan 21, 2020

djc commented Feb 21, 2020

LucioFranco left a comment

Choose a reason for hiding this comment

LucioFranco Feb 21, 2020

Choose a reason for hiding this comment

djc Feb 21, 2020

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

djc commented Jan 20, 2020 •

edited

Loading