single_threaded_task_pool scope api doesn't work in wasm

[mockersf]

Bevy version

0.5 and d119c1ce14da59089a65373d59715a41d05251ad

Operating system & version

Firefox & Chromium

What you did

Trying to use the task pool to execute several tasks, I hoped to have the same code running in both wasm and native

use bevy::{prelude::*, tasks::prelude::*};

fn main() {
    let mut builder = App::build();
    builder.add_plugins(DefaultPlugins);

    let asset_io = bevy::asset::create_platform_default_asset_io(&mut builder);

    builder
        .insert_resource(asset_io)
        .init_resource::<Vec<String>>()
        .add_startup_system(get_from_future.system())
        .add_system(log_stuff.system())
        .run();
}

fn get_from_future(
    task_pool: Res<ComputeTaskPool>,
    mut expensive_to_get: ResMut<Vec<String>>,
    asset_io: Res<Box<dyn bevy::asset::AssetIo>>,
) {
    let asset_io = &*asset_io;
    *expensive_to_get = task_pool
        .scope(|scope| {
            for i in 0..3 {
                scope.spawn(async move {
                    std::str::from_utf8(
                        &asset_io
                            .load_path(&std::path::Path::new(&format!("file_{}.big", i)))
                            .await
                            .unwrap(),
                    )
                    .unwrap()
                    .to_string()
                });
            }
        })
        .into_iter()
        .collect();
}

fn log_stuff(expensive_to_get: Res<Vec<String>>) {
    bevy::log::warn!("got {:?}", *expensive_to_get);
}

This works on native!

Building this for wasm32 gives an error:

26 |                 scope.spawn(async move {
   |                       ^^^^^ future created by async block is not `Send`

Indeed, spawn requires the future to be Send in the single_threaded_task_pool. This could probably be removed anyway as it calls spawn_local which doesn't need Send

bevy/crates/bevy_tasks/src/single_threaded_task_pool.rs

Lines 129 to 131 in d119c1c

	pub fn spawn<Fut: Future<Output = T> + 'scope + Send>(&mut self, f: Fut) {
	self.spawn_local(f);
	}

Let's try to run this with spawn_local to check if it would work better.At least it compiles that way!

And crash in the browser

test.js:276 panicked at 'assertion failed: `(left == right)`
  left: `true`,
 right: `false`: cannot recursively acquire mutex',

And rightly so. In the scope function:

bevy/crates/bevy_tasks/src/single_threaded_task_pool.rs

Lines 79 to 83 in d119c1c

	scope
	.results
	.iter()
	.map(|result| result.lock().unwrap().take().unwrap())
	.collect()

The lock() is on a mutex, and that can't work because you can't block in wasm...

Using the task pool in wasm will work for trivial task that aren't long enough to lock the mutex, but will fail for any real case...

I tried a few things but couldn't get it to work

[TheRawMeatball]

I'm not sure if the mutex is the problematic part here - because of the code surrounding code, these mutexes shouldn't be contested. The only other place these mutexes could be locked is the task they correspond to, and the above loop should ensure all tasks have already been completed. The locking here should never block.

[mockersf]

they are locked to write the result here:

bevy/crates/bevy_tasks/src/single_threaded_task_pool.rs

Line 137 in d119c1c

result.lock().unwrap().replace(f.await);

and locked to read the result here:

bevy/crates/bevy_tasks/src/single_threaded_task_pool.rs

Line 82 in d119c1c

.map(|result| result.lock().unwrap().take().unwrap())

[bjorn3]

result.lock().unwrap().replace(f.await); should probably be changed to let res = f.await; result.lock().unwrap().replace(res);.

[mockersf]

result.lock().unwrap().replace(f.await); should probably be changed to let res = f.await; result.lock().unwrap().replace(res);.

just tried that, doesn't work 😞

[Error] panicked at 'called `Option::unwrap()` on a `None` value', bevy_tasks/src/single_threaded_task_pool.rs:82:57

Stack:

http://127.0.0.1:4000/target/wasm.js:829:28
wasm-stub@[wasm code]
<?>.wasm-function[1838]@[wasm code]
<?>.wasm-function[12952]@[wasm code]
<?>.wasm-function[3528]@[wasm code]
<?>.wasm-function[5184]@[wasm code]
<?>.wasm-function[9360]@[wasm code]
<?>.wasm-function[8971]@[wasm code]
<?>.wasm-function[9287]@[wasm code]
<?>.wasm-function[8483]@[wasm code]
<?>.wasm-function[199]@[wasm code]
<?>.wasm-function[2871]@[wasm code]
<?>.wasm-function[222]@[wasm code]
<?>.wasm-function[2471]@[wasm code]
<?>.wasm-function[9798]@[wasm code]
wasm-stub@[wasm code]
_dyn_core__ops__function__FnMut__A____Output___R_as_wasm_bindgen__closure__WasmClosure___describe__invoke__h4b7b1b37763fbaeb@[native code]
__wbg_adapter_51@http://127.0.0.1:4000/target/wasm.js:250:134
real@http://127.0.0.1:4000/target/wasm.js:199:21
promiseReactionJob@[native code]


	(anonymous function) (wasm.js:841)
	wasm-stub
	<?>.wasm-function[1838]
	<?>.wasm-function[12952]
	<?>.wasm-function[3528]
	<?>.wasm-function[5184]
	<?>.wasm-function[9360]
	<?>.wasm-function[8971]
	<?>.wasm-function[9287]
	<?>.wasm-function[8483]
	<?>.wasm-function[199]
	<?>.wasm-function[2871]
	<?>.wasm-function[222]
	<?>.wasm-function[2471]
	<?>.wasm-function[9798]
	wasm-stub
	_dyn_core__ops__function__FnMut__A____Output___R_as_wasm_bindgen__closure__WasmClosure___describe__invoke__h4b7b1b37763fbaeb
	__wbg_adapter_51 (wasm.js:250:135)
	real (wasm.js:199)
	promiseReactionJob

[TheRawMeatball]

they are locked to write the result here:

bevy/crates/bevy_tasks/src/single_threaded_task_pool.rs

Line 137 in d119c1c

result.lock().unwrap().replace(f.await);

and locked to read the result here:

bevy/crates/bevy_tasks/src/single_threaded_task_pool.rs

Line 82 in d119c1c

.map(|result| result.lock().unwrap().take().unwrap())

Yes, but shouldn't the while executor.try_tick() {} loop mean all tasks have already been finished and there are no outstanding locks on anything?

[mockersf]

no, it just poll each future once: https://docs.rs/async-executor/1.4.1/async_executor/struct.LocalExecutor.html#method.try_tick

[TheRawMeatball]

But it's executed in a while loop, and try_tick only returns false if there are no tasks left to run: https://docs.rs/async-executor/1.4.1/src/async_executor/lib.rs.html#172

[TheRawMeatball]

And, the unwrap that panics in your last code is the second unwrap:

[Error] panicked at 'called Option::unwrap() on a None value', bevy_tasks/src/single_threaded_task_pool.rs:82:57

[mockersf]

But it's executed in a while loop, and try_tick only returns false if there are no tasks left to run: https://docs.rs/async-executor/1.4.1/src/async_executor/lib.rs.html#172

it pops the tasks from the queue, poll it once, and go to the next. it's not waiting for tasks to be finished or putting them back in the queue

[mockersf]

And, the unwrap that panics in your last code is the second unwrap:

Yup that's with the change suggested by @bjorn3 and a mystery to me... unless await works a little different in wasm which is entirely possible as you can't block in wasm

[TheRawMeatball]

await works a little different in wasm

await is a language-level construct that will behave identically regardless of the target platform.

it pops the tasks from the queue, poll it once, and go to the next. it's not waiting for tasks to be finished or putting them back in the queue

Alright, with that context I think I solved the reason why this bug exists: The code, as evident by the comment relies on that loop finishing all tasks. However, this isn't the case if the tasks contain an await that won't resolve immediately. In the original code, this left the mutex locked which errored because you can't block in wasm (this would've deadlocked in any other platform). In the new code, the mutex isn't locked but the option isn't populated, which also causes an error resulting in the second bug.

[TheRawMeatball]

And the code in the original bug report fundamentally never would've ran on the web, as it requires you block on the main thread until the IO operation necessary to finish loading is complete. This does run on native because native uses futures::block_on which properly blocks until IO is complete and file_io is actually sync, but this is something you shouldn't do regardless of the target platform.

[mockersf]

And the code in the original bug report fundamentally never would've ran on the web

Exactly my point, we can't use the single threaded task pool on the web where it's the only place it's used 👍

[TheRawMeatball]

The single-threaded task pool (imo) is a temporary solution until threaded wasm matures, and it is used - pretty heavily in fact. The SystemStage executor uses TaskPool::scope as a thread pool, and this use is still valid even on the web, as it removes the need for an entirely separate implementation. And my main point was this code wasn't ideal on any platform, as this is effectively blocking new frames and system execution on IO - perhaps the solution is to make Scope::spawn take a function instead? This might make more sense given this shouldn't be used with long-running features.

EDIT: The await system does seem to be used by the system executor to manage waking up available systems, so maybe not.

However, the point that awaiting anything that isn't guaranteed to complete in the scope is a very bad idea stands, so the fact that TaskPool::scope is effectively futures::block_on definitely should be better documented.