Skip to content

Async::Task#wait_all #89

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

Closed
wants to merge 2 commits into from
Closed

Async::Task#wait_all #89

wants to merge 2 commits into from

Conversation

bryanp
Copy link

@bryanp bryanp commented Jan 11, 2021

Provides a way to wait on all children, including children and their tasks. This came out of a conversation I had with @ioquatix last week and is the approach I ended up moving forward with for my own use-case.

Types of Changes

  • Bug fix.
  • New feature.
  • Performance improvement.

Testing

  • I added new tests for my changes.
  • I ran all the tests locally.

Waits on all children to complete, including subtasks. Returns the result of the toplevel task.
@chuckremes
Copy link

I'd like to see this feature added too. Useful.

@ioquatix
Copy link
Member

ioquatix commented Jan 17, 2022

It's useful but it's also risky because some interfaces will create tasks for persistent connections that will outlive your own tasks and if you try to wait on them you will be waiting forever. We could try to introduced wait_all for all non-transient tasks perhaps, but it still has risks.

@bryanp
Copy link
Author

bryanp commented Jan 17, 2022

Coming back to this, I think it would help for async to define what kind of library it is. Right now the decision to not include this feature feels arbitrary. There are lots of ways for users to trip over themselves—why exclude this particular one? IMO defining the types of problems the library solves vs what the user is left to solve would help inform these discussions.

@trevorturk
Copy link
Contributor

trevorturk commented Jan 21, 2022

I've been thinking about this (shower thoughts) and I don't know if this is helpful, but I've been thinking of Async::Barrier as less of a "barrier" and more of a "bucket" and I wonder if that mindset is valid, and, if so, might be helpful when considering this. When I'm using a barrier, I'm thinking "let's toss this in that bucket, and this over here, and maybe this, too" and then calling wait I believe essentially means wait_all within that "bucket" context.

bucket = Async::Barrier.new
barrier.async { some_expensive_work }
some_other_method
something_over_here
do_this_in_the_bucket_too(bucket)
def do_this_in_the_bucket_too(bucket) = bucket.async { this_too }
bucket.wait

I'm still trying to wrap my head around all of this async and/or concurrency stuff (I'm also learning Swift and SwiftUI now) but I wonder if this kind of framing is helpful at all -- tl;dr calling Async::Barrier.wait is essentially wait_all for that group of tasks.

I was also wondering if it would be useful to be able to "tag" async tasks and then wait_all_for_tag(tag) but I'm not sure if that's any different from the barrier concept, really...

Anyway, just some food for thought on this topic! I hope I'm not too far off base! :)

@ioquatix
Copy link
Member

ioquatix commented Jan 25, 2022

@trevorturk that's pretty accurate.

Right now the decision to not include this feature feels arbitrary

@bryanp it's not arbitrary, it's because the risk of wait_all stalling indefinitely/causing a deadlock is quite high AND might not be obvious in testing vs in production.

Suppose you have an Async::HTTP::Internet instance. A hypothetical wait_all implementation might end up waiting on an internal task created by HTTP/2 for persistent connections. It's better to use Async::Barrier and be explicit about things, or wait on a parent task which explicitly waits on its children (or not) and so on. This ensures your don't wait on hidden tasks.

We could mitigate this by only waiting on non-transient tasks, but this only covers some use cases. Although I'd be happier about it since transient tasks are designed explicitly around life-cycles that go beyond the current task, which effectively means they should be invisible to wait_all.

That being said, even if wait_all existed, I would never recommend people use it over an explicit barrier, which is why I hesitate to introduce it.

@ioquatix
Copy link
Member

ioquatix commented May 4, 2022

Thanks everyone for the discussion. For the reasons given above, I'm going to hold off on this PR for now.

@ioquatix ioquatix closed this May 4, 2022
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

Successfully merging this pull request may close these issues.

4 participants