Skip to content
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

future_to_promise Promise is not rejected if rust panics asynchronously #2724

Closed
mstange opened this issue Nov 20, 2021 · 5 comments
Closed
Labels

Comments

@mstange
Copy link
Contributor

mstange commented Nov 20, 2021

Describe the Bug

From the future_to_promise documentation:

If the future provided panics then the returned Promise will not resolve. Instead it will be a leaked promise. This is an unfortunate limitation of wasm currently that’s hoped to be fixed one day!

This behavior is very undesirable, so I think it would be good to have a bug on file for it.

I'm also curious about what the exact wasm limitation is, and where I can follow plans to fix it.

Steps to Reproduce

Rust:

use js_sys::Promise;
use wasm_bindgen::prelude::*;
use wasm_bindgen_futures::{future_to_promise, JsFuture};

#[wasm_bindgen]
pub fn mywasmfun() -> Promise {
    future_to_promise(mywasmfun_impl())
}

async fn mywasmfun_impl() -> Result<JsValue, JsValue> {
    panic!("Please catch this panic");
}

JS:

import init, { mywasmfun } from './pkg/wasm_bindgen_async_panic.js';

async function run() {
  await init();

  try {
    await mywasmfun();
  } catch (e) {
    console.log("Caught exception:", e);
  }
}

run();

Expected Behavior

Panics should be propagated upwards the "async stack" so that any awaits on the JS side can catch panics and don't keep waiting forever.

Actual Behavior

The panic never propagates, the await never completes.

@mstange mstange added the bug label Nov 20, 2021
@mstange
Copy link
Contributor Author

mstange commented Nov 20, 2021

So, from what I understand, the promise can only reject if something calls its reject handler. Usually this handler would be called from the Rust code. But since the Rust code has panicked, there's no one to call reject.

Might there be a way to make the JS-side of the bindings smarter, so that it can catch the panic exception and call reject without help from the Rust side?

@alexcrichton
Copy link
Contributor

Thanks for the report, but panics unfortunately basically don't work as means of communicating errors. Given the lack of exception handling in wasm handling panics is extremely difficult right now and largely not done by wasm-bindgen, leading to issues such as this. This is unlikely to change in the near future unfortuantely.

@xtuc
Copy link
Member

xtuc commented Aug 8, 2023

@alexcrichton do you see a way to do error handling with rewriting the binary? Could the unreachable instruction be replaced by a wasm-bindgen call that would end up rejecting the Promise?

xtuc added a commit to xtuc/workers-rs that referenced this issue Aug 10, 2023
Because of a limitation with wasm_bindgen (rustwasm/wasm-bindgen#2724),
a Rust panic in asynchronous code isn't able to reject the Promise it
happened in.

This change makes the main worker Promise cancellable, with a panic hook
or by rewriting the wasm binary, it's possible to reject the Promise and
handle the error.

The worker concurrency model makes rejecting the Promise not a guarantee
in cases the worker has a lot of traffic that cause a panic. See comment
in source code.

Usage (for now), add to your worker:
```rust
fn start() {
    std::panic::set_hook(Box::new(|i| {
        __workers_rs_cancel();
        worker::console_error_panic_hook::hook(i);
    }));
}
```

For example, previously:
```
POST https://example.com/ - Exception Thrown @ 8/10/2023, 12:26:59 PM
✘ [ERROR]   Error: The script will never generate a response.
```

with this change:
```
POST https://example.com/ - Exception Thrown @ 8/10/2023, 12:27:05 PM
  (error) panicked at 'oops', src/lib.rs:43:5

Stack:

Error
    at entry.js:746:17
    at logError (entry.js:304:14)
    at __wbg_new_abda76e883ba8a5f (entry.js:745:10)
    at console_error_panic_hook::hook::hb5e89e21e6f36fcb (wasm://wasm/0351507a:wasm-function[174]:0x1bca6)
    at test_coredump_worker::start_start_glue::{{closure}}::hc242a0946799f25b (wasm://wasm/0351507a:wasm-function[854]:0x2d662)
    at std::panicking::rust_panic_with_hook::h85b7f6628c291e12 (wasm://wasm/0351507a:wasm-function[269]:0x2234c)
    at std::panicking::begin_panic_handler::{{closure}}::h1e17bad04a5713a4 (wasm://wasm/0351507a:wasm-function[323]:0x24aa6)
    at std::sys_common::backtrace::__rust_end_short_backtrace::h63adae5c31458c4b (wasm://wasm/0351507a:wasm-function[875]:0x2d72e)
    at rust_begin_unwind (wasm://wasm/0351507a:wasm-function[491]:0x29a2d)
    at core::panicking::panic_fmt::hf5c4cd929d4aaa9e (wasm://wasm/0351507a:wasm-function[601]:0x2ba1d)
```
@temeddix
Copy link

@xtuc Thank you for investigating this problem. May I ask, does that commit you made fixes this issue?

@daxpedda
Copy link
Collaborator

I'm going to close this as not really actionable.
The solution is to implement the exception handling proposal in Rust, until then panic! aren't supported and are more or less unsound. See #3687.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants