[Merged by Bors] - Stageless: close the finish channel so executor doesn't deadlock

[hymm]

Objective

• Fix panic_when_hierachy_cycle test hanging
• The problem is that the scope only awaits one task at a time in get_results. In stageless this task is the multithreaded executor. That tasks hangs when a system panics and cannot make anymore progress. This wasn't a problem before because the executor was spawned after all the system tasks had been spawned. But in stageless the executor is spawned before all the system tasks are spawned.

Solution

• We can catch unwind on each system and close the finish channel if one panics. This then causes the receiver end of the finish channel to panic too.
• this might have a small perf impact, but when running many_foxes it seems to be within the noise. So less than 40us.

Other possible solutions

• It might be possible to fairly poll all the tasks in get_results in the scope. If we could do that then the scope could panic whenever one of tasks panics. It would require a data structure that we could both poll the futures through a shared ref and also push to it. I tried FuturesUnordered, but it requires an exclusive ref to poll it.
• The catch unwind could be moved onto when we create the tasks for scope instead. We would then need something like a oneshot async channel to inform get_results if a task panics.

[bors]

try

Build failed:

• run-examples-on-wasm

[cart]

I'm pretty sure I'm on board for these changes. I'm going to briefly consider the alternatives, as the extra catch_unwinds do make me a bit uneasy in terms of perf (according to rust-lang/rust#34727 LTO might benefit this case). Adding additional per-system work is generally something I'm worried about. But given what you saw in many_foxes this seems stomach-able.

[hymm]

as the extra catch_unwinds do make me a bit uneasy in terms of perf

Yeah definitely a concern.

There might be a way to write a custom future for get_results where you poll the concurrent queue for new tasks which you push to a FuturesUnordered (or https://crates.io/crates/unicycle) and then poll the FuturesUnordered. Then loop until done. I haven't had a change to try it yet, so I have no idea if it would be more performant or not.

[cart]

One alternative that wasn't listed that wouldn't affect (per system) perf: we could add a scope.spawn_last() function that calls await on those tasks only after normal tasks have finished. It would mean adding another ConcurrentQueue to Scope which would increase the size of Scope, adds another bleh transmute into the equation, and would allocate unless we use ConcurrentQueue::bounded(1)

[cart]

I'm clearly missing something because this doesn't work as I expected when I implement spawn_last with a ConcurrentQueue and use scope.spawn_last(executor):

let get_results = async {
    let mut results =
        Vec::with_capacity(spawned_ref.len() + spawned_last_ref.len());
    let mut index = 0;
    let mut task_failed = false;
    while let Ok(task) = spawned_ref.pop() {
        if task_failed {
            task.cancel().await;
        } else {
            if let Some(result) = task.await {
                unsafe { *results.get_unchecked_mut(index) = result };
            } else {
                task_failed = true;
            }
        }
        index += 1;
    }
    // this detects _no_ failed tasks when a system panics. index is 0
    // prints "false 0"
    println!("{task_failed} {index}");

    while let Ok(task) = spawned_last_ref.pop() {
        if task_failed {
            task.cancel().await;
        } else {
            if let Some(result) = task.await {
                unsafe { *results.get_unchecked_mut(index) = result };
            } else {
                task_failed = true;
            }
        }
        index += 1;
    }

    // this does not run because we hit the deadlock above for the executor task
    println!("{task_failed} {index}");
    
    if task_failed {
        panic!("At least one task failed");
    }
    unsafe { results.set_len(index) };
    results
};

[cart]

Apparently this scope isn't registering the system task at all?

[cart]

For the curious: https://github.com/cart/bevy/tree/spawn_last

[cart]

I think I answered my own question on discord:

[cart]

I don't think spawn_last is an option here.

[hymm]

If you're using the stage less branch, this looks very wrong as the first time you enter get_results the only task might be the executor task. When that task runs it then spawns the system tasks. So get_results needs to be able to handle more tasks spawning while it runs.

[cart]

(Rebased because github was very confused + still including #7444 changes in the PR diff. I made no code changes)

[cart]

I'm thinking this is our best option for now. I'm guessing any sort of "fair polling" based approach will likely incur more overhead than the per-system panic handling code (which can still run in parallel).

[cart]

And "fair polling" would need to accommodate an ever-changing list of tasks as each task can produce more tasks.

[james7132]

LGTM. It's a shame we need to catch_unwind on multiple levels like this, but I don't see a better solution here.

[cart]

I think we should revisit the multithreaded schedule executor strategy to avoid nested spawns on scope, which shoouulld remove the need for this. But for now I think this is our best bet.
bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• build-without-default-features (bevy)
• build-without-default-features (bevy_ecs)
• build-without-default-features (bevy_reflect)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• msrv
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12