From bf8f568788c4a0826a4b4818b39ce92f99cff647 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Thu, 12 Sep 2024 23:25:09 +0800 Subject: [PATCH] refactor!(napi): Ref should not hold the value ptr (#2243) It's unsafe --- bench/src/async_compute.rs | 43 +++++--------- crates/napi/src/env.rs | 33 ++--------- crates/napi/src/js_values/arraybuffer.rs | 6 +- crates/napi/src/js_values/buffer.rs | 6 +- crates/napi/src/js_values/value_ref.rs | 64 +++++++++------------ examples/napi-compat-mode/src/napi4/tsfn.rs | 8 +-- examples/napi-compat-mode/src/task.rs | 15 ++--- 7 files changed, 61 insertions(+), 114 deletions(-) diff --git a/bench/src/async_compute.rs b/bench/src/async_compute.rs index 55e0c6e2..fea15081 100644 --- a/bench/src/async_compute.rs +++ b/bench/src/async_compute.rs @@ -1,54 +1,37 @@ -use bindgen_prelude::PromiseRaw; use napi::threadsafe_function::*; -use napi::*; +use napi::{bindgen_prelude::*, *}; -struct BufferLength(Ref); +struct BufferLength(Buffer); impl Task for BufferLength { type Output = usize; - type JsValue = JsNumber; + type JsValue = u32; fn compute(&mut self) -> Result { Ok(self.0.len() + 1) } - fn resolve(&mut self, env: Env, output: Self::Output) -> Result { - env.create_uint32(output as u32) - } - - fn finally(&mut self, env: Env) -> Result<()> { - self.0.unref(env)?; - Ok(()) + fn resolve(&mut self, _: Env, output: Self::Output) -> Result { + Ok(output as u32) } } #[js_function(1)] -fn bench_async_task(ctx: CallContext) -> Result> { - let n = ctx.get::(0)?; - let task = BufferLength(n.into_ref()?); +fn bench_async_task(ctx: CallContext) -> Result> { + let n = ctx.get::(0)?; + let task = BufferLength(n); let async_promise = ctx.env.spawn(task)?; Ok(async_promise.promise_object()) } #[js_function(2)] fn bench_threadsafe_function(ctx: CallContext) -> Result { - let buffer_ref = ctx.get::(0)?.into_ref()?; - let callback = ctx.get::(1)?; - - let tsfn = ctx.env.create_threadsafe_function( - &callback, - 0, - |mut ctx: ThreadsafeCallContext<(usize, Ref)>| { - ctx - .env - .create_uint32(ctx.value.0 as u32) - .and_then(|v| ctx.value.1.unref(ctx.env).map(|_| vec![v])) - }, - )?; + let buffer_ref = ctx.get::(0)?; + let callback = ctx.get::>(1)?; std::thread::spawn(move || { - tsfn.call( - Ok((buffer_ref.len() + 1, buffer_ref)), + callback.call( + Ok((buffer_ref.len() + 1) as u32), ThreadsafeFunctionCallMode::NonBlocking, ); }); @@ -58,7 +41,7 @@ fn bench_threadsafe_function(ctx: CallContext) -> Result { #[js_function(1)] fn bench_tokio_future(ctx: CallContext) -> Result { - let buffer_ref = ctx.get::(0)?.into_ref()?; + let buffer_ref = ctx.get::(0)?; ctx .env .execute_tokio_future(async move { Ok(buffer_ref.len()) }, |env, v: usize| { diff --git a/crates/napi/src/env.rs b/crates/napi/src/env.rs index 3e2d5ed3..1d225086 100644 --- a/crates/napi/src/env.rs +++ b/crates/napi/src/env.rs @@ -865,7 +865,7 @@ impl Env { } /// This API create a new reference with the initial 1 ref count to the Object passed in. - pub fn create_reference(&self, value: T) -> Result> + pub fn create_reference(&self, value: &T) -> Result> where T: NapiRaw, { @@ -875,34 +875,11 @@ impl Env { check_status!(unsafe { sys::napi_create_reference(self.0, raw_value, initial_ref_count, &mut raw_ref) })?; - Ok(Ref { - raw_ref, - count: 1, - inner: (), - }) - } - - /// This API create a new reference with the specified reference count to the Object passed in. - pub fn create_reference_with_refcount(&self, value: T, ref_count: u32) -> Result> - where - T: NapiRaw, - { - let mut raw_ref = ptr::null_mut(); - let raw_value = unsafe { value.raw() }; - check_status!(unsafe { - sys::napi_create_reference(self.0, raw_value, ref_count, &mut raw_ref) - })?; - Ok(Ref { - raw_ref, - count: ref_count, - inner: (), - }) + Ref::new(self, value) } /// Get reference value from `Ref` with type check - /// - /// Return error if the type of `reference` provided is mismatched with `T` - pub fn get_reference_value(&self, reference: &Ref<()>) -> Result + pub fn get_reference_value(&self, reference: &Ref) -> Result where T: NapiValue, { @@ -910,7 +887,7 @@ impl Env { check_status!(unsafe { sys::napi_get_reference_value(self.0, reference.raw_ref, &mut js_value) })?; - unsafe { T::from_raw(self.0, js_value) } + Ok(unsafe { T::from_raw_unchecked(self.0, js_value) }) } /// Get reference value from `Ref` without type check @@ -918,7 +895,7 @@ impl Env { /// Using this API if you are sure the type of `T` is matched with provided `Ref<()>`. /// /// If type mismatched, calling `T::method` would return `Err`. - pub fn get_reference_value_unchecked(&self, reference: &Ref<()>) -> Result + pub fn get_reference_value_unchecked(&self, reference: &Ref) -> Result where T: NapiValue, { diff --git a/crates/napi/src/js_values/arraybuffer.rs b/crates/napi/src/js_values/arraybuffer.rs index 234c9bc7..90d81007 100644 --- a/crates/napi/src/js_values/arraybuffer.rs +++ b/crates/napi/src/js_values/arraybuffer.rs @@ -5,7 +5,7 @@ use std::slice; use crate::bindgen_runtime::{TypeName, ValidateNapiValue}; use crate::{ - check_status, sys, Error, JsUnknown, NapiValue, Ref, Result, Status, Value, ValueType, + check_status, sys, Env, Error, JsUnknown, NapiValue, Ref, Result, Status, Value, ValueType, }; pub struct JsArrayBuffer(pub(crate) Value); @@ -198,8 +198,8 @@ impl JsArrayBuffer { })) } - pub fn into_ref(self) -> Result> { - Ref::new(self.0, 1, self.into_value()?) + pub fn into_ref(self) -> Result> { + Ref::new(&Env::from(self.0.env), &self) } } diff --git a/crates/napi/src/js_values/buffer.rs b/crates/napi/src/js_values/buffer.rs index 71dd5aea..a29bf8d5 100644 --- a/crates/napi/src/js_values/buffer.rs +++ b/crates/napi/src/js_values/buffer.rs @@ -4,10 +4,12 @@ use std::ptr; use super::{Value, ValueType}; use crate::bindgen_runtime::ValidateNapiValue; +use crate::Env; use crate::{ bindgen_runtime::TypeName, check_status, sys, Error, JsUnknown, NapiValue, Ref, Result, Status, }; +#[deprecated(since = "3.0.0", note = "Please use Buffer or &[u8] instead")] pub struct JsBuffer(pub(crate) Value); impl TypeName for JsBuffer { @@ -52,8 +54,8 @@ impl JsBuffer { }) } - pub fn into_ref(self) -> Result> { - Ref::new(self.0, 1, self.into_value()?) + pub fn into_ref(self) -> Result> { + Ref::new(&Env::from(self.0.env), &self) } } diff --git a/crates/napi/src/js_values/value_ref.rs b/crates/napi/src/js_values/value_ref.rs index 71f96ff1..0e138701 100644 --- a/crates/napi/src/js_values/value_ref.rs +++ b/crates/napi/src/js_values/value_ref.rs @@ -1,63 +1,53 @@ -use std::ops::Deref; -use std::ptr; +use std::{marker::PhantomData, ptr}; -use super::{check_status, Value}; -use crate::{bindgen_runtime::ToNapiValue, sys, Env, Result}; +use super::{check_status, NapiRaw}; +use crate::{ + bindgen_runtime::{FromNapiMutRef, FromNapiValue, ToNapiValue}, + sys, Env, Result, +}; pub struct Ref { pub(crate) raw_ref: sys::napi_ref, - pub(crate) count: u32, - pub(crate) inner: T, + pub(crate) _phantom: PhantomData, } #[allow(clippy::non_send_fields_in_send_ty)] unsafe impl Send for Ref {} unsafe impl Sync for Ref {} -impl Ref { - pub(crate) fn new(js_value: Value, ref_count: u32, inner: T) -> Result> { +impl Ref { + pub fn new(env: &Env, value: &T) -> Result> { let mut raw_ref = ptr::null_mut(); - assert_ne!(ref_count, 0, "Initial `ref_count` must be > 0"); - check_status!(unsafe { - sys::napi_create_reference(js_value.env, js_value.value, ref_count, &mut raw_ref) - })?; + check_status!(unsafe { sys::napi_create_reference(env.0, value.raw(), 1, &mut raw_ref) })?; Ok(Ref { raw_ref, - count: ref_count, - inner, + _phantom: PhantomData, }) } - pub fn reference(&mut self, env: &Env) -> Result { - check_status!(unsafe { sys::napi_reference_ref(env.0, self.raw_ref, &mut self.count) })?; - Ok(self.count) - } + pub fn unref(self, env: Env) -> Result<()> { + check_status!(unsafe { sys::napi_reference_unref(env.0, self.raw_ref, &mut 0) })?; - pub fn unref(&mut self, env: Env) -> Result { - check_status!(unsafe { sys::napi_reference_unref(env.0, self.raw_ref, &mut self.count) })?; - - if self.count == 0 { - check_status!(unsafe { sys::napi_delete_reference(env.0, self.raw_ref) })?; - } - Ok(self.count) + check_status!(unsafe { sys::napi_delete_reference(env.0, self.raw_ref) })?; + Ok(()) } } -impl Deref for Ref { - type Target = T; - - fn deref(&self) -> &T { - &self.inner +impl Ref { + /// Get the value from the reference + pub fn get_value(&self, env: Env) -> Result { + let mut result = ptr::null_mut(); + check_status!(unsafe { sys::napi_get_reference_value(env.0, self.raw_ref, &mut result) })?; + unsafe { T::from_napi_value(env.0, result) } } } -#[cfg(debug_assertions)] -impl Drop for Ref { - fn drop(&mut self) { - debug_assert_eq!( - self.count, 0, - "Ref count is not equal to 0 while dropping Ref, potential memory leak" - ); +impl Ref { + /// Get the value reference from the reference + pub fn get_value_mut(&self, env: Env) -> Result<&mut T> { + let mut result = ptr::null_mut(); + check_status!(unsafe { sys::napi_get_reference_value(env.0, self.raw_ref, &mut result) })?; + unsafe { T::from_napi_mut_ref(env.0, result) } } } diff --git a/examples/napi-compat-mode/src/napi4/tsfn.rs b/examples/napi-compat-mode/src/napi4/tsfn.rs index 34ea87d9..0a7b9620 100644 --- a/examples/napi-compat-mode/src/napi4/tsfn.rs +++ b/examples/napi-compat-mode/src/napi4/tsfn.rs @@ -121,13 +121,13 @@ pub fn test_tokio_readfile(ctx: CallContext) -> Result { #[js_function(3)] pub fn test_tsfn_with_ref(ctx: CallContext) -> Result { - let callback: Function, napi::JsUnknown> = ctx.get::>>(0)?; + let callback: Function, napi::JsUnknown> = ctx.get(0)?; let options = ctx.get::(1)?; - let option_ref = ctx.env.create_reference(options); + let option_ref = Ref::new(&ctx.env, &options); let tsfn = callback - .build_threadsafe_function() + .build_threadsafe_function::>() .callee_handled::() - .build_callback(move |mut ctx| { + .build_callback(move |ctx| { ctx .env .get_reference_value_unchecked::(&ctx.value) diff --git a/examples/napi-compat-mode/src/task.rs b/examples/napi-compat-mode/src/task.rs index 147859f6..277e3375 100644 --- a/examples/napi-compat-mode/src/task.rs +++ b/examples/napi-compat-mode/src/task.rs @@ -1,8 +1,8 @@ use std::convert::TryInto; use napi::{ - bindgen_prelude::PromiseRaw, CallContext, Env, Error, JsBuffer, JsBufferValue, JsNumber, - JsObject, Ref, Result, Task, + bindgen_prelude::{Buffer, PromiseRaw}, + CallContext, Env, Error, JsNumber, JsObject, Result, Task, }; struct ComputeFib { @@ -44,11 +44,11 @@ fn test_spawn_thread(ctx: CallContext) -> Result> { } struct CountBufferLength { - data: Ref, + data: Buffer, } impl CountBufferLength { - pub fn new(data: Ref) -> Self { + pub fn new(data: Buffer) -> Self { Self { data } } } @@ -71,16 +71,11 @@ impl Task for CountBufferLength { fn reject(&mut self, _env: Env, err: Error) -> Result { Err(err) } - - fn finally(&mut self, env: Env) -> Result<()> { - self.data.unref(env)?; - Ok(()) - } } #[js_function(1)] fn test_spawn_thread_with_ref(ctx: CallContext) -> Result> { - let n = ctx.get::(0)?.into_ref()?; + let n = ctx.get::(0)?; let task = CountBufferLength::new(n); let async_work_promise = ctx.env.spawn(task)?; Ok(async_work_promise.promise_object())