SequencedTaskRunnerHandle / mojo from one off tasks in TaskScheduler?

[Gabriel Charette]

Hello all, Ilya hit an interesting use case @ https://codereview.chromium.org/2955063005/#msg13.

If a one off task uses Mojo, it will crash trying to SequencedTaskRunnerHandle::Get(). This is error prone and could result in errors slipping in production say the mojo codepath isn't hit by tests (and hard to diagnose even when caught as happened on that CL).

How about one off tasks provide a SequencedTaskRunnerHandle? This would allow parallel one off tasks to be extended into a sequence from self (which the current task is the first of)? 

Sounds pretty simple to implement given this is already how the scheduler impl works (a parallel task is queued as a task_scheduler::internal::Sequence with a single task in it.

I don't really see downsides?

[Robert Liao]

I currently can't think of any downsides, and it works nicely into our existing architecture where these are already sequences.

[François Doray]

I think it's a good idea.

[Robert Liao]

Let's do it.

The only strangeness that I can think of is that the task runner you posted to is not going to equal the task runner you get from the handle, but that's okay right?

[Gabriel Charette]

Perfect, will do.

Le ven. 30 juin 2017 15:20, Robert Liao <rob...@chromium.org> a écrit :

Let's do it.

The only strangeness that I can think of is that the task runner you posted to is not going to equal the task runner you get from the handle, but that's okay right?

I think so, we already intend to eventually move TaskRunner::RunsTasksInCurrentSequence() to SequencedTaskRunner. On parallel tasks it merely validates execution contexts which is better done through AssertFooAllowed(). Besides that can't think of anything that would fail a TaskRunner switch (plus most parallel tasks come straight from base::PostTaskWithTraits() not even from a TaskRunner).

[Marijn Kruisselbrink]

It doesn't appear that this actually happened? Although of course it's not too hard to write CreateSequencedTaskRunnerWithTraits({...}).PostTask(...) instead of PostTaskWithTraits({...}, ...), so it's not like having a magic sequence available for one-off posted tasks makes the code that much simpler.

--
You received this message because you are subscribed to the Google Groups "scheduler-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scheduler-dev+unsubscribe@chromium.org.
To post to this group, send email to schedu...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAJTZ7L%2BF-%2BJLvkWZWVUx9U8yWvHMa3k-AFpAsh0h1Hairy7YeA%40mail.gmail.com.

[Gabriel Charette]

Oops indeed, still on my todo list, should be able to get to it this week. You can indeed just CreateSequenced...()->PostTask() inline right now but I'd like to provide a smoother API long term.

To unsubscribe from this group and stop receiving emails from it, send an email to scheduler-de...@chromium.org.

To post to this group, send email to schedu...@chromium.org.

[Gabriel Charette]

Actually, hmmmm, just looked into this and while we do have everything we need to implement this, it feels rather unfortunate to instantiate a new SequencedTaskRunner bound to the existing base::internal::Sequence (which initially has a single parallel tasks in it) just to bind it to SequencedTaskRunnerHandle for the odd case where it's needed (and having a hook for that API to probe on demand is a mess as we saw with the SequencedWorkerPool impl we're trying to hook out of these).

The failure mode for posting a parallel tasks that ends up needing SequencedTaskRunnerHandle::Get() isn't tricky (DCHECK or worst case nullptr crash on Canary if code isn't tested).

And since in this whole migration this is the only instance of this I heard : I'm leaning towards not providing this facilitation and forcing callers to use an inline base::CreateSequenced...()->PostTask(...);

Thanks Marijn

[Gabriel Charette]

What is "this"? There have indeed been a lot of changes to scheduling since 2017...

On Wed, Sep 13, 2023 at 11:09 AM Raul Sanchez <ra...@timedoctor.com> wrote:

Was this finally implemented?
Because I am seeing crash which looks like the one described here

Cheers