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

io: misc improvements #1256

Closed
4 tasks done
carllerche opened this issue Jul 4, 2019 · 7 comments
Closed
4 tasks done

io: misc improvements #1256

carllerche opened this issue Jul 4, 2019 · 7 comments
Milestone

Comments

@carllerche
Copy link
Member

carllerche commented Jul 4, 2019

Tracking misc smaller improvements to tokio-io.

This issue is to be closed once there are no remaining breaking changes to make or remaining breaking changes are tracked in separate issues.

Features

Polish

  • module layout
@carllerche carllerche added this to the v0.2 milestone Jul 4, 2019
@taiki-e
Copy link
Member

taiki-e commented Jul 25, 2019

#1301 just added traits and combinators. We should add AsyncBufRead implementations to some types. (added check box)

EDIT: It seems that there were fewer types that could implement this than I had thought.
EDIT2: It seems this is done.

@jimmycuadra
Copy link

jimmycuadra commented Aug 5, 2019

Can someone clarify which types are intended to implement AsyncBufRead? I was just trying to figure out how to loop over the lines of stdin and discovered tokio::io::Stdin doesn't implement this trait, which led me to this issue. Sounds like this is intended but just hasn't been done yet?

@carllerche
Copy link
Member Author

@jimmycuadra any type that can implement it should.

@jebrosen
Copy link
Contributor

jebrosen commented Oct 3, 2019

This looks like a good place to bring this up: in tokio 0.1, AsyncRead was implemented for some types in std::io such as Repeat, Chain, and Take with corresponding combinators (sometimes only usable when the underlying type itself implemented Read?). async_std and futures_util have corresponding implementations for most or all of the utilities in std.

So, what makes the most sense for tokio 0.2? I had a few ideas:

  • Implement AsyncRead for the existing std types where possible. Where not possible, they would be have to be implemented in a different way. tokio 0.1 did this for some types.
  • Implement a Read-to-AsyncRead adapter. futures-util calls this AllowStdIo. If this option is chosen, perhaps we could use some kind of marker supertrait like SafeAsAsyncRead as a speed bump against using it with arbitrary Read types that might block.
  • Reimplement parallel types for all of std (this is what async_std does).
  • Reexport or wrap types from another crate such as futures-util.
    • I think this is currently the most direct option for users of tokio-io as of 0.2.0-alpha.6, via futures_tokio_compat.

On another note, I would love to see an AsyncSeek trait in tokio and implemented for (at least) Cursor and File for feature parity with std.

@hawkw
Copy link
Member

hawkw commented Oct 4, 2019

  • Implement AsyncRead for the existing std types where possible. Where not possible, they would be have to be implemented in a different way. tokio 0.1 did this for some types.

@jebrosen I'm not convinced it's good idea to implement AsyncRead/AsyncWrite for types that also implement Read/Write. There are several methods in Async{Read, Write}Ext whose names and argument types overlap with Read or Write methods, so it's possible that users might find themselves in situations where they have to explicitly state which trait is being used. It would be a shame to have to write stuff like

AsyncWriteExt::write_all(&mut my_writer, buf)

instead of

my_writer.write_all(buf)

@jebrosen
Copy link
Contributor

jebrosen commented Oct 7, 2019

In writing my previous comment I had actually missed a huge amount of functionality that was already implemented, and #1632 adds the few utilitites I was especially interested in.

I believe these are all that's missing now compared to std:

@hawkw
Copy link
Member

hawkw commented Oct 8, 2019

#1642 adds AsyncBufRead::split.

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

5 participants