search

tokio-rs / tokio-uring

An io_uring backed runtime for Rust
MIT License
1.14k stars 122 forks source link

fix: API IO bounds should use BoundedBuf #217

Closed FrankReh closed 1 year ago

FrankReh commented 1 year ago

Some of the more recently introduced API IO calls used the original IoBuf and IoBufMut traits without realizing the more general trait is BoundedBuf and BoundedBufMut.

Interesting that callers to the library might not notice until they try to pass a slice of a buffer to one of the IO calls.

FrankReh commented 1 year ago

Also interesting that the reviewer(s) didn't catch the misuse of the trait either. (Speaking for a friend.)

FrankReh commented 1 year ago

Asking @mzabaluev and/or @Noah-Kennedy to review. I believe the fixes are straight forward, almost cosmetic.

I didn't change the deref and deref_mut signatures in src/buf/mod.rs. Please confirm that you think those shouldn't be changed either.

oliverbunting commented 1 year ago

@FrankReh you might consider changing the fixed buffer registries that just got generalised too

FrankReh commented 1 year ago

you might consider changing the fixed buffer registries that just got generalised too

True that. I wanted to get this in while it was current and I do remember seeing those and wondering but then I forgot. Thanks for the reminder.

FrankReh commented 1 year ago

@oliverbunting I'm thinking it's good that the fixed buffer registries are generic over IoBuf.

It is by design that an IoBuf can be passed to an IO operation that takes a BoundedBuf. See Bounded for the implementation. It's also true that a Slice can be passed to the same IO operations.

I guess one way to think of it is the IoBuf and the Slice are specific and the BoundedBuf is general. Maybe there are more precise terms to use. I know we don't have documentation on the main page about this and our design doc is now very misleading.

mzabaluev commented 1 year ago

I didn't see a real need for this change, but if you can pass Vec<Slice<B>> to the vectored operations where you could use Vec, and it does not add any overhead to the original use, why not?

Maybe BoundedBuf could be later changed to a more fundamental abstraction provided by async-io-traits.