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

Global drop queue for Root #700

Merged
merged 3 commits into from
Mar 22, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions crates/neon-runtime/src/napi/bindings/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ mod napi5 {
#[cfg(feature = "napi-6")]
mod napi6 {
use super::super::types::*;
use std::os::raw::c_void;

generate!(
extern "C" {
Expand All @@ -268,6 +269,15 @@ mod napi6 {
key_conversion: KeyConversion,
result: *mut Value,
) -> Status;

fn set_instance_data(
env: Env,
data: *mut c_void,
finalize_cb: Finalize,
finalize_hint: *mut c_void,
) -> Status;

fn get_instance_data(env: Env, data: *mut *mut c_void) -> Status;
}
);
}
Expand Down
39 changes: 39 additions & 0 deletions crates/neon-runtime/src/napi/lifecycle.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
use std::mem::MaybeUninit;
use std::os::raw::c_void;
use std::ptr;

use crate::napi::bindings as napi;
use crate::raw::Env;

/// # Safety
/// `env` must point to a valid `napi_env` for this thread
pub unsafe fn set_instance_data<T: Send + 'static>(env: Env, data: T) -> *mut T {
let data = Box::into_raw(Box::new(data));

assert_eq!(
napi::set_instance_data(env, data.cast(), Some(drop_box::<T>), ptr::null_mut(),),
napi::Status::Ok,
);

data
}

/// # Safety
/// * `T` must be the same type used in `set_instance_data`
/// * Caller must ensure reference does not outlive `Env`
/// * Return value may be `null`
/// * `env` must point to a valid `napi_env` for this thread
pub unsafe fn get_instance_data<T: Send + 'static>(env: Env) -> *mut T {
let mut data = MaybeUninit::uninit();

assert_eq!(
napi::get_instance_data(env, data.as_mut_ptr(),),
napi::Status::Ok,
);

data.assume_init().cast()
}

unsafe extern "C" fn drop_box<T>(_env: Env, data: *mut c_void, _hint: *mut c_void) {
Box::<T>::from_raw(data.cast());
}
2 changes: 2 additions & 0 deletions crates/neon-runtime/src/napi/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ pub mod date;
pub mod error;
pub mod external;
pub mod fun;
#[cfg(feature = "napi-6")]
pub mod lifecycle;
pub mod mem;
pub mod object;
pub mod primitive;
Expand Down
2 changes: 1 addition & 1 deletion src/handle/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
pub(crate) mod internal;

#[cfg(feature = "napi-1")]
mod root;
pub(crate) mod root;

#[cfg(feature = "napi-1")]
pub use self::root::Root;
Expand Down
30 changes: 26 additions & 4 deletions src/handle/root.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,33 @@ use std::marker::PhantomData;
use std::mem::ManuallyDrop;

use neon_runtime::reference;
#[cfg(feature = "napi-6")]
use neon_runtime::tsfn::ThreadsafeFunction;

use context::Context;
use handle::Handle;
#[cfg(feature = "napi-6")]
use lifecycle::InstanceData;
use object::Object;
use types::boxed::Finalize;

#[repr(transparent)]
#[derive(Clone)]
struct NapiRef(*mut c_void);
pub(crate) struct NapiRef(*mut c_void);

// # Safety
// `NapiRef` are intended to be `Send` and `Sync`
unsafe impl Send for NapiRef {}
unsafe impl Sync for NapiRef {}

/// `Root<T>` holds a reference to a `JavaScript` object and prevents it from
/// being garbage collected. `Root<T>` may be sent across threads, but the
/// referenced objected may only be accessed on the JavaScript thread that
/// created it.
#[repr(transparent)]
pub struct Root<T> {
internal: NapiRef,
#[cfg(feature = "napi-6")]
drop_queue: &'static ThreadsafeFunction<NapiRef>,
_phantom: PhantomData<T>,
}

Expand All @@ -40,15 +50,19 @@ impl<T: Object> Root<T> {
/// garbage collected until the `Root` is dropped. A `Root<T>` may only
/// be dropped on the JavaScript thread that created it.
///
/// The caller _must_ ensure `Root::into_inner` or `Root::drop` is called
/// The caller _should_ ensure `Root::into_inner` or `Root::drop` is called
/// to properly dispose of the `Root<T>`. If the value is dropped without
/// calling one of these methods, it will *panic*.
/// calling one of these methods:
/// * N-API < 6, Neon will `panic` to notify of the leak
/// * N-API >= 6, Neon will drop from a global queue at a runtime cost
pub fn new<'a, C: Context<'a>>(cx: &mut C, value: &T) -> Self {
let env = cx.env().to_raw();
let internal = unsafe { reference::new(env, value.to_raw()) };

Self {
internal: NapiRef(internal as *mut _),
#[cfg(feature = "napi-6")]
drop_queue: InstanceData::drop_queue(cx),
_phantom: PhantomData,
}
}
Expand All @@ -75,6 +89,8 @@ impl<T: Object> Root<T> {

Self {
internal: self.internal.clone(),
#[cfg(feature = "napi-6")]
drop_queue: self.drop_queue,
_phantom: PhantomData,
}
}
Expand Down Expand Up @@ -124,6 +140,7 @@ impl<T: Object> Finalize for Root<T> {
}

impl<T> Drop for Root<T> {
#[cfg(not(feature = "napi-6"))]
fn drop(&mut self) {
// Destructors are called during stack unwinding, prevent a double
// panic and instead prefer to leak.
Expand All @@ -140,4 +157,9 @@ impl<T> Drop for Root<T> {
);
}
}

#[cfg(feature = "napi-6")]
fn drop(&mut self) {
let _ = self.drop_queue.call(self.internal.clone(), None);
}
}
4 changes: 4 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ pub mod handle;
pub mod meta;
pub mod object;
pub mod prelude;
#[cfg(feature = "napi-1")]
pub mod reflect;
pub mod result;
#[cfg(feature = "legacy-runtime")]
Expand All @@ -35,6 +36,9 @@ pub mod macro_internal;
#[cfg(feature = "proc-macros")]
pub use neon_macros::*;

#[cfg(feature = "napi-6")]
mod lifecycle;

#[cfg(all(feature = "legacy-runtime", feature = "napi-1"))]
compile_error!("Cannot enable both `legacy-runtime` and `napi-*` features.\n\nTo use `napi-*`, disable `legacy-runtime` by setting `default-features` to `false` in Cargo.toml\nor with cargo's --no-default-features flag.");

Expand Down
50 changes: 50 additions & 0 deletions src/lifecycle.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
use std::mem;

use neon_runtime::raw::Env;
use neon_runtime::reference;
use neon_runtime::tsfn::ThreadsafeFunction;

use crate::context::Context;
use crate::handle::root::NapiRef;

pub(crate) struct InstanceData {
drop_queue: &'static ThreadsafeFunction<NapiRef>,
}

fn drop_napi_ref(env: Option<Env>, data: NapiRef) {
if let Some(env) = env {
unsafe {
reference::unreference(env, mem::transmute(data));
}
}
}

impl InstanceData {
pub(crate) fn get<'a, C: Context<'a>>(cx: &mut C) -> &'a mut InstanceData {
let env = cx.env().to_raw();
let data =
unsafe { neon_runtime::lifecycle::get_instance_data::<InstanceData>(env).as_mut() };

if let Some(data) = data {
return data;
}

let drop_queue = Box::new(unsafe {
let mut queue = ThreadsafeFunction::new(env, drop_napi_ref);
queue.unref(env);
queue
});

let data = InstanceData {
drop_queue: Box::leak(drop_queue),
};

unsafe { &mut *neon_runtime::lifecycle::set_instance_data(env, data) }
}

pub(crate) fn drop_queue<'a, C: Context<'a>>(
cx: &mut C,
) -> &'static ThreadsafeFunction<NapiRef> {
&InstanceData::get(cx).drop_queue
}
}
1 change: 0 additions & 1 deletion src/reflect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use handle::{Handle, Managed};
use result::JsResult;
use types::{build, JsString, JsValue};

#[cfg(feature = "napi-1")]
pub fn eval<'a, 'b, C: Context<'a>>(
cx: &mut C,
script: Handle<'b, JsString>,
Expand Down
3 changes: 3 additions & 0 deletions src/types/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use neon_runtime::raw;
use crate::context::internal::Env;
use crate::context::{Context, FinalizeContext};
use crate::handle::{Handle, Managed};
use crate::object::Object;
use crate::types::internal::ValueInternal;
use crate::types::Value;

Expand Down Expand Up @@ -159,6 +160,8 @@ impl<T: Send + 'static> Clone for JsBox<T> {
}
}

impl<T: Send + 'static> Object for JsBox<T> {}

impl<T: Send + 'static> Copy for JsBox<T> {}

impl<T: Send + 'static> Value for JsBox<T> {}
Expand Down
7 changes: 7 additions & 0 deletions test/napi/lib/threads.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,4 +64,11 @@ const assert = require('chai').assert;
// If the EventQueue is not unreferenced, the test runner will not cleanly exit
addon.leak_event_queue();
});

it('should drop leaked Root from the global queue', function (cb) {
addon.drop_global_queue(cb);

// Asynchronously GC to give the task queue a chance to execute
setTimeout(() => global.gc(), 10);
});
});
41 changes: 41 additions & 0 deletions test/napi/src/js/threads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,3 +144,44 @@ pub fn leak_event_queue(mut cx: FunctionContext) -> JsResult<JsUndefined> {

Ok(cx.undefined())
}

pub fn drop_global_queue(mut cx: FunctionContext) -> JsResult<JsUndefined> {
struct Wrapper {
callback: Option<Root<JsFunction>>,
queue: EventQueue,
}

impl Finalize for Wrapper {}

// To verify that the type is dropped on the global drop queue, the callback
// is called from the `Drop` impl on `Wrapper`
impl Drop for Wrapper {
fn drop(&mut self) {
if let Some(callback) = self.callback.take() {
self.queue.send(|mut cx| {
let callback = callback.into_inner(&mut cx);
let this = cx.undefined();
let args = vec![cx.undefined()];

callback.call(&mut cx, this, args)?;

Ok(())
});
}
}
}

let callback = cx.argument::<JsFunction>(0)?.root(&mut cx);
let queue = cx.queue();

let wrapper = cx.boxed(Wrapper {
callback: Some(callback),
queue,
});

// Put the `Wrapper` instance in a `Root` and drop it
// Without the global drop queue, this will panic
let _ = wrapper.root(&mut cx);

Ok(cx.undefined())
}
1 change: 1 addition & 0 deletions test/napi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ fn main(mut cx: ModuleContext) -> NeonResult<()> {
cx.export_function("greeter_new", greeter_new)?;
cx.export_function("greeter_greet", greeter_greet)?;
cx.export_function("leak_event_queue", leak_event_queue)?;
cx.export_function("drop_global_queue", drop_global_queue)?;

Ok(())
}