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

write_all #110

Closed
FrankReh opened this issue Sep 12, 2022 · 8 comments
Closed

write_all #110

FrankReh opened this issue Sep 12, 2022 · 8 comments

Comments

@FrankReh
Copy link
Collaborator

I had wanted a write_all for tokio_uring TcpStream. Locally I added it as a method but would you prefer a public trait like the AsyncWriteExt trait in tokio? I'm confused by the tokio std::io::Write trait also have a write_all method. So maybe a tokio_uring::io::Write trait is actually what is called for?

Or as a POC, is it acceptable to just add the write_all directly to the TcpStream and UnixStream?

@Noah-Kennedy
Copy link
Contributor

@FrankReh I agree completely on a Write trait, as well as a Read trait. I think these should be called OwnedWrite and OwnedRead.

@Noah-Kennedy
Copy link
Contributor

Currently working on a POC for this.

@FrankReh
Copy link
Collaborator Author

FrankReh commented Sep 12, 2022

Okay. I'll keep my write_all to myself for now, but it means my other work I wanted to commit as PRs is blocked because the test case(s) use the write_all. Your Owned{Write, Read} aren't blocked by my work so I'll wait for those traits to land.

BTW my write_all is a trivial modification to the write_all in std so you may be providing it yourself in OwnedWrite. No prob. In fact, I had to decide between basing the loop on the IoBuf or Slice type and I choose Slice for no real good reason. I was going to ask your opinion anyway.

@Noah-Kennedy
Copy link
Contributor

No, go ahead and open up a PR. I'm doing some refactoring right now in my PR and I don't want to block anyone with this.

@FrankReh
Copy link
Collaborator Author

@Noah-Kennedy Just a reminder that ProvideBuffers will be very nice to support with this library. I don't know how that factors into your OwnedWrite and OwnedRead trait plans. Maybe they are orthogonal. Once I get the builder api for you to look at and perhaps approved, I'll see about a builder method that lets the caller either provide buffers or call out the library to allocate its own buffers. Of course ownership of the original buffer might make it hard for me to figure out how to provide both options so I may start with the library allocating the buffers, given parameters to a builder method.

The existing io-uring library supports buffers for reading and writing to files. Interesting as a first step, but using buffers to do multi recv and recvmsg is what's really interesting to me and that comes with the next kernel, 6.0, due in Oct, next month.

Maybe this deserves its own issue. Ownership and the drop method for these fixed buffers will be interesting.

FrankReh added a commit to FrankReh/tokio-uring that referenced this issue Sep 14, 2022
Provide an async write_all method for the TcpStream type.

Also a doc example and a rework of the examples/tcp_listener.rs
to act as a ping-pong server.

Fixes tokio-rs#110

Partially fixes actually. Traits are planned in the future that provide
the write_all method.
@Noah-Kennedy
Copy link
Contributor

@FrankReh I think that we may need to add some additional buffer traits that abstract over things uring ops can use as buffers for use within op builders internally.

@FrankReh
Copy link
Collaborator Author

@Noah-Kennedy I'm all ears. I've created #112 to discuss fixed buffers with a pbuf_ring feature introduced in 5.19. Maybe related to internal traits, maybe not.

@Noah-Kennedy
Copy link
Contributor

Thanks!

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

2 participants