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

Use futures AsyncRead, AsyncWrite, ready! macro everywhere #1297

Closed
95th opened this issue Jul 14, 2019 · 11 comments
Closed

Use futures AsyncRead, AsyncWrite, ready! macro everywhere #1297

95th opened this issue Jul 14, 2019 · 11 comments

Comments

@95th
Copy link
Contributor

95th commented Jul 14, 2019

Now that futures crate has AsyncRead and AsyncWrite traits and ready! Macro, I think we should use those and re-export them in tokio.

Is there a reason we have not done this so far?

@carllerche
Copy link
Member

ready! was previously not available in futures-core. It is now, so we can switch to that.

As for AsyncRead / AsyncWrite, that was mentioned here.

I will keep this issue open for a bit in case there is follow up discussion.

@95th
Copy link
Contributor Author

95th commented Jul 14, 2019

Thanks.. I'll submit a PR for ready!. About AsyncRead/AsyncWrite, I had the same concern as mentioned in that conversation. But it is just a concern, I have not run into any such compatibility issues yet.

@Kvikal
Copy link

Kvikal commented Jul 19, 2019

I have run into an issue regarding this. I wanted to use BufReader with tokio_tcp::split::TcpStreamReadHalf, but Tokio itself does not provide one (unless I missed it).
Futures has futures::io::BufReader, but it is built using different AsyncRead trait then the one used by tokio_tcp::split::TcpStreamReadHalf.

It would certainly be nice, if tokio/futures shared the same AsyncRead/AsyncWrite traits.

@LucioFranco
Copy link
Member

@Kvikal there is this https://github.com/tokio-rs/tokio/blob/master/tokio-io/src/async_buf_read.rs

@Nemo157
Copy link
Contributor

Nemo157 commented Jul 23, 2019

If you do need compatibility between libraries using tokio's traits and libraries using futures' traits for testing stuff now, I threw together a shim: https://github.com/Nemo157/futures-tokio-compat. As the readme mentions this is purely there to allow experimental testing (as I wanted to try using new hyper integrated with a library that was already based around futures' IO/task spawning traits), I have no plans on publishing it.

@LucioFranco
Copy link
Member

@Nemo157 this seems really nice! We may want to bring this into actual tokio-io under a futures-compat feature?

@Nemo157
Copy link
Contributor

Nemo157 commented Jul 23, 2019

My hope is that there is only one set of traits in the end, I don't see any reason to have multiple of them (at least, when they're this similar, if they were more differentiated then they might be useful in different usecases). The only result of that I see is fragmentation in the ecosystem requiring compat shims like this everywhere.

Also note that the compat layer there is not zero-cost, and as far as I know cannot be; for spawning it requires extra boxing in one direction, and the IO traits don't proxy the buffer initialization so they will fall back to zeroing on both sides (it is possible to proxy it one direction at least, but I didn't feel like guaranteeing that unsafe code).

But, either way I'll stick an Apache/MIT license on that repo, so if you do wish to pull it into tokio feel free to do so.

@LucioFranco
Copy link
Member

@Nemo157 sorry, I didn't mean that we should have it in there as the goal around keeping it there forever. Mostly around the thought, while we play around with the traits users can easily continue to work with other libraries. I do hope, in the end, there is only one set of traits 😄.

@o01eg
Copy link

o01eg commented Oct 19, 2019

What if at least move provided AsyncRead::poll_read_buf and AsyncWrite::poll_write_buf to *Ext traits?

@carllerche
Copy link
Member

@o01eg these functions are intended to be implemented as needed. This prevents moving them to ext traits.

kevinastone added a commit to kevinastone/gotham that referenced this issue Nov 5, 2019
kevinastone added a commit to kevinastone/gotham that referenced this issue Nov 8, 2019
With the anticipated release of async-await in stable rust this week, I took an effort to migrate Gotham to run on std futures using the pre-release versions of the dependencies (tokio, hyper, etc).

This doesn't attempt to introduce `await` or `async fn` into the codebase (there was a single unavoidable case due to an `tokio::File::metadata` API change).  That is designed as a future activity.

This migration involved a few key efforts:

1. Convert from Futures 0.1 to Futures-preview 0.3 (mostly `Future<Item=>` to `Future<Output=Result<>>`).
2. Update dependencies to pre-release versions (`tokio-0.2` and `hyper-0.13`).  There's still other dependencies that are outstanding and blocking the full release.
3. Migrate Handler trait to a pinned box HandlerFuture.

This is a work-in-progress with a few blockers before this would be ready:

Gotham Dependencies:

