From ffc4980d520f6c57e6ee24ca3c9c4561f3419f71 Mon Sep 17 00:00:00 2001 From: Alberto Pose Date: Tue, 14 Mar 2023 07:32:17 +0000 Subject: [PATCH] fix(napi): panic when Promise callbacks trigger after Promise is dropped (#1469) (#1516) Co-authored-by: Alberto Pose --- .../src/bindgen_runtime/js_values/promise.rs | 33 ++++++++++++++---- examples/napi/__test__/typegen.spec.ts.md | 2 ++ examples/napi/__test__/typegen.spec.ts.snap | Bin 3810 -> 3828 bytes examples/napi/__test__/values.spec.ts | 30 ++++++++++++++++ examples/napi/index.d.ts | 2 ++ examples/napi/src/threadsafe_function.rs | 21 +++++++++++ 6 files changed, 82 insertions(+), 6 deletions(-) diff --git a/crates/napi/src/bindgen_runtime/js_values/promise.rs b/crates/napi/src/bindgen_runtime/js_values/promise.rs index f6168d1f..09fe1fd8 100644 --- a/crates/napi/src/bindgen_runtime/js_values/promise.rs +++ b/crates/napi/src/bindgen_runtime/js_values/promise.rs @@ -1,7 +1,11 @@ -use std::ffi::{c_void, CStr}; +use std::ffi::CStr; use std::future; use std::pin::Pin; use std::ptr; +use std::sync::{ + atomic::{AtomicBool, Ordering}, + Arc, +}; use std::task::{Context, Poll}; use tokio::sync::oneshot::{channel, Receiver, Sender}; @@ -12,6 +16,13 @@ use super::{FromNapiValue, TypeName, ValidateNapiValue}; pub struct Promise { value: Pin>>>, + aborted: Arc, +} + +impl Drop for Promise { + fn drop(&mut self) { + self.aborted.store(true, Ordering::SeqCst); + } } impl TypeName for Promise { @@ -90,7 +101,8 @@ impl FromNapiValue for Promise { let mut promise_after_then = ptr::null_mut(); let mut then_js_cb = ptr::null_mut(); let (tx, rx) = channel(); - let tx_ptr = Box::into_raw(Box::new(tx)); + let aborted = Arc::new(AtomicBool::new(false)); + let tx_ptr = Box::into_raw(Box::new((tx, aborted.clone()))); check_status!( unsafe { sys::napi_create_function( @@ -98,7 +110,7 @@ impl FromNapiValue for Promise { then_c_string.as_ptr(), 4, Some(then_callback::), - tx_ptr as *mut _, + tx_ptr.cast(), &mut then_js_cb, ) }, @@ -133,7 +145,7 @@ impl FromNapiValue for Promise { catch_c_string.as_ptr(), 5, Some(catch_callback::), - tx_ptr as *mut c_void, + tx_ptr.cast(), &mut catch_js_cb, ) }, @@ -154,6 +166,7 @@ impl FromNapiValue for Promise { )?; Ok(Promise { value: Box::pin(rx), + aborted, }) } } @@ -193,8 +206,12 @@ unsafe extern "C" fn then_callback( get_cb_status == sys::Status::napi_ok, "Get callback info from Promise::then failed" ); + let (sender, aborted) = + *unsafe { Box::from_raw(data as *mut (Sender<*mut Result>, Arc)) }; + if aborted.load(Ordering::SeqCst) { + return this; + } let resolve_value_t = Box::new(unsafe { T::from_napi_value(env, resolved_value[0]) }); - let sender = unsafe { Box::from_raw(data as *mut Sender<*mut Result>) }; sender .send(Box::into_raw(resolve_value_t)) .expect("Send Promise resolved value error"); @@ -224,7 +241,11 @@ unsafe extern "C" fn catch_callback( "Get callback info from Promise::catch failed" ); let rejected_value = rejected_value[0]; - let sender = unsafe { Box::from_raw(data as *mut Sender<*mut Result>) }; + let (sender, aborted) = + *unsafe { Box::from_raw(data as *mut (Sender<*mut Result>, Arc)) }; + if aborted.load(Ordering::SeqCst) { + return this; + } sender .send(Box::into_raw(Box::new(Err(Error::from(unsafe { JsUnknown::from_raw_unchecked(env, rejected_value) diff --git a/examples/napi/__test__/typegen.spec.ts.md b/examples/napi/__test__/typegen.spec.ts.md index 4bc5c775..849279e2 100644 --- a/examples/napi/__test__/typegen.spec.ts.md +++ b/examples/napi/__test__/typegen.spec.ts.md @@ -211,6 +211,8 @@ Generated by [AVA](https://avajs.dev). export function acceptThreadsafeFunction(func: (err: Error | null, value: number) => any): void␊ export function acceptThreadsafeFunctionFatal(func: (value: number) => any): void␊ export function acceptThreadsafeFunctionTupleArgs(func: (err: Error | null, arg0: number, arg1: boolean, arg2: string) => any): void␊ + export function tsfnReturnPromise(func: (err: Error | null, value: number) => any): Promise␊ + export function tsfnReturnPromiseTimeout(func: (err: Error | null, value: number) => any): Promise␊ export function getBuffer(): Buffer␊ export function appendBuffer(buf: Buffer): Buffer␊ export function getEmptyBuffer(): Buffer␊ diff --git a/examples/napi/__test__/typegen.spec.ts.snap b/examples/napi/__test__/typegen.spec.ts.snap index 8bcb74de66edee8b56455c8ebab37d6f241c9cca..7ab511221fef7467a82a3e1e1e95c7356724d78d 100644 GIT binary patch literal 3828 zcmV*N`1cG7M+o3sx&Bk!?waw5}64 zMm$T-?vBMJ8O~7BMiJ2bL7)3lBwyH1=s9yChnLx1Ic^+V>57qhg8BQj+P%c*1}O!J(DlE_8INW>;&!ehooLNkDr;+J2eh)Vk1qi_E7 z*=^i z?AeFbybw5+Y==(|5*e%qcJT1Q5=cD^Ch+>EB*;BE7neI-A`HUt;K z6WHQE!^*|+nlw%&&83e@2U<<|hJq*}OQcyzN4;%*0&$PQYc;vJ5nA*y;GsErLaxX; z_!q?egluh)30P^7GXZaJZwr=3?CuAVCb!>_>ntw?%lfwh{3VZWbyW$!U0e1{bv|FQ8=tK7(ERv#6gl367VSuBVeh5*&vR1EYB&siJ| zF(e*9RH6xdz_>T&dDJI6@Gpe3ZYQPZzu|JGrYl6KY9HQ?bqX0j@5ZBpi62?dFxSMH z#f3OdSRf(#_Q@L_$(cWRLCV2K>5=*vaJ{g*Fy@~T&hj)#ch$T@9>-kVbRI;G%|o6P zk~L=HkM<(rs8A~+(61OULRb<#GcWD+VW3Nv>yU7q*X1j>RT=R-EbCpgr%>6+<(vi< z4+q;GqVOqfz`IOnWs=I&O3i8H{R-K0p)EDdwR%?K zJ1pd6P^boE#t&S_kLSD`u9Wr+lDltQVpy(P4hArrtr z?zbuyHTMNiBBe>hx09o3~X&32>ugu97m((_LW$5B!){|Pw;jQuMKRrFz%>MN`9Dm@Ui08`Mi*>V5<;q zpbh>5zn8*y>I+PhV-}*SZAjnpwF_@MQ`fiwRG|84^^DOXW059koXKnOl8@BL8uU~R zSUR>9Rc|><`=WsNoyc#Vypu4y38Gc0l?y%M^Bc^lR`cfeU~!b5J{;SGd{ts)r+!1l z?2u+wl_xAr^XRdy0oUf>2x<$|t169FEA_RFwK%5?ylYIfX=E!A@Xj!SD9Bn}l{NY- z7RmZfs+^&^acYKoT0=ISoq#2%wOCqxZV&~7^?4Q5TP3QWq)xBORXCU&B#Mhhs_+pQ zthx=NW*X3y8MGpcIAIBeJw*FZ@jF=;xi0nI(Tq#aq1xpUBqk)CU(b^Ht)i1waSf~(p4qp7>3-ogG?_R7Esugy@pQq9PT`g&&$i%!v}+y z(=v-JJ+!}(1`Z)Zd++%iLRZ_?PSTy*n>vy?%}jO8S?mDz7u)W7xuB$b)1xFcj_*T>T5S5ZG$4YuM_tdpjS?8{S{X>6j`d z;CV+nAyYNb1-e=ls)62CsZ%4xv2|cNe5pLPX4>#hr|j91(#Pb4EnKJ`nAfJ!X&T+A#JQ0#0VC&_G7adi5+VMey&?y zQ0FCOt&8tV+pC66v-NF**Slm*irNoc)rqcZ>DGYDmaB@E-EIv}o?gX;*t~y#y}YP) zl$bag!&)~3#$aFH!`{17-&R1cG{AHG-m+#>i9-Qxd;8$2u%0$(b$M>SIkO^`G1&S^ z$eDQ7iZ`J2rwZtNcQsts>d?sH8g0gT?J_Xu^AYAQS@m9<>(3)36Y>-fF-~APBY?>Q zJdIW@nqI&AoB+yE$`cWYw8%qNWfYR;z>|}$`qDVS5K`RsYZc%M__dsDK3pICC6zqc zTn9e4a<9pb0Dq-&>w^UMwa&AheA^QlwU06RdC$VPz%IZ8rG*@fVRQ(3&eJ#sKjTRU zRuvM6Z5>n_MDVF9=@s;;dRYTMItc6z(MZsVQx8^t!_)EexCq(8HLChOs>=Mm`1ZlJ z<3nuYd|Gs$ki}t~iXvx2efp&i-)@doOw@syDg*4n*+*ki7Yz{2N)*;$5LL(BgD_;7 z>@XOEcHO|{jW8bkYLzUJPl9n&WHE#4N-SYrA4u8_rcb$zL)6FIwZrulOHezL{Rn-n z+Q}^%*iHgR^nV})A;L87dbTWV>pjEpY5RM{#Hk-WGqBZ({qmKlAwED@m#??CT75t> zwDPu4y-Dlz6niy1on9ePFe+f8Ns(D3) zK>u1yXAK>%yhK!`!!0DWhN?wW4SH@Qov^3~S?7YUvZuUAPD6;_P<*H#P6#iOvn0Jp zFp<1P?vVGYEW>Ij&EK1gA}W`BuSefQ2KHmY@oon=JE*Ez90?**GKRdc7{@%^Ks4$| z@d8+agl9_lJOfY^gE;6B?r(4L0!iSE_TJrck7Cqiyg|a}G%k;o)EN`xqErv7G_<^i zcLpE(3ZzV6b#{!It3yXma|VKH=0+_M?lllm0=Y9xSdck`OeE5n1lTPPRXTwv9BjB( z6sG`%b&=|z^_+xR*uf@MgvLS*p^Zes4mtQWe|X){VN$=&@fO?N7!RAmbUtT^)Z@q| z)(dQ&e&FQ4|M~Yn)i?@rF-z&3*BErA*K$I1tBDPq?xs^Ec+b_oruGeNOm&>7E+d9B znu7xIJ|Z55bIK>-d2NH9qId$jz_Vbu9B|0p}P~)GqyFxBMOE` zo%(%8zpI+`be^7J&lGDXD49N z1*Q$>rE4}+>VU5Quf>rHTDv7@2VZx&Q#AcueeCbpewQ(6S5viFJhrt_18kY<5l=qQ zQIX4-brCTl^70DT@xKb|Mw}zk);i*Kaldl(35vn$1ur z4j;Ku>5ckm0{Q{i9)G&&5Cpynp?K=!eD<=#N(&MgfEa z9yt}PL1nw@opdaj4k|t(>WcjHSjzt=#=*d=EZ zgLi@ z-+|lcS66d^V9a$)h`WQ{widX^b5?85ClH-HhR^`ssKNF=lKa+bp0F%!Ek?w1cf;h6 z5lf~oBA%${E<8A1nqya>Rla$6dU{gC>?IfIpNB%&w}zcp9~4gT&G_E&OFi0eXB2gH zDi<+3oiV2JI3QIGbk-T&;iEQBGwI-Ivgv>})mTpia^%0ky$w6LmCaQMfl@$k9hZvI zK*xAP2n*7XrE_NIc5O-K<+CJ=@`?=4P%bZL+Tv^HnuC`IUmqRr9gQ4|@3;Z5{lmit zzT^wvO?oP<;P;46 qA@2`d7Op+I+j4d13bcjiU@H#!yLI~Q%9|C}xBdr9OoJwqH~;`BKVn1x literal 3810 zcmV<84ju79RzVS6P}%62?>rvUep}p3wMs{1NziI(km7?+}tR8l_3RAfJEUR}GD(BH`Z` zjEKd2oW|<&*{i;JecvY|Xu7#`1FYO5qZtIWygbGl&auRLmU& zP5Zn>D2llvgzut3+w(Fg)0YB7i0G-3q-T793rdu;_nlNsz@oKj@!u9buF5{B&_FDb zFvzH!!7?b(K+iXMI_H8t-6!@rXYUmTJhk_qVemv@SYt4H9$1QlBr7D~DTEQQRMBh@XL>Bp ztJ&#A26=(a0WaaspbpW&Plke4KnKat;0NJ&)AE2gF!Fe-E>oYBU<0#!LPJKh=iBEj zj)v$G55OzY1U{hO8}mHslO6aQ+*!Ag(*56YIaA#g5Q^G|+p$g|;}_jH+L`#ldWN|s zW)>IXIAMVV@7pKucqC_j=LI1L8>L6;W5D&o?n0ZtM>xyVB;8g04tX4Naoc|2IW`Y@ zQb^Y5i9hW{!d9VHM4(+UWQ4FJdSPDL>%%~nEY~jKIIqvI+*YN>^RTRU(Vl{`oy$HA z6b}d6oQngZY8oXbDy%btg(_D*Z zCEj5nKQ-ho$JmA?8n^^(;sEp@N%3iY{A%4bc60H)Ona6ELx|(dTH+24OAFuDf`QV3 zNt*Usi(e7#Xp=?rWkf?m^h2Adi(h4T%DE4K%N_VL^oLct?+)nMab9{J)4kHZz~hK| zK4+F`-*C{m6CK}CMe4T2460KF5HJPj19qdBlM5ObU{z?2d08UC8l{k`Y;Or|WXJ>% z$ir5~qWZq#Nu(r+s5>-2S4jsUyXh2yfrlIB%PTOBLU|GMeMlvI-?ZOHRLF(tS%l{p zRaEQY47WGWfDXqqY1OxB0pBsV9D^4+Cm_t)U4Iy9GAEC2hl(+XY-J&a zX~ew9Ix?`ibs+c;%rP8|oZFXT)j(M=Go3awn~5ue|pN41)_*9VJHTKaHo1M*dYmF@a9 z6|+N{Sy7&_FwLWIHDkHRaMrg zvsjSzja1n~RpV3-^|Z#^baVompw?n(_PIe64d&;SS8t`L{vmaERj$Ipedq*)Yy?|_&M-Ze${DMaXjR*PkI4SoA z1fa*7Bw|Tb=9t(WQ|i1+x7A8FZac0@DaX*{mK|hz>9By4U0G}BWY6KwQ~12Pnmv9r zh&e5z$kIal3u#~wGSv5;&meS_ZEYuAxxK9+nbXWf=b%O}oY1mEtIKcr?$JzTkb3=P zB$W~JIYz1rta?0EU|nT(CVUKAI1G7^tR{xUU4p59#TEif4R#G%Rd#RXV|l^*t2iA~ zB?LV0NGD{f8oEeVi$WF9+ah&xq&T()OuH`?$JSIE-szORSd#jfn6QO~;(>ncF*^0b zoq!W%4_+S(P8C-Jn2u=|CX3Fv_W!1Qg&3?|8moIt6&44l3*AKAv?&P->(Hl+h62G^!*<|97K-=0rSSrk?4XjSj%{Ql4#4-k3 zKM6S#FIxTvg#KI+UF@!=>slQeIb5U8IImp_=6pWF*d?ppYkmE3gk(aV;vvQfEN28X zS%9a}szlT4cb^kLI!bvW0+AMZ$f}4!(j0hlqE%lQ2M8g>ZNF9mu87~t$>!tr$zM~+ zlg)MLb35-f*%4r`RBU~a;J(&zwi9oAJfpTTCO+?3`WD$GSfI3!gE6!YLC$#^$6#kX z;lQdy0@2oi+8}~YRZ6d@Pu0sB_-G)oIz%HuCr&dsLSBTk-Zm zx8p-B<9u4UPsrjhPDPQkp+5amhi|t>6%%z}rqTd=aQ3T_)I|e?SqWhk2BA9c9)uyw zWQW2S*mWJ7*TZ=5)hgLRJ_+Ke$YKWBl~}^MK9IB91v4Pt?LB{UdR@PqR0_ExKKW|~%Buc=oxog!SXhU2L`wu^TA zaY~)RJEaz}oK33g9@@R?P1!#6I*jSu!uxZku@cQ2Dg+v&VmfQcc;&5|Dt>H%)GCk` zsA}|F;W=SZ5wgw&UuCuPA~_Ag;X$;nejJ}yB;6a_lvLD1l%TIxNVXf$NSmk^sHsq4K{7;h^DOQJex5CK9UT)N>+CVH=yHA~j|< zNNr{$Y?Fhp`NR8p4wL$Ij@Q2K$9Osurt>*Vq;5wxF~DZ?^dl$#`|p4LrP^^Km$Q`4 zd5uAL_IjS+4rvs*qG}4P2JuLXEX-^;sriD#VTK6Zr>LWKQp0w z$r+Wz+^hucVj`rlZSCF4c+uBn-<`d`zWL(2{@uIf6m;3;xH{9t3kT`6bPg< zXo9_NL3}m0L$&ZhlIC+XZdE*-mK8H>y)N^Jn_lyJiPmg9N9@Q{T(zt#MWa7r-VQ)% z)esXoCqY$>m`>;mhK@zCrJ-_Mb)|JDrsr&Hj5U3RXM_5EN588i?sT4>W04V)4P`%6 zKXX}?*YY0V#JBTLk9}Bx(kR_g=p-Dj`J{sWTLn%#^lKxr$Eq0j1&{UOE{< zqHEF+I|kS6N|#LLbTMW}j73$A97{K9c-Pc7R;5>;;Vv+3I4@1J8Kq9>+W(p#si?JC za(3`dmpMh%-__^jj_!9UlQuO~UBaVV8#Tg~svhy=BOMjFj9J$pW{SML^K?A1ftztr zJy*V{V?z~VkOiJkCx}j@g8vH`w159nvrh@TJfYbPW#aIm_5P5G^I;n6eP8lpHbkvW zTMx8^=EfEN-P1n}6?-WLLg0nvXGA|VmOy{J?obLCIN*_!@);;whVC544%0D;ZxD4S z{SB2f#||`>%vRFe?O5GkHGx3nSih?B=09U-uep0&)j}*ilWLB<920r9u$#BSt!A|R zf~s$LbtUz7LTPTARKCl(#9RN$U)|Q$3!}xg_NGlVHch#~eGaVhA6QFkF7a|5ho9rV5lxBONE4P)0ln-G49)VHu^pmlevy^~s&wd3;H z>TdgdtccEZvFHJp=q1zqJ#Nhd^YYY+USEN=q5f*1ucQpXbb~;AClmt538BE&ZU0OMg^djJ3c diff --git a/examples/napi/__test__/values.spec.ts b/examples/napi/__test__/values.spec.ts index 474f2742..40b807a0 100644 --- a/examples/napi/__test__/values.spec.ts +++ b/examples/napi/__test__/values.spec.ts @@ -123,6 +123,8 @@ import { acceptThreadsafeFunctionTupleArgs, promiseInEither, runScript, + tsfnReturnPromise, + tsfnReturnPromiseTimeout, } from '../' test('export const', (t) => { @@ -849,6 +851,34 @@ Napi4Test('accept ThreadsafeFunction tuple args', async (t) => { }) }) +test('threadsafe function return Promise and await in Rust', async (t) => { + const value = await tsfnReturnPromise((err, value) => { + if (err) { + throw err + } + return Promise.resolve(value + 2) + }) + t.is(value, 5) + await t.throwsAsync( + () => + tsfnReturnPromiseTimeout((err, value) => { + if (err) { + throw err + } + return new Promise((resolve) => { + setTimeout(() => { + resolve(value + 2) + }, 300) + }) + }), + { + message: 'Timeout', + }, + ) + // trigger Promise.then in Rust after `Promise` is dropped + await new Promise((resolve) => setTimeout(resolve, 400)) +}) + Napi4Test('object only from js', (t) => { return new Promise((resolve, reject) => { receiveObjectOnlyFromJs({ diff --git a/examples/napi/index.d.ts b/examples/napi/index.d.ts index 3f63dfb5..ce0d13ad 100644 --- a/examples/napi/index.d.ts +++ b/examples/napi/index.d.ts @@ -201,6 +201,8 @@ export function tsfnAsyncCall(func: (...args: any[]) => any): Promise export function acceptThreadsafeFunction(func: (err: Error | null, value: number) => any): void export function acceptThreadsafeFunctionFatal(func: (value: number) => any): void export function acceptThreadsafeFunctionTupleArgs(func: (err: Error | null, arg0: number, arg1: boolean, arg2: string) => any): void +export function tsfnReturnPromise(func: (err: Error | null, value: number) => any): Promise +export function tsfnReturnPromiseTimeout(func: (err: Error | null, value: number) => any): Promise export function getBuffer(): Buffer export function appendBuffer(buf: Buffer): Buffer export function getEmptyBuffer(): Buffer diff --git a/examples/napi/src/threadsafe_function.rs b/examples/napi/src/threadsafe_function.rs index a6bfea0f..a8573ec1 100644 --- a/examples/napi/src/threadsafe_function.rs +++ b/examples/napi/src/threadsafe_function.rs @@ -126,3 +126,24 @@ pub fn accept_threadsafe_function_tuple_args(func: ThreadsafeFunction<(u32, bool ); }); } + +#[napi] +pub async fn tsfn_return_promise(func: ThreadsafeFunction) -> Result { + let val = func.call_async::>(Ok(1)).await?.await?; + Ok(val + 2) +} + +#[napi] +pub async fn tsfn_return_promise_timeout(func: ThreadsafeFunction) -> Result { + use tokio::time::{self, Duration}; + let promise = func.call_async::>(Ok(1)).await?; + let sleep = time::sleep(Duration::from_millis(200)); + tokio::select! { + _ = sleep => { + return Err(Error::new(Status::GenericFailure, "Timeout".to_owned())); + } + value = promise => { + return Ok(value? + 2); + } + } +}