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

Drop::drop() not called on a panic! when compiling to WASM #58874

Closed
coolreader18 opened this issue Mar 2, 2019 · 8 comments
Closed

Drop::drop() not called on a panic! when compiling to WASM #58874

coolreader18 opened this issue Mar 2, 2019 · 8 comments
Labels
A-destructors Area: destructors (Drop, ..) O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@coolreader18
Copy link
Contributor

coolreader18 commented Mar 2, 2019

Running 1.34.0-nightly 2019-02-22, but it also happens on stable. Repr here: https://github.com/coolreader18/wasm-no-drop/.

I noticed this with an Rc<RefCell<_>>, when I borrowed it mutably and something panicked while the borrow was held, trying to borrow that same RefCell again panicked with "already borrowed: BorrowMutError". This might be a wasm-bindgen error, cause it seems weird that this bug would go unnoticed for the whole time that wasm32-unknown-unknown has been stable (unless I missed the issue).

Demo currently in production: https://rustpython.github.io/demo, add a line that contains global x, click Run, remove that line, and try to run it again. There's a version of that that shows the panic error message, and the first time it's "not implemented: global` and the second time it's "borrow error".

@ids1024
Copy link
Contributor

ids1024 commented Mar 2, 2019

When you say wasm, I presume you mean the wasm32-unknown-emscripten target?

Panic isn't guaranteed to result in destructors being called; this depends on whether panic=unwind or panic=abort is used. Normally this is configured in Cargo.toml, but some targets may support only abort, which wouldn't result in destructors being run.

That said, there does seem to be an unwinding implementation for emscripten: https://github.com/rust-lang/rust/blob/master/src/libpanic_unwind/emcc.rs

So I think this should work, but I don't have much experience with wasm.

@jonas-schievink jonas-schievink added A-destructors Area: destructors (Drop, ..) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ labels Mar 2, 2019
@coolreader18
Copy link
Contributor Author

No, wasm32-unknown-unknown, compiled using wasm-pack/wasm-bindgen. Looking through the source code, it seems like the comment here explains what's going on pretty well. Maybe there could be documentation for this, because this seems like fairly unexpected behavior, especially when most of std works with WASM.

@ids1024
Copy link
Contributor

ids1024 commented Mar 3, 2019

Ah yeah, with wasm32-unknown-unknown I'm not surprised it doesn't work.

Maybe there could be documentation for this, because this seems like fairly unexpected behavior, especially when most of std works with WASM.

In theory I think it's expected behavior for unwinding panics to not necessarily be available on all targets (and a library can't rely on it since an application could use panic=abort).

But yeah, it doesn't seem to be documented too well. I don't see any mention of unwind vs abort in the documentation for Drop, or even the documentation for panic!.

@alexcrichton
Copy link
Member

The wasm32-unknown-unknown target is panic=abort by default which means that panics "abort" the process, which in this case is a native wasm trap. When the exception handling proposal is implemented we'll integrate with that and provide the ability to run destructors on panics, but at this time the wasm target doesn't support any form of destructors on unwinding.

@coolreader18
Copy link
Contributor Author

coolreader18 commented Dec 16, 2019

Ah yeah, thanks, luckily I was able to work around an issue where panics would break the entire application. Do you know of an ETA for when exception handling will be standardized/supported in wasm runtimes?

@alexcrichton
Copy link
Member

I'd recommend following https://github.com/WebAssembly/exception-handling which is the standard we'll use once implemented in browsers.

@gmorenz
Copy link

gmorenz commented Nov 8, 2021

With that proposal now being implemented in v8, are we likely to see progress on this? Is there a rust issue to watch for this?

gmorenz added a commit to gmorenz/workers-rs that referenced this issue Nov 26, 2021
Per rust-lang/rust#58874
re-entering a webasm instance that paniced is undefined behavior.

The #[event(fetch)] macro currently inserts a panic in the case
that the fetch handler returns an `Err`, this removes that panic.
@lukechu10
Copy link
Contributor

Sorry for the ping but with exception handling implemented in both Chrome and Safari, any chance for progress on this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-destructors Area: destructors (Drop, ..) O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants