tokio::io::* futures depend on details of std::io::Read and Write implementations about tokio-core HOT 22 CLOSED

@aturon, @carllerche, and I discussed this the other day and our conclusions were to hit a few birds with one stone. In summary, we're thinking we'll:

• Create a new tokio-io crate
• Add AsyncRead and AsyncWrite to this crate
• Add poll_{read,write} to these traits
• Move all combinators to this crate, including framing
• Eventually extend the crate with bytes-related helpers
• Change all combinators to work with AsyncRead and AsyncWrite
• Deprecate the tokio_core::io module during the 0.1 release series.

I've begun prototyping this crate and have migrated tokio-core and tokio-tls as a proof of concept.

Such a new crate is intended to close these issues:

• #61 - all generics work with AsyncRead and AsyncWrite, explicitly demarcating where async is needed and preventing standard objects from being leaked in by accident
• #62 - there's no more Io trait
• #68 - we'll integrate with bytes
• #81 - we'll integrate with bytes
• #86 - refactoring can be implemented immediately
• #119 - we're moving out traits
• #123 - with bytes integration this should be possible
• #135 - we can rename immediately

The downside of this transition is "yet another crate", but the pros seem to outweigh the cons overall in this case.

from tokio-core.

As an update https://github.com/alexcrichton/tokio-io is nearing the 0.1 release (I've been commenting on a number of associated issues) and I plan on using this to track the migration.

If you're interested in this (or associated issues) please feel free to take a look and report any issues you find!

from tokio-core.

Thanks for the report! We discussed this yesterday and reached a similar conclusion as well. We're also thinking of adding a dependency on the bytes crate which would also add a very nice vector for adding new traits with new suites of methods as well.

from tokio-core.

Discussed with @dwrensha tonight and an important benefit of the current scheme which is that the traits are not connected to tokio-core but rather the standard library. The point was made that Read and Write (even the async versions proposed here) are all independent of tokio-core itself, so otherwise to be abstract over these traits you'd have to pull in an event loop you might not use.

That may just mean that this crate is not the appropriate location for it, however and we could perhaps either add a new crate or add it to the futures crate.

from tokio-core.

To be more concrete, I would like capnp-futures-rust to continue not needing to depend on tokio-core.

from tokio-core.

It's certainly not appropriate for tokio to have dependencies on tokio-specific implementation details of standard traits, so that just leaves making them an external crate.

from tokio-core.

At least one person has now been bitten by this.

from tokio-core.

Yeah, I spent a lot of time trying to understand how loops work with tokio_core::io::Read (polling or thread pool like libuv) in case of Read<std::fs::File, _>, because it does not accept Handle explicitly.

P.S. What is the canonical way to read files now?

from tokio-core.

I don't believe tokio currently supports file IO at all, though per this issue the type signatures confusingly suggest otherwise. If you're not building a high-performance database engine, you're likely best off implementing it with a threadpool.

from tokio-core.

Just got bitten by this -- was really confused why nothing was being run.

It would be nice have a warning in the docs like "if you use std::io::stdin for any of the tokio_core::io::read_* functions, you'll be sorry."

Ended up making a crate to work around this problem at the cost of cross-platform support.

from tokio-core.

Could this somehow interact with genio?

from tokio-core.

Something to consider here is that this will make composing third-party Read types harder.

Right now, I can plug a BzDecoder with a Read object that is internally async and have that combination Just Work. BzDecoder sees the WouldBlock errors, doesn't know what to do with them, then just goes ahead and forwards them -- this is exactly what we want. With a custom AsyncRead trait will that be possible? An AsyncRead plugged into a Read should be AsyncRead.

from tokio-core.

@Sid0 this will still be supported. It will require "lifting" the BzDecoder type:

struct AsyncBzDecoder<T>(BzDecoder<T>);

and then impl AsyncRead / AsyncWrite where T: AsyncRead / AsyncWrite.

Hopefully this makes sense.

from tokio-core.

Ah, I guess that shouldn't be too bad. Still possibly something to flag in the documentation though.

from tokio-core.

@alexcrichton I've noticed the unsafe fn prepare_uninitialized_buffer(...). If I understand it correctly, it tries to improve performance by skipping initialization of a buffer. I dealt with something similar in my genio crate. I used unsafe marker trait to solve the issue (the read and read_exact functions are not unsafe to call but unsafe to implement).

Can you explain why you chose other approach?

from tokio-core.

@carllerche may have more to say here as well.

I think one aspect it covers is object safety? That is, a Box<AsyncRead> will preserve whether it initializes a buffer or not.

from tokio-core.

Yeah, object safety, less traits, and no need for specialization are valid reasons.

from tokio-core.

Thank you! Can you explain why the function is unsafe to call? I'm unsure how it should work. (Maybe an example would help.)

from tokio-core.

@Kixunil it's moreso unsafe to override rather than to call, so strictly speaking it's sort of just using unsafe as a "please be careful when implementing this" rather than "please be careful calling this". The function body can have access to uninitialized memory through the slice argument.

from tokio-core.

@alexcrichton Thank you for explaining! I feel bit uneasy seeing it because AFAIK convention is that unsafe fn ... means "unsafe to call" and unsafe trait ... "unsafe to implement". I fear that it might cause confusion, maybe even leading to memory bugs.

If you think that putting unsafe on trait itself or separating it into other trait isn't good idea, could you at least put info into documentation, please?

from tokio-core.

Oh yeah it's definitely not a "perfect" use of unsafe, but it hopefully gets the job done for now! I definitely agree the documentation should be quite thorough about why it's unsafe

from tokio-core.

Ok!

I've now published the tokio-io crate on crates.io and over the next few hours I'll be publishing a bunch of other crates to take advantage of it. We'll also be posting more information in the near future about this transition, so stay tuned!

from tokio-core.