[ ] Update Futures from `futures-preview` to `futures = 0.3` when the other dependencies (hyper, tokio, etc) update in concert.
[ ] Update Tokio to `0.2` from alpha pre-releases
[ ] Update Hyper to `0.13` from alpha pre-releases
[ ] Update Tower-Service to `0.3` from alpha pre-releases.  Hyper is migrating many of its traits to `tower-service::Service` and so is now a direct dependency.
[ ] Released version of `futures_rustls` which is currently a branch of `tokio-rustls` ported to Futures-preview
[ ] Released version of `futures-tokio-compat` or suggested `tokio::compat` library for bridging `futures::AsyncRead` and `tokio::AsyncRead`.  See tokio-rs/tokio#1297
    and async-rs/async-std#54

Middleware Dependencies:

[ ] Diesel - Requires updated release of `tokio-threadpool`
[ ] JWT - Requires updated release of `jsonwebtoken`
kevinastone added a commit to kevinastone/gotham that referenced this issue Nov 8, 2019
With the anticipated release of async-await in stable rust this week, I took an effort to migrate Gotham to run on std futures using the pre-release versions of the dependencies (tokio, hyper, etc).

*NOTE*: This doesn't attempt to introduce `await` or `async fn` into the codebase (there was a single unavoidable case due to an `tokio::File::metadata` API change).  That is designed as a future activity.

This migration involved a few key efforts:

1. Convert from Futures 0.1 to Futures-preview 0.3 (mostly `Future<Item=>` to `Future<Output=Result<>>`).
2. Update dependencies to pre-release versions (`tokio-0.2` and `hyper-0.13`).  There's still other dependencies that are outstanding and blocking the full release.
3. Migrate Handler trait to a pinned box HandlerFuture.

This is a **work-in-progress** with a few blockers before this would be ready:

Gotham Dependencies:

- [ ] Update Futures from `futures-preview` to `futures = 0.3` when the other dependencies (hyper, tokio, etc) update in concert.
- [ ] Update Tokio to `0.2` from alpha pre-releases
- [ ] Update Hyper to `0.13` from alpha pre-releases
- [ ] Update Tower-Service to `0.3` from alpha pre-releases.  Hyper is migrating many of its traits to `tower-service::Service` and so is now a direct dependency.
- [ ] Released version of `futures_rustls` which is currently a branch of `tokio-rustls` ported to Futures-preview
- [ ] Released version of `futures-tokio-compat` or suggested `tokio::compat` library for bridging `futures::AsyncRead` and `tokio::AsyncRead`.  See tokio-rs/tokio#1297
    and async-rs/async-std#54

Middleware Dependencies:

- [ ] Diesel - Requires updated release of `tokio-threadpool`
- [ ] JWT - Requires updated release of `jsonwebtoken`
kevinastone added a commit to kevinastone/gotham that referenced this issue Nov 8, 2019
With the anticipated release of async-await in stable rust this week, I took an effort to migrate Gotham to run on std futures using the pre-release versions of the dependencies (tokio, hyper, etc).

*NOTE*: This doesn't attempt to introduce `await` or `async fn` into the codebase (there was a single unavoidable case due to an `tokio::File::metadata` API change).  That is designed as a future activity.

This migration involved a few key efforts:

1. Convert from Futures 0.1 to Futures-preview 0.3 (mostly `Future<Item=>` to `Future<Output=Result<>>`).
2. Update dependencies to pre-release versions (`tokio-0.2` and `hyper-0.13`).  There's still other dependencies that are outstanding and blocking the full release.
3. Migrate Handler trait to a pinned box HandlerFuture.

This is a **work-in-progress** with a few blockers before this would be ready:

Gotham Dependencies:

- [ ] Update Futures from `futures-preview` to `futures = 0.3` when the other dependencies (hyper, tokio, etc) update in concert.
- [ ] Update Tokio to `0.2` from alpha pre-releases
- [ ] Update Hyper to `0.13` from alpha pre-releases
- [ ] Update Tower-Service to `0.3` from alpha pre-releases.  Hyper is migrating many of its traits to `tower-service::Service` and so is now a direct dependency.
- [ ] Released version of `futures_rustls` which is currently a branch of `tokio-rustls` ported to Futures-preview
- [ ] Released version of `futures-tokio-compat` or suggested `tokio::compat` library for bridging `futures::AsyncRead` and `tokio::AsyncRead`.  See tokio-rs/tokio#1297
    and async-rs/async-std#54

Middleware Dependencies:

- [ ] Diesel - Requires updated release of `tokio-threadpool`
- [ ] JWT - Requires updated release of `jsonwebtoken` since it's dependency `ring` conflicts withs `futures-rustls`.
kevinastone added a commit to kevinastone/gotham that referenced this issue Nov 8, 2019
With the anticipated release of async-await in stable rust this week, I took an effort to migrate Gotham to run on std futures using the pre-release versions of the dependencies (tokio, hyper, etc).

*NOTE*: This doesn't attempt to introduce `await` or `async fn` into the codebase (there was a single unavoidable case due to an `tokio::File::metadata` API change).  That is designed as a future activity.

This migration involved a few key efforts:

