search

darfink / chakracore-rs

An idiomatic Rust wrapper for the JSRT interface
39 stars 10 forks source link

Cloning Context/Value fails w/o ContextGuard #14

Open srijs opened 7 years ago

srijs commented 7 years ago

Problem:

The JsAddRef and JsRelease APIs require a current context to be set in order to work. However, they are being exposed through the Clone and Drop traits that don't require a ContextGuard to be present when clone is called, or the value is dropped.

This results in the following panics, when a value is cloned and/or dropped w/o a current context:

thread '<unnamed>' panicked at 'Call to 'JsAddRef(value, ::std::ptr::null_mut())' failed with: NoCurrentContext', /
Users/sam/.cargo/git/checkouts/chakracore-rs-259abda0f25cff98/cbe5ca1/src/value/function.rs:158
thread '<unnamed>' panicked at 'Call to 'unsafe { JsRelease(self.as_raw(), &mut count) }' failed with: NoCurrentContext', /Users/sam/.cargo/git/checkouts/chakracore-rs-259abda0f25cff98/cbe5ca1/src/value/function.rs:158

I'm struggling to think of a way to expose a safe API through clone/drop, tbh. Especially drop seems to be problematic, because Rust offers no way to control under which circumstances a value might be dropped.

A possible way to fix it could be to tie the lifetime of a Context or Value to the lifetime of the ContextGuard, which would require the value to be dropped before the context guard is dropped, i.e. before the current context is unset, and would require the context guard to be alive in order to clone a value.

The downside is that this complicates storing contexts or values in Rust data structures, but it's the only possible solution I can think of at this point.

darfink commented 7 years ago

I've spent quite some time this forenoon contemplating just this. I've implemented a fix for the Drop scenario recently, but it did increase the complexity of the implementation, and whilst something similar would be possible to implement for the clone scenario, I feel the solution would be too grotty.

The most prominent alternative I've come up with so far is using a default context, created during the Runtime instantiation. This may work thanks to the fact that objects recently became shared between contexts in ChakraCore. Therefore one context may decrement the reference count of values constructed using other contexts.

In fact, I'm not quite sure of the list of things not shared between contexts.

This would solve these problems, though some issues would persist in a multi-threaded environment, since Runtime is Send. Values created on a runtime's previous thread would no longer have any default Context to rely upon.

And I agree that introducing additional lifetime dependencies (i.e Value → ContextGuard) is preferred to be avoided.

darfink commented 7 years ago

With a detailed response from a ChakraCore team member I believe I am confident enough to start implementing the default context approach. This would alleviate several of the aforementioned issues, and would allow the API to be simplified. Though the safety risks associated with multi-threading would remain.