JsBox RFC

[kjvalencik]

JsBox is a smart pointer to data created in Rust and managed by the V8 garbage collector. JsBox are a basic building block for higher level APIs like neon classes.

fn person_new(mut cx: FunctionContext) -> JsResult<JsBox<Person>> {
    let name = cx.argument::<JsString>(0)?.value(&mut cx);
    let person = Person::new(name);

    Ok(cx.boxed(person))
}

fn person_greet(mut cx: FunctionContext) -> JsResult<JsString> {
    let person = cx.argument::<JsRef<Person>>(0)?;
    let greeting = person.greet();

    Ok(cx.string(greeting))
}

Rendered RFC

Work in progress implementation

[dherman]

This is very compelling! Seeing the contrast and drawbacks really helps justify the design you ended up with.

[dherman]

Would it make sense to consider a high-level convenience type, something like type JsRefCell<T> = JsBox<JsRefCell<T>>;, for the common case of interior mutability? This would make a common case more concise, e.g. in your Pool example above, it would change from

     let mut pool = cx.argument::<JsBox<RefCell<Pool>>>(0)?;

to

     let mut pool = cx.argument::<JsRefCell<Pool>>(0)?;

It would also create an opportunity to document the use case of interior mutability directly in the API docs, making the idiom a little more discoverable.

Maybe not necessary for this RFC, but we could leave a note in "unresolved questions" about future convenience APIs and mention the idea there.

[kjvalencik]

I added a section about future extension that might include this. In my opinion, this is above and beyond this RFC since in addition to the type alias we would want other helper methods (e.g., cx.ref_cell(..))

[dherman]

I'm still kind of shocked about this soundness hole. I could've sworn I checked for it and panic.

Conceptually, I still find the idea of being able to use RAII and Rust mutability rules to lock the main JS thread to be pretty compelling. But I like how you've at least decoupled that idea from the idea of safely exposing Rust data owned by the JS engine back to Rust.

I wonder if we might still have a place for the Lock API just as a way to have self-documenting critical sections. OTOH, it might end up being a cute idea that's not particularly useful anymore. Curious what you think?

[kjvalencik]

I don't think the Lock API fits here because nothing is being locked. I don't understand what you mean by RAII to lock the main js thread. The existing API doesn't take any action to lock the the VM.

[kjvalencik]

@dherman I thought about this some more and it finally clicked. I think the Lock guard API only makes sense for accessing data from Buffer.

When accessing a Buffer, Neon needs to uphold Rusts guarantees on ownership and mutability of the slice. However, since the data is not owned by Rust, there may exist references unknown to Rust.

For example, imagine a scenario where a user borrows a &[u8] to the Buffer. According to Rust invariants, this data will not change during the lifetime of the borrow. However, if JavaScript is invoked, e.g. by calling a JsFunction, the data could be mutated, violating the invariant.

The Lock API solves this by holding an exclusive reference to Context. The Lock statically prevents calling any Neon methods that could potentially mutate the slice.

This problem does not exist for JsBox. The data is owned by Rust and can't be mutated by pure JavaScript. At best, the user could invoke JavaScript that calls another Neon method. However, that Neon method would still be bound by Rusts dynamic borrow checking (RefCell). It's simpler, more ergonomic, and no less safe to return shared references without a Lock/Guard on JsBox.

Lastly, there is an edge case where the Lock API might not uphold invariants. If the Buffer was created as an external buffer (memory created and managed externally), it's possible for other non-Neon native code to mutate these types of buffers. Neon could use n-api methods for detecting externals and refuse to lend them. Additionally, neon could provide unsafe fn borrow_unchecked methods for reading data from externals.

[dherman]

It took me a couple of reads but I think you've really nailed the difference:

• Rust data (JsBox) can be protected by Rust's Cell invariants, so we don't need to worry about reborrows via VM re-entrancy;
• C/C++ data (JsArrayBuffer) can't be protected by Rust's Cell invariants, so we do need to worry about reborrows via VM re-entrancy.

So if I understand you, you're saying the Lock API makes sense to keep, but specifically and only for typed arrays.

On the one hand, it's nice that we can borrow a JsBox more ergonomically. On the other, the asymmetry is a little frustrating. It's a shame there's no way (AFAIK) to dynamically borrow an ArrayBuffer, which we could then use to make this safe without having to lock the JS VM. You can permanently detach its backing buffer, but that doesn't help this use case; we'd need a temporary, reversible version of detaching.

The Lock statically prevents calling any Neon methods that could potentially mutate the slice.

Exactly! This is definitely subtle (if ingenious—thank Niko for the idea), and is probably worth some better explanation in docs, assuming we keep the API.

If we only end up needing it for ArrayBuffer and not JsBox, I wonder if we want a less general name than Borrow, especially since JsBox doesn't use it. Maybe something that makes more explicit that it's a stop-the-world borrow, so the connection to Lock is clearer?

Lastly, there is an edge case where the Lock API might not uphold invariants. If the Buffer was created as an external buffer (memory created and managed externally), it's possible for other non-Neon native code to mutate these types of buffers. Neon could use n-api methods for detecting externals and refuse to lend them. Additionally, neon could provide unsafe fn borrow_unchecked methods for reading data from externals.

Good catch. That makes sense to me.

So, some of this stuff is out of scope for this RFC, but probably worth summarizing the issues in the "open questions" section so we can capture them for subsequent work.

[kjvalencik]

@dherman I like the clear naming of borrow for the API. However, we could move it from neon::borrow to neon::binary to clarify the relationship to JsArrayBuffer.

[dherman]

Love this idea!

[dherman]

Yes, these are both highly rare cases, which is the spirit of panic.

[kjvalencik]

Currently the RFC defines the APIs as returning a JsResult, but that's inconsistent with other methods. I think that should be changed to return an unwrapped JsBox<_>.

If we ever decide in the future that we want this potential failure exposed directly, it would be best done all at once for consitency.

[dherman]

I feel comfortable making all the APIs return an unwrapped JsBox<_>. NeonResult is meant to represent whether the operation caused a JS exception. Failing because it was already in a throwing state is basically a programmer error that we happen not to be able to enforce through the type system and therefore a panic; the VM shutting down is extremely rare and unrelated to triggering a JS exception.

[kjvalencik]

This RFC has entered the final comment period.

[kjvalencik]

Future Expansion

Background

While exploring the Persistent RFC, an alternate design that requires manual drops was proposed. Manual drops are very simple for single use Persistent, but can become complicated when Persistent are cloned.

Possible Solution

JsBox could support an optional finalizer that is called on the main JavaScript thread prior to the Rust data being dropped. Since in most cases multi-use Persistent will be owned by a JsBox, it provides an opportunity to perform a manual drop.

API

pub trait Finalize {
    fn finalize(self, cx: FinalizeContext);
}

impl<T: Send + 'static> JsBox<T> {
    pub fn with_finalizer<'a, C>(cx: &mut C, value: T) -> Handle<'a, JsBox<T>>
    where
        C: Context<'a>,
        T: Finalize;
}

An additional JsBox constructor is added that accepts a T: Finalize and calls the finalize method prior to dropping.

Following idiomatic patterns establish elsewhere, Persistent would implement a Drop trait that panics to prevent user error.

impl<T> Drop for Persistent<T> {
    fn drop(&mut self) {
       panic!("Persistent must be manually dropped.");
    }
}

Alternative

An alternative design is to provide the finalize method as an argument to the constructor instead of as a trait.

impl<T: Send + 'static> JsBox<T> {
    pub fn with_finalizer<'a, C>(
        cx: &mut C,
        value: T,
        finalizer: fn(FinalizerContext, T),
    ) -> Handle<'a, JsBox<T>>
    where
        C: Context<'a>,
}

The advantage of the trait approach is that it co-locates the finalizer with the data. The disadvantage is that Neon would not prevent a user from defining a trait, but failing to use the with_finalizer constructor.

In my opinion the trait approach is preferred and the ergonomics could be improved in and when the Rust specialization RFC is implemented.

Example

struct MyServer {
    server: Server,
    // Wrapping the `Persistent` in a `ManuallyDrop` provides two advantages:
    // 1. Immediately signals that something in this struct needs special attention
    // 2. If a manual `drop` is missed, it causes a leak instead of a `panic`
    callback: ManuallyDrop<Persistent<JsFunction>>,
}

impl Finalize for MyServer {
    fn finalize(self, cx: FinalizeContext) {
        self.callback.drop(&mut cx);
    }
}

fn create_server(cx: FunctionContext) -> JsResult<JsBox<MyServer>> {
    let callback = cx.argument::<JsFunction>(0)?.persistent(&mut cx);
    let server = MyServer {
        server: Server::new(),
        callback: ManuallyDrop::new(callback),
    };

    Ok(JsBox::with_finalizer(server))
}

[dherman]

The changes look good to me! Just one FIXME to update.

[kjvalencik]

Merged in 951e625