1. Convert from Futures 0.1 to Futures-preview 0.3 (mostly `Future<Item=>` to `Future<Output=Result<>>`).
2. Update dependencies to pre-release versions (`tokio-0.2` and `hyper-0.13`).  There's still other dependencies that are outstanding and blocking the full release.
3. Migrate Handler trait to a pinned box HandlerFuture.

This is a **work-in-progress** with a few blockers before this would be ready:

Gotham Dependencies:

- [ ] Update Futures from `futures-preview` to `futures = 0.3` when the other dependencies (hyper, tokio, etc) update in concert.
- [ ] Update Tokio to `0.2` from alpha pre-releases
- [ ] Update Hyper to `0.13` from alpha pre-releases
- [ ] Update Tower-Service to `0.3` from alpha pre-releases.  Hyper is migrating many of its traits to `tower-service::Service` and so is now a direct dependency.
- [ ] Released version of `futures_rustls` which is currently a branch of `tokio-rustls` ported to Futures-preview
- [ ] Released version of `futures-tokio-compat` or suggested `tokio::compat` library for bridging `futures::AsyncRead` and `tokio::AsyncRead`.  See tokio-rs/tokio#1297
    and async-rs/async-std#54

Middleware Dependencies:

- [ ] Diesel - Requires updated release of `tokio-threadpool`
- [ ] JWT - Requires updated release of `jsonwebtoken` since it's dependency `ring` conflicts withs `futures-rustls`.
@carllerche
Copy link
Member

Discussion is primarily in #1744 now.

colinbankier pushed a commit to gotham-rs/gotham that referenced this issue Jan 20, 2020
* Ported examples to Std::Future

The following examples were not updated (and thus commented out in `Cargo.toml`) due to incompatible dependencies:

- [ ] `hello_world_until` is dependent on a compatible version of `tokio-signal`
- [ ] `diesel` is dependent on the Diesel middleware
- [ ] `openssl` is dependent on `tokio-openssl`
- [ ] `websocket` is dependent on `tokio-tungstenite`

* Migrate Gotham to std Futures to support Async/Await

With the anticipated release of async-await in stable rust this week, I took an effort to migrate Gotham to run on std futures using the pre-release versions of the dependencies (tokio, hyper, etc).

*NOTE*: This doesn't attempt to introduce `await` or `async fn` into the codebase (there was a single unavoidable case due to an `tokio::File::metadata` API change).  That is designed as a future activity.

This migration involved a few key efforts:

1. Convert from Futures 0.1 to Futures-preview 0.3 (mostly `Future<Item=>` to `Future<Output=Result<>>`).
2. Update dependencies to pre-release versions (`tokio-0.2` and `hyper-0.13`).  There's still other dependencies that are outstanding and blocking the full release.
3. Migrate Handler trait to a pinned box HandlerFuture.

This is a **work-in-progress** with a few blockers before this would be ready:

Gotham Dependencies:

- [ ] Update Futures from `futures-preview` to `futures = 0.3` when the other dependencies (hyper, tokio, etc) update in concert.
- [ ] Update Tokio to `0.2` from alpha pre-releases
- [ ] Update Hyper to `0.13` from alpha pre-releases
- [ ] Update Tower-Service to `0.3` from alpha pre-releases.  Hyper is migrating many of its traits to `tower-service::Service` and so is now a direct dependency.
- [ ] Released version of `futures_rustls` which is currently a branch of `tokio-rustls` ported to Futures-preview
- [ ] Released version of `futures-tokio-compat` or suggested `tokio::compat` library for bridging `futures::AsyncRead` and `tokio::AsyncRead`.  See tokio-rs/tokio#1297
    and async-rs/async-std#54

Middleware Dependencies:

- [ ] Diesel - Requires updated release of `tokio-threadpool`
- [ ] JWT - Requires updated release of `jsonwebtoken` since it's dependency `ring` conflicts withs `futures-rustls`.

* Updated to crates.io verison of futures-rustls

* Migrated to tokio-0.2 final and hyper-0.13 final

This involved changing the way that the runtime is managed and handling
`block_on` for synchronously running futures.

Hyper-0.13 required changing to the tower_service trait for implementing
a `Connect` trait used for the TLS and plain tcp stream wrappers.

The other major change is that Hyper migrated from a `Chunk` trait to
use `Bytes`.  Bytes-0.5 is now a trait rather than a struct which no
longer implements `Extends<u8>` requiring a new `BytesMut` buffer for
some common concatenation operations.

* Re-enable hello_world_until

Replace tokio-signal with tokio (signal was migrated into tokio itself)

* Remove commented out code

Co-Authored-By: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>

* Async-ified the init_server helpers

* Simplify executing futures in tls/test to match plain/test

* Re-enabled diesel middleware and example

* Addressed review comments.

Co-authored-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
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

No branches or pull requests

7 participants