This repository has been archived by the owner. It is now read-only.

tokio::io::* futures depend on details of std::io::Read and Write implementations #61

Closed
Ralith opened this Issue Oct 19, 2016 · 22 comments

Comments

Projects
None yet
8 participants
@Ralith
Contributor

Ralith commented Oct 19, 2016

These futures call std::io::Read methods and expect them to internally register the task's interest in an event if it's not immediately ready. This isn't generally true for implementations of Read and Write implementations not based on PollEvented. Tokio could introduce a new AsyncRead and AsyncWrite traits to make this requirement explicit.

@Ralith Ralith changed the title from `tokio::io::*` futures depend on details of `std::io::Read` and `Write` implementations to tokio::io::* futures depend on details of std::io::Read and Write implementations Oct 19, 2016

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Oct 19, 2016

Contributor

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.

Contributor

alexcrichton commented Oct 19, 2016

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.

@alexcrichton alexcrichton added this to the 0.2 release milestone Oct 19, 2016

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Oct 28, 2016

Contributor

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.

Contributor

alexcrichton commented Oct 28, 2016

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.

@dwrensha

This comment has been minimized.

Show comment
Hide comment
@dwrensha

dwrensha Oct 28, 2016

Contributor

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

Contributor

dwrensha commented Oct 28, 2016

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

@Ralith

This comment has been minimized.

Show comment
Hide comment
@Ralith

Ralith Oct 28, 2016

Contributor

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.

Contributor

Ralith commented Oct 28, 2016

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.

@Ralith

This comment has been minimized.

Show comment
Hide comment
Contributor

Ralith commented Nov 28, 2016

@loyd

This comment has been minimized.

Show comment
Hide comment
@loyd

loyd Jan 2, 2017

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?

loyd commented Jan 2, 2017

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?

@Ralith

This comment has been minimized.

Show comment
Hide comment
@Ralith

Ralith Jan 2, 2017

Contributor

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.

Contributor

Ralith commented Jan 2, 2017

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.

@Rufflewind

This comment has been minimized.

Show comment
Hide comment
@Rufflewind

Rufflewind Feb 5, 2017

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.

Rufflewind commented Feb 5, 2017

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.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Feb 6, 2017

Contributor

@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.

Contributor

alexcrichton commented Feb 6, 2017

@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.

@Kixunil

This comment has been minimized.

Show comment
Hide comment
@Kixunil

Kixunil Feb 15, 2017

Could this somehow interact with genio?

Kixunil commented Feb 15, 2017

Could this somehow interact with genio?

@sunshowers

This comment has been minimized.

Show comment
Hide comment
@sunshowers

sunshowers Feb 27, 2017

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.

sunshowers commented Feb 27, 2017

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.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Feb 27, 2017

Member

@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.

Member

carllerche commented Feb 27, 2017

@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.

@sunshowers

This comment has been minimized.

Show comment
Hide comment
@sunshowers

sunshowers Feb 27, 2017

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

sunshowers commented Feb 27, 2017

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 6, 2017

Contributor

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!

Contributor

alexcrichton commented Mar 6, 2017

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!

@Kixunil

This comment has been minimized.

Show comment
Hide comment
@Kixunil

Kixunil Mar 10, 2017

@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?

Kixunil commented Mar 10, 2017

@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?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 10, 2017

Contributor

@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.

Contributor

alexcrichton commented Mar 10, 2017

@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.

@carllerche

This comment has been minimized.

Show comment
Hide comment
@carllerche

carllerche Mar 10, 2017

Member

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

Member

carllerche commented Mar 10, 2017

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

@Kixunil

This comment has been minimized.

Show comment
Hide comment
@Kixunil

Kixunil Mar 11, 2017

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

Kixunil commented Mar 11, 2017

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 12, 2017

Contributor

@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.

Contributor

alexcrichton commented Mar 12, 2017

@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.

@Kixunil

This comment has been minimized.

Show comment
Hide comment
@Kixunil

Kixunil Mar 13, 2017

@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?

Kixunil commented Mar 13, 2017

@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?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 13, 2017

Contributor

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

Contributor

alexcrichton commented Mar 13, 2017

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

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Mar 15, 2017

Contributor

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!

Contributor

alexcrichton commented Mar 15, 2017

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!

Kixunil added a commit to Kixunil/tokio-io that referenced this issue Mar 15, 2017

Added note about unsafe to doc about AsyncRead::prepare_uninitialized…
…_buffer()

As discussed in [tokio-rs#61 of tokio-core](tokio-rs/tokio-core#61 (comment)), documentation is added to explain the purpose of `unsafe` on this fn.

en pushed a commit to en/tokio-core that referenced this issue Jun 27, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.