Closed surma closed 3 months ago
This is unfortunately one of the most fragile pieces of the bindings. It's also a bit unfortunate that currently we require leaking and 'static
lifetime; I attempted to fix some of this by trying to attach the lifetime of the callback to the context. That approach is probably the one I'd suggest we go after here, but it's definitely more involved than your suggestion.
I think that breaking changes are fine, especially if it means fixing issues like this one; in that sense, in the spirit of prioritizing correctness first, I agree that your approach is probably the best to fix the issue here. Do you mind pushing your fix?
@saulecabrera pushed!
Please don't forget to update the CHANGELOG file and crate version before merging
What version should we do? v4 for breaking change? Or is this small enough to warrant a 3.1.0-alpha.2?
Yeah 4.0.0-alpha.1
Now that https://github.com/bytecodealliance/javy/pull/618 has landed, I believe we no longer have this issue, I'll go ahead and close this PR.
This PR adds a test to show crashing behavior. I have a solution ready, but wanted to discuss options first.
The Problem
new_callback
(and by extensionwrap_callback
) take a closure, whose state has to be stored somewhere. Currently, it’s stored in aRefCell
that is leaked viaBox::leak
. However, the closure isFnMut
, which means the closure’s state needs to be mutated.RefCell::borrow_mut
allows this to work by deferring the borrow checks to runtime.However, in a re-entrant scenario (closure is invoked from JS, closure calls back in to JS and JS calls back into the same closure), the runtime borrow checker of
RefCell
will panic. This is, in my opinion, unexpected.Solution
The solution I have tested is to change the closure from
FnMut
toFn
. That way, we can ditch theRefCell
and just useBox::leak
to get a*'static const F
. If a developer wants to mutate state, they can still resort toRefCell
orMutex
or similar mechanisms to do so.If we change the existing APIs, it is arguably a breaking change. Adding new APIs that use
Fn
instead ofFnMut
seems clunky but allows backwards compatibility.Open to other suggestions.