tokio-rs / tokio-uring

An io_uring backed runtime for Rust
MIT License
1.11k stars 117 forks source link

Add UnsubmittedRead and Link API #294

Open ileixe opened 7 months ago

ileixe commented 7 months ago

This PR introduces two API changes:

  1. Add UnsubmittedRead in read part,
  2. Introduce Submit and Link struct.

(1) is basically following suggested API change in: https://github.com/tokio-rs/tokio-uring/pull/244 and (2) is for linked operations.

User now can link their operations like below

let read1 = file
    .read_at(buf1, 0);
let read2 = file.read_at(buf2, HELLO.len() as u64).;
let write = file.write(HELLO);

let (res1, read2_future) = read1.link(read2).link(write).submit().await;
let (res2, write_future) = read2_future.await;
let res3 = write_future.await;

Closes: https://github.com/tokio-rs/tokio-uring/issues/289

ollie-etl commented 7 months ago

THere are some API issue to address here.

Opening up the flags to the user is presents the user with the ability to break the tokio-uring lifecycle managment. Setting IOSQE_CQE_SKIP_SUCCESS is catastrophic.

The link API is also flawed. If a IOSQE_IO_LINK is observed in the final SQE at submission, it is ignored. The current API doesn't protect the user from that, or provide any way of knowing if the ring has been submitted in between the ops

Finally, what stops the user experiencing concurrency errors, if await is called between read1.submit and read2.submit? Another task could submit to the ring.

ileixe commented 7 months ago

@ollie-etl Thanks for the review, you're absolutely right for possible edge cases. I myself tried to provide additional API while hiding the set_flags() first but I found it's a bit blurry to me which is the usable API. So here I'm compromising to put some constraints to users while providing the very least primitives which enables their want including us first. (e.g. No yield between links submitting "valid" sequence of operations)

If possible (and viable), I'd like to discuss some of API options in this PR so complete this functionality in here.

Let me leave some approches to discuss that direction in advance, I added link!() and hardlink!() macros before and it looks like,

let read1 = file.read();
let write1 = file.write();

let (res1, res2) = link!(read1, write1);

This will expand as you can easily expect

let read1 = read1.submit().set_flags(Flags::IO_LINK);
let write1 = write1.submit();

(read1.await, write1.await)

With those API, we can hide uring primitive to users and prevent the mistakes.

But I felt it's not very flexible and restricted because user cannot use futures directly. (e.g. what if I want to await only last one?, what if I just want to use existed API like join_all?). To claim value of such API, I felt I should provide wider variants of APIs (e.g. link!() may accept iterables)

(For other directions, I failed all the non-macro approaches as link!() requires basically different output types with different length of inputs.)

Any suggestion would be appreicated.

ollie-etl commented 7 months ago

My thinking on this is that Link becomes a struct, and that one of the Op traits (maybe OneshotOutputTransform ) gains a link method. This would consume the unsubmitted Op, and return a link Op. This would expose the submit method, which would either succeed in performing an atomic submission, or fail. It would also be responsible for setting the link flags.

I guess there is a question around Futures behaviour. Should the Link struct return a single future on submission? There are options here - do you only return once the chain completes, or do you provide futres for all the ops in the link chain? Both are varid, so we probably want to support both.

ileixe commented 7 months ago

I also tried to introduce somehow nested struct Link (I saw your previous PR which is the only one that I can find for the reference to implement.)

But the problem about Link implementation that I encountered is that it's hard to imagine what the future which Link op submitted looks like. The future should return different types of outputs and I felt it's almost impossible in Rust. (Here, I assume the future should return every results)

Still, I want to try the direction as it's far cleaner API so leave here my trials briefly hoping better idea.

struct UnsubmittedOneshot<D> {
     fn link<D2>(self, other: UnsubmittedOneShot<D2>) -> Link<D, D2> {
        // returns Link struct which have two different OneShot(s). It's gonna happen recursively.
     }
}

struct Link<D1, D2> {
    // Somehow awesome nested struct which have all operations.

    fn submit(&self) -> MultiInflightOneshot<D1, D2> {
        // Return new InflightOneshot which have several OPs.
    }
}

impl Future for MultiInflightOneshot<D1, D2> {
    type Output = ??? # This is dynamically decided (D1, D2, D3, D4... etc)
}
ollie-etl commented 7 months ago
struct UnsubmittedOneshot<D> {
     fn link<D2>(self, other: UnsubmittedOneShot<D2>) -> Link<D, D2> {
        // returns Link struct which have two different OneShot(s). It's gonna happen recursively.
     }
}

struct Link<D1, D2> {
    // Somehow awesome nested struct which have all operations.

    fn submit(&self) -> InflightOneshot<Link<D1,D2> {
        // Use the existing Inflight oneshot
    }
}

impl Future for InflightOneshot<Link<D1, D2>> {
    // Link Future returns a tuple of the output of the first operation, and a future for subsequent linked ops
    type Output = (<D1 as Future>::Output, D2)
}
ileixe commented 7 months ago
let links = file.read().chain(file.write());

let (read_output, write_future) = links.await;
let (write_output, next_future) = write_future.await;

You are suggesting such API above, right? It may be possible but not sure with the current API (InflightOnshot has unbounded generic D, so I cannot implement Future again and we need to submit all the operations first to make valid links, so here we still need to know all the types before).

Let me try with some more modification.

ollie-etl commented 7 months ago

@ileixe yes, thats what I had in mind, although I haven't fully thought through all the implications / issues

ileixe commented 7 months ago

@ollie-etl I added Link struct and introduce new API: Submit(). Can you take a look whether it's viable?

I don't like introducing Submit() trait as it makes breaking change, but could not find a clean way integrating with current traits.

ollie-etl commented 7 months ago

@ileixe I'd like the opinion of @Noah-Kennedy, as this is building on a lot of his work