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

[WIP] Upgrade tokio to tokio 0.2 #214

Closed
wants to merge 18 commits into from

Conversation

fisherdarling
Copy link

@fisherdarling fisherdarling commented Aug 30, 2019

This PR upgrades the async portion of flate2 to use the new tokio 0.2 AsyncRead and AsyncWrite types.

The main issue with this refactor is that the AsyncRead and AsyncWrite types do not have to implement Read and Write respectively. This means the generic constraints on the read::* and write::* encoders and decoders in flate2 are now incompatible with the new tokio 0.2 types.

This means that types like read::ZlibDecoder<TcpStreamReadHalf> do not compile, since TcpStreamReadHalf does not impl Read.

@alexcrichton
Copy link
Member

Thanks for this! I don't know personally how best to integrate with 0.2, I don't know how it's designed. I think it'd be best to name this a tokio02 feature or something like that, and then we could ideally move all the impls to a tokio02 module which would hose all the implementation details here.

@fisherdarling
Copy link
Author

@alexcrichton Hey, I think I got a working tokio2 async version of deflate. Under the tokio2 folder in src, there is a copy of each compression kind, for each of those kinds I have implemented AsyncRead/Write. Currently only deflate has a working tokio2 AsyncRead/Write implementation, there are just dummy impls for the Gz and zlib modules. The read and write versions of deflate are reexported under {read, write, bufread}::tokio2::deflate::*.

In order to do this I had to write my own AsyncWriter in the tokio2 version of zio.rs which replaced the sync zio::Writer. I am worried that this new Writer may not be working with the underlying miniz stream correctly, I was hoping you could look into it? I have a roundtrip compression/decompression test under tests/tokio.rs that works (it's really just a PoC). That test also tests the async encoder and decoder data with a sync encoder and decoder.

Another part of this was separating the tokio into two features tokio1 and tokio2. I am not very happy with how I am separating this feature, thoughts?

Let me know what you think of the changes. I'm wondering if we should just merge deflate first (when you think it's ready), and work on the others afterwards.

Thanks

@alexcrichton
Copy link
Member

Thanks for this! The changes definitely look very extensive here, I think going so far as to add new GzEncoder and such types, right? If that's the case this is probably best done as a separate crate, but if we can reuse the same existing types that would be great to keep it all in the same crate. I'm not sure how much of the internals need to change between the async/sync versions though?

I think it's fine to just focus on deflate for now, can always add the others later. For the features, I would prefer to not break the existing tokio feature, so I think it's fine to just add a tokio2 feature as a new feature.

@Nemo157
Copy link
Member

Nemo157 commented Oct 15, 2019

I just happened to stumble across this issue, I'm maintaining a crate async-compression that's providing bindings to futures::io::{AsyncRead, AsyncWrite} using the underlying implementation from here (along with other algorithms from various compression crates). Those traits are mostly identical to what tokio(0.2) provides, so I've been contemplating expanding it to also provide the tokio versions, but have been sort of waiting to see what happens with the ecosystem (having this identical pair of traits long-term seems unlikely).

@DCjanus
Copy link

DCjanus commented Nov 23, 2020

ping

@Nemo157
Copy link
Member

Nemo157 commented Nov 23, 2020

(There's now tokio 0.3 too, which has new incompatible traits. async-compression has support for using flate2 with both 0.2 and 0.3.)

@alexcrichton
Copy link
Member

Oh nice! I think it would probably be best to deprecate the support in favor of the async-compression crate then?

@novacrazy
Copy link

There is now Tokio 1.0

@Expyron Expyron mentioned this pull request Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants