gfx-rs / wgpu-rs

Rust bindings to wgpu native library
https://wgpu.rs
Mozilla Public License 2.0
1.69k stars 186 forks source link

Handle winit exception in web to avoid breaking async executor #923

Closed Frizi closed 3 years ago

Frizi commented 3 years ago

In web environment, winit throws an "not an error" JS exception in order to satisfy the ! return type of EventLoop::run. We encounter that inside our start function, which in turn is being run inside wasm_bindgen_futures::spawn_local. Unfortunately, spawn_local executor doesn't account for JS exception, and silently enters broken state where no more futures would ever run.

In detail, the is_spinning flag inside the executor is being set when the tasks are being polled. The flag is only reset once all tasks are polled. This flag prevents the executor from scheduling next queue processing, which is reasonable in the event of scheduling more tasks from inside already running task. The queue will be processed to the end anyway in the same JS microtask. Unfortunately because of the exception, the processing returns early and the flag is never being reset, thus preventing the queue from being processed ever again.

I've noticed this behaviour by observing that StagingBelt never reuses staging buffers in web environment. This is caused by StagingBelt::recall future never being polled.

The fix is to catch the JS exception, which is a bit tricky from inside the rust code. I did this by coercing wasm_bindgen to generate a function which can call other functions in a try block, using Function.prototype.call.call(closure). The added benefit is that we can now detect and silence the annoying winit exception. All other exceptions are being printed into the console directly, still avoiding the executor broken state.

Fixes #454

grovesNL commented 3 years ago

I think we should try to fix this upstream in winit instead of trying to workaround it at this level. There's probably a better way to exit run instead of exceptions since that was originally introduced into winit, or we could try to introduce a web-only extension to winit

Frizi commented 3 years ago

I think we should try to fix this upstream in winit instead of trying to workaround it at this level

I don't think we can, at least not without significant changes to winit architecture. Their current API requires that run never returns. JS Exception is really the only option in web environment to somehow implement that, as blocking the thread is not possible, because we are already running inside a single-threaded runtime.

They specifically tried to not introduce any per-platform API for event loop. I wouldn't count on them changing their minds.

One place where we could upstream anything is inside wasm_bindgen_futures, to make sure the executor doesn't break under those circumstances. But even then, we want to catch it at least to silence the winit exception.

kvark commented 3 years ago

It just boggles my mind to think that this code is what winit expects any users (targeting the Web) to have :/

Frizi commented 3 years ago

I would say they expect you to ignore the error. It's just unfortunate that it can also break async executors. I don't think this was a common problem before. I guess you can also interpret it as "the call blocks forever", which is kinda correct. If you run a task that never ends inside a single-threaded executor, you block the whole thing forever. Except that on web, you don't actually want all the consequences of that.

It's just winit being a leaky abstraction, pretending to own the whole event loop. But on web, it just can't.

Frizi commented 3 years ago

I just realised that the executor problem was already a known issue. So this is basically a new take on solving #454, a smaller alternative to #508.

kvark commented 3 years ago

Project moved to wgpu

grovesNL commented 3 years ago

@Frizi :+1: I created https://github.com/rust-windowing/winit/issues/1956 to discuss it in winit. If we can't fix it there then we should proceed with this fix