Task runner (mid-level API), part 1

[soywod]

Split version of #165, part 1.

• Added core changes
• Added the new Resolver flow that aims to enqueue task AND get resolution straight. Resolver::run_task returns a new Flow called TaskResolver that can feed Stream::progress.
• Made sure Scheduler and Resolver are Send.

[coveralls]

Pull Request Test Coverage Report for Build 9085261933

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 64.616%

---

Totals
Change from base Build 9053701501:	0.0%
Covered Lines:	893
Relevant Lines:	1382

---

💛 - Coveralls

[jakoschiko]

I don't understand why we need a Resolver that wraps the Scheduler.

Suggestion: Let's add this method to Scheduler:

impl Scheduler {
    pub fn run_task<'a, T: Task>(&'a mut self, task: T) -> TaskRunner<'a, T> {
        let task_handle = self.enqueue_task(task);
        TaskRunner { scheduler: self, task_handle }
    }
}

TaskRunner looks like this:

pub struct TaskRunner<'a, T: Task> {
    scheduler: &'a mut Scheduler,
    task_handle: TaskHandle<T>,
}

impl<'a, T: Task> Flow for TaskRunner<'a, T> {
    type Event = T::Output;
    type Error = SchedulerError;

    fn enqueue_input(&mut self, bytes: &[u8]) {
        self.scheduler.enqueue_input(bytes);
    }

    fn progress(&mut self) -> Result<Self::Event, FlowInterrupt<Self::Error>> {
        todo!()
    }
}

Then we can use it like this:

async fn main() {
    let stream = TcpStream::connect("127.0.0.1:12345").await.unwrap();
    let mut stream = Stream::insecure(stream);

    let client = ClientFlow::new(ClientFlowOptions::default());
    let mut scheduler = Scheduler::new(client);

    let capabilities = stream
        .progress(scheduler.run_task(CapabilityTask::default()))
        .await
        .unwrap();

    println!("capabilities: {capabilities:?}");

    stream
        .progress(scheduler.run_task(AuthenticateTask::plain("alice", "pa²²w0rd")))
        .await
        .unwrap();

    stream
        .progress(scheduler.run_task(LogoutTask::default()))
        .await
        .unwrap();
}

If we want to run tasks in parallel, we could even provide something like this:

let (foo, bar) = stream
    .progress(scheduler.run_task_2(FooTask::default(), BarTask::default()))
    .await
    .unwrap();

And we could play a little bit with the API to make it more readable:

let foo = stream
    .progress(FooTask::default().run(scheduler)) // Swap order to reduce nesting
    .await
    .unwrap();

Pros:

• No additional Resolver type
• Instead of duplicating the API and adding many functions to Resolver, we can reuse the existing Task constructors

What do you think?

[soywod]

I initially tried sth similar, but the compiler was not happy at all about the lifetime introduction. Let me try again.

[soywod]

Awesome, looks like it works well this time. I did not think about creating a new struct from the scheduler itself that returns self, somehow the compiler is happy. Thank you, I prefer this version more!

[soywod]

(Regarding the CI, I wait for #175 to be merged to fix it)

EDIT: done!

[jakoschiko]

I'm on vacation for a couple of days. I'll provide some feedback next week.

[duesee]

Hey Clement, some preliminary comments :-)

[jakoschiko]

Hello! Thanks for progressing this PR. We were just discussing it and stumbled upon a design issue that is created by having enqueue_task and run_task on the Scheduler. Let's look at this example:

let handle = scheduler.enqueue_task(AuthenticateTask::plain("alice", "pa²²w0rd", true));

let capabilities = stream
    .progress(scheduler.run_task(CapabilityTask::new()))
    .await // This will silently ignore the result of the enqueued AuthenticateTask
    .unwrap()
    .unwrap();

Having both methods on the same type might lead to accidental misusage. Maybe you were right about introducing a dedicated type called Resolver (or any other name).

• Scheduler is more low-level and provides only enqueue_task. Its usage is complicated but rather flexible.
• Resolver is more high-level and provides only run_task. Its usage is easy but rather restrictive.

However, these are long-term thoughts. If you are happy with the current state then let's keep it for now and improve the design later. I'm sure I'll change my mind about this a couple of times :p

[duesee]

Awesome, thanks! As soon as the remaining discussions are fixed we should quickly merge :-) I'm already accepting, thanks for the perseverence!

[soywod]

Having both methods on the same type might lead to accidental misusage. Maybe you were right about introducing a dedicated type called Resolver (or any other name).

Indeed, I makes sense to separate them to avoid misuage. I initiated a new module resolver, let me know what you think about it.

Ready for another review!

[jakoschiko]

Thanks again! I added some more comments, but only minor issues, you can choose whether you want to fix them.

If you are ready, we can merge it.

[soywod]

Thanks again! I added some more comments, but only minor issues, you can choose whether you want to fix them.

Fixes pushed, waiting for the CI. If no more comment then it can be merged straight! I will open another PR with all other tasks.

Thank you 🙂