refactor: add WaitGroup for waiting for a dynamic set of tasks

[katrinafyi]

This is a small step towards recursion. It will enable waiting for a dynamic number of tasks which can spawn other tasks, which allows for correct and deadlock-free termination in the presence of (eventual) recursion.

I think recursion is not an astoundingly difficult feature but, because lychee is async and stream, it needs the combination of a few parts which might be tricky to get right. This is one of those parts. In fact, I had two different deadlock bugs while working on this PR before reaching the final implementation. If this was all done in one big recursion PR, then I think these bugs would be a lot harder to find and fix.

The old implementation in #1603 used Arc<AtomicUsize> to implement this requirement. This new PR uses a RAII-based guard value to represent tasks, which should make it much easier to write correct code - it's all handled by the borrow checker and Drop, so it's impossible to forget to adjust the counter.

In terms of testing, changes the check.rs command to rely on WaitGroup for termination. You can read about this in the comments. This means that every run of lychee goes through the WaitGroup code and acts as a test for WaitGroup. There is a risk that if it is buggy, the impacts would be big. However, I think the unit tests are big enough that they're a decent exercise of the WaitGroup. My second deadlock bug was found by make test.

You can also see the docs at https://waitgroup.lychee-docs-katrinafyi.pages.dev/lychee_lib/waiter/

Related work:

• https://docs.rs/waitgroup/latest/waitgroup/ - seems to work
• https://docs.rs/atomic-waitgroup/latest/atomic_waitgroup/index.html - doesn't support recursive.
• https://docs.rs/wgp/latest/wgp/ - seems to work, but uses unsafe and has had correctness bug.
• https://docs.rs/crossbeam-utils/latest/crossbeam_utils/sync/struct.WaitGroup.html, https://docs.rs/omango/latest/omango/wg/struct.WaitGroup.html but this is for threading rather than async.
• lots of things for waiting on a set of futures, but we can't use that here because we don't have individual features for each task https://docs.rs/tokio-util/latest/tokio_util/task/task_tracker/struct.TaskTracker.html, https://docs.rs/tokio/1.49.0/tokio/task/join_set/struct.JoinSet.html#method.join_all, https://docs.rs/futures/latest/futures/stream/struct.FuturesUnordered.html, https://docs.rs/spawn_groups/latest/spawn_groups/struct.SpawnGroup.html

[mre]

Lgtm.

@thomas-zahner, any thoughts before we merge this?

[thomas-zahner]

Very cool PR! I don't see any problems with it.

@katrinafyi Could you add a test to waiter.rs to to test if recursion would really work that way? So that the unused part (// unused for now, but will be used for recursion eventually.) is covered and tested already. This would be great documentation and help me (and others) understand how this is used in a simpler scenario without lychee's additional complexity.

[katrinafyi]

@thomas-zahner That's a really interesting idea. I've added a (hopefully) simple example which you can see here: https://github.com/lycheeverse/lychee/compare/74046a9..b27a34f

[thomas-zahner]

@katrinafyi Thank you, that's very cool! This helps my understanding quite a bit.

[thomas-zahner]

Either move into mod test or annotate with #[cfg(test)], this should resolve the #[allow(dead_code)], right?

[katrinafyi]

Hmm I put it outside of mod tests because I wanted it to be included in the docs.

I tried a few things and it's a bit tricky. We need compiling to work (1) normally, (2) in docs, and (3) in tests.

• #[cfg(any(test, doc))] on the examples: dev-dependencies is not available in doc, so tokio-stream has to be in dependencies. but then, it's flagged as an unused crate in normal compilation.
• putting inside mod test and making test module #[cfg(any(test, doc))] has the same problems

Do you have thoughts or other suggestions?

[thomas-zahner]

Same here

[thomas-zahner]

This is only depended on in the new test, right? If so, move it to the [dev-dependencies] group