Skip to content
This repository has been archived by the owner on May 6, 2018. It is now read-only.

Consider Removing the io::Read and io::Write supertraits on AsyncRead and AsyncWrite #83

Closed
cramertj opened this issue Nov 7, 2017 · 5 comments
Milestone

Comments

@cramertj
Copy link
Contributor

cramertj commented Nov 7, 2017

Many of the methods on io::Read and io::Write, such as write_all, are not appropriate to call in an async context. Furthermore, lots of code using io::Read and io::Write expects (correctly or not) that those interfaces are blocking. Removing the supertraits would make it harder to misuse the AsyncRead and AsyncWrite types as io::Read and io::Write types, and would allow moving the io::XXX functions to methods on the AsyncRead and AsyncWrite traits (this last point is a benefit because methods are more easily discoverable than free functions). This change could also allow AsyncRead and AsyncWrite types with non-io::Error error types.

There are places in which it makes sense to provide an async-readable/writeable type as an io::Read or io::Write. That functionality can still be provided by choosing to opt-in to an io::Read or io::Write impl, or by using some form of wrapper type which allows using an AsyncRead/AsyncWrite as an io::Read/io::Write (basically the inverse of #76).

cc @carllerche

@carllerche carllerche added this to the v0.2 milestone Nov 7, 2017
@alexcrichton
Copy link
Contributor

One of the biggest motivational factors for the current design was for integration with the openssl crate. Essentially the idea was that we shouldn't have to rewrite all of the openssl crate to work with async I/O but rather the crate should be able to work in both modes with little duplication. This also applies to other crates like flate2 and such where the desire is to have a (hopefully not duplicated) surface area which supports both async I/O and synchronous I/O.

In that sense it seems like this would theoretically solve that constraint but I'd recommend getting the legwork done to prove this out if we're seriously considering this. I'm personally pretty wary of the impact this would have on downstream crates for what I'd see as not a whole lot of benefit, although I could be wrong!

@cramertj
Copy link
Contributor Author

cramertj commented Nov 8, 2017

getting the legwork done to prove this out

Can you explain more what you're looking for here? I'd be happy to put in some time on this, but I don't see why any changes would be needed to openssl at all.

@alexcrichton
Copy link
Contributor

Oh sorry sure yeah. What I mean is that openssl shouldn't need modifications (in that it's already "async compatible"), but I'd want to game out the ergonomic overhead of using openssl with tokio-io

@carllerche
Copy link
Member

As a counter point, I am pretty sure that openssl really isn't correct (same with the other TLS libs, see tokio-rs/tokio-tls#33).

flate2 is probably fine.

If AsyncRead no longer implemented io::Read, what would probably happen would be there would be a wrapper:

trait AsyncRead {
    fn into_std(self) -> Std(Self);

    fn as_std(&mut self) -> Std(&mut self);
}

impl<T: AsyncRead> io::Read for Std(T) {
    // ...
}

@carllerche
Copy link
Member

The tokio-io crate has been moved into the tokio git repo.

I created a new issue over there. See link above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants