Re-entry problem

[surma]

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 extension wrap_callback) take a closure, whose state has to be stored somewhere. Currently, it’s stored in a RefCell that is leaked via Box::leak. However, the closure is FnMut, 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 to Fn. That way, we can ditch the RefCell and just use Box::leak to get a *'static const F. If a developer wants to mutate state, they can still resort to RefCell or Mutex 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 of FnMut seems clunky but allows backwards compatibility.

Open to other suggestions.

[saulecabrera]

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?

[surma]

@saulecabrera pushed!

[jeffcharles]

Please don't forget to update the CHANGELOG file and crate version before merging

[surma]

What version should we do? v4 for breaking change? Or is this small enough to warrant a 3.1.0-alpha.2?

[jeffcharles]

Yeah 4.0.0-alpha.1

[saulecabrera]

Now that #618 has landed, I believe we no longer have this issue, I'll go ahead and close this PR.