From 88773a7a8edd9b57eac597ee05f4926dc7686e16 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Mon, 10 Apr 2023 17:02:13 +0800 Subject: [PATCH] fix(napi): re-throw error in ThreadsafeFunction callback if we could --- crates/napi/src/error.rs | 5 +- crates/napi/src/threadsafe_function.rs | 73 ++++++++++++------ .../__snapshots__/typegen.spec.ts.md | 2 + .../__snapshots__/typegen.spec.ts.snap | Bin 3952 -> 3966 bytes examples/napi/__tests__/values.spec.ts | 16 +++- examples/napi/index.d.ts | 2 + examples/napi/src/threadsafe_function.rs | 5 ++ 7 files changed, 76 insertions(+), 27 deletions(-) diff --git a/crates/napi/src/error.rs b/crates/napi/src/error.rs index b716cd86..5f701f6a 100644 --- a/crates/napi/src/error.rs +++ b/crates/napi/src/error.rs @@ -81,7 +81,10 @@ impl From for Error { let mut result = std::ptr::null_mut(); let status = unsafe { sys::napi_create_reference(value.0.env, value.0.value, 0, &mut result) }; if status != sys::Status::napi_ok { - return Error::new(Status::from(status), "".to_owned()); + return Error::new( + Status::from(status), + "Create Error reference failed".to_owned(), + ); } Self { status: Status::GenericFailure, diff --git a/crates/napi/src/threadsafe_function.rs b/crates/napi/src/threadsafe_function.rs index 65b8c4c9..85421f35 100644 --- a/crates/napi/src/threadsafe_function.rs +++ b/crates/napi/src/threadsafe_function.rs @@ -178,7 +178,7 @@ enum ThreadsafeFunctionCallVariant { struct ThreadsafeFunctionCallJsBackData { data: T, call_variant: ThreadsafeFunctionCallVariant, - callback: Box Result<()>>, + callback: Box) -> Result<()>>, } /// Communicate with the addon's main thread by invoking a JavaScript function from other threads. @@ -434,7 +434,7 @@ impl ThreadsafeFunction { ThreadsafeFunctionCallJsBackData { data, call_variant: ThreadsafeFunctionCallVariant::Direct, - callback: Box::new(|_d: JsUnknown| Ok(())), + callback: Box::new(|_d: Result| Ok(())), } }))) .cast(), @@ -463,8 +463,8 @@ impl ThreadsafeFunction { ThreadsafeFunctionCallJsBackData { data, call_variant: ThreadsafeFunctionCallVariant::WithCallback, - callback: Box::new(move |d: JsUnknown| { - D::from_napi_value(d.0.env, d.0.value).and_then(cb) + callback: Box::new(move |d: Result| { + d.and_then(|d| D::from_napi_value(d.0.env, d.0.value).and_then(cb)) }), } }))) @@ -478,7 +478,7 @@ impl ThreadsafeFunction { #[cfg(feature = "tokio_rt")] pub async fn call_async(&self, value: Result) -> Result { - let (sender, receiver) = tokio::sync::oneshot::channel::(); + let (sender, receiver) = tokio::sync::oneshot::channel::>(); self.handle.with_read_aborted(|aborted| { if aborted { @@ -492,12 +492,12 @@ impl ThreadsafeFunction { ThreadsafeFunctionCallJsBackData { data, call_variant: ThreadsafeFunctionCallVariant::WithCallback, - callback: Box::new(move |d: JsUnknown| { - D::from_napi_value(d.0.env, d.0.value).and_then(move |d| { - sender.send(d).map_err(|_| { + callback: Box::new(move |d: Result| { + sender + .send(d.and_then(|d| D::from_napi_value(d.0.env, d.0.value))) + .map_err(|_| { crate::Error::from_reason("Failed to send return value to tokio sender") }) - }) }), } }))) @@ -508,7 +508,13 @@ impl ThreadsafeFunction { })?; receiver .await - .map_err(|err| crate::Error::new(Status::GenericFailure, format!("{}", err))) + .map_err(|_| { + crate::Error::new( + Status::GenericFailure, + "Receive value from threadsafe function sender failed", + ) + }) + .and_then(|ret| ret) } } @@ -527,7 +533,7 @@ impl ThreadsafeFunction { Box::into_raw(Box::new(ThreadsafeFunctionCallJsBackData { data: value, call_variant: ThreadsafeFunctionCallVariant::Direct, - callback: Box::new(|_d: JsUnknown| Ok(())), + callback: Box::new(|_d: Result| Ok(())), })) .cast(), mode.into(), @@ -554,8 +560,8 @@ impl ThreadsafeFunction { Box::into_raw(Box::new(ThreadsafeFunctionCallJsBackData { data: value, call_variant: ThreadsafeFunctionCallVariant::WithCallback, - callback: Box::new(move |d: JsUnknown| { - D::from_napi_value(d.0.env, d.0.value).and_then(cb) + callback: Box::new(move |d: Result| { + d.and_then(|d| D::from_napi_value(d.0.env, d.0.value).and_then(cb)) }), })) .cast(), @@ -581,10 +587,12 @@ impl ThreadsafeFunction { Box::into_raw(Box::new(ThreadsafeFunctionCallJsBackData { data: value, call_variant: ThreadsafeFunctionCallVariant::WithCallback, - callback: Box::new(move |d: JsUnknown| { - D::from_napi_value(d.0.env, d.0.value).and_then(move |d| { - sender.send(d).map_err(|_| { - crate::Error::from_reason("Failed to send return value to tokio sender") + callback: Box::new(move |d: Result| { + d.and_then(|d| { + D::from_napi_value(d.0.env, d.0.value).and_then(move |d| { + sender.send(d).map_err(|_| { + crate::Error::from_reason("Failed to send return value to tokio sender") + }) }) }) }), @@ -677,7 +685,7 @@ unsafe extern "C" fn call_js_cb( values.collect() }; let mut return_value = ptr::null_mut(); - let status = match args { + let mut status = match args { Ok(args) => unsafe { sys::napi_call_function( raw_env, @@ -705,11 +713,26 @@ unsafe extern "C" fn call_js_cb( }, }; if let ThreadsafeFunctionCallVariant::WithCallback = call_variant { - if let Err(err) = callback(JsUnknown(crate::Value { - env: raw_env, - value: return_value, - value_type: crate::ValueType::Unknown, - })) { + // throw Error in JavaScript callback + let callback_arg = if status == sys::Status::napi_pending_exception { + let mut exception = ptr::null_mut(); + status = unsafe { sys::napi_get_and_clear_last_exception(raw_env, &mut exception) }; + Err( + JsUnknown(crate::Value { + env: raw_env, + value: exception, + value_type: crate::ValueType::Unknown, + }) + .into(), + ) + } else { + Ok(JsUnknown(crate::Value { + env: raw_env, + value: return_value, + value_type: crate::ValueType::Unknown, + })) + }; + if let Err(err) = callback(callback_arg) { let message = format!( "Failed to convert return value in ThreadsafeFunction callback into Rust value: {}", err @@ -717,7 +740,7 @@ unsafe extern "C" fn call_js_cb( let message_length = message.len(); unsafe { sys::napi_fatal_error( - "threadsafe_function.rs:642\0".as_ptr().cast(), + "threadsafe_function.rs:720\0".as_ptr().cast(), 26, CString::new(message).unwrap().into_raw(), message_length, @@ -769,7 +792,7 @@ unsafe extern "C" fn call_js_cb( }, sys::Status::napi_ok, ); - let error_msg = "Call JavaScript callback failed in thread safe function"; + let error_msg = "Call JavaScript callback failed in threadsafe function"; let mut error_msg_value = ptr::null_mut(); assert_eq!( unsafe { diff --git a/examples/napi/__tests__/__snapshots__/typegen.spec.ts.md b/examples/napi/__tests__/__snapshots__/typegen.spec.ts.md index 499a98b4..f7cfbfd9 100644 --- a/examples/napi/__tests__/__snapshots__/typegen.spec.ts.md +++ b/examples/napi/__tests__/__snapshots__/typegen.spec.ts.md @@ -518,6 +518,8 @@ Generated by [AVA](https://avajs.dev). ␊ export function tsfnReturnPromiseTimeout(func: (err: Error | null, value: number) => any): Promise␊ ␊ + export function tsfnThrowFromJs(tsfn: (err: Error | null, value: number) => any): Promise␊ + ␊ export function tsRename(a: { foo: number }): string[]␊ ␊ export interface TsTypeChanged {␊ diff --git a/examples/napi/__tests__/__snapshots__/typegen.spec.ts.snap b/examples/napi/__tests__/__snapshots__/typegen.spec.ts.snap index 27a610deccc883cea3475db33eb48f344c7f87dc..5ac96f15313bb5a4af445e000cd1cf384300ea41 100644 GIT binary patch literal 3966 zcmV-^4}tJORzVY`1ySj=8`|Kf?_hcActGW`wvb=mzgT#$SBhN`31m8b0c zoDy-dnD9h>e)r|jG~W-&7`m?RT!8}j?~wcCyNIVkW_cue_8s9%$#_bpJR{%f-tR#8 z_c_a`AYqcg>}e7UA~~5*vdkw5i&jt!mvfq(GeLxWzGD5;x^Jdx}H*cHxwv7o8c{ir6aAE68Wfsz0I>z{vD{Wy{H zIS&`CqM$o_JU>s z8d0ISW-^oDjAh|mL8!k?w}!| zZlI>;%Uf8Fom@!1*l{cZ$hTRBQ{iVc%UDeJ)0myHI1iJpY<83$C<6#mp27BxFYJ=7 zSo)Zz*n64UFIQIN)g7OSqZ%*hG=yDq^iYQH+6FZB_NVz`LbIA3yOb*x!hvU4UaY|$ z&vqHM)OfDMc0M6S@*<(5IR(4CG$xzn2~}$ipHJZTYbGQ}Fai{ETK>_<;otO%t&c2( z8LFzG9vad3IFxf7-7HRZ;8#Km*wli2!bO|(fR!sbEF}F1T}k2}M%ZygZ6LG(-?p~x zqtRf8=c#n&raBLSZcALG5q>z%_yS<}sb+hI8;7TNwBTR}Ga{*w3eJ_5#u*{R*CBU1 z2~X+z_7|bB2H^r6y$DVz7`CRGKzlH(vH(NA1XoA{^oQ^H5-Qqj<%#9wGl7EiwQlX zG=4rL6V4MFrqDZ4t2HEVARu_Mm1W_D$08jiU%(ohg)WuUqf`m?HUuEw6<(p%PR+sp zhWh2HwnzTNo_Hm$Fg|@Imw^1KEDy9o2KV55{i6R|0beopW(8jwI@W-x9U&E5)l?sJ zK^XzR)x;?^eII@`W>A5=F09iYX^%n7$9|xVX`HiI&Mg+KkU3?uxopRJ@O(FeoVtKL zHc%T&{G4Tk(K!6+p=gC28l#q->=s55UCQwsb6^op z>7MxzAjOabkcte+Zk9pVXVLOabFRN>mME#jD+;Z zW3Xn}B%aUao}ket8)fG+k2Fk84zLa}KIZ{!kF&^Dfc1oa=Hnz6YY!g=%Lc=>MV!z$ zkEnMQ+60svv8@(<5@ucNJYh57*wO~28_J;c&w>5X#<&Ms?60jq39d)^_)$M0gb{BZ zKiY!u5=|wTFh?0{98#GMWGc1{x+6vAYy4qG7vJ4tXR66J9%DX$}z_)4PDZ z1O2})#xXx&Ndn6 zq=yiUK=|iKuyLnP#2L-xn~imag?1ef9Zgq}A-AHH&S19LY+o08+K#wO*MkO{)LtG0 z0#Zo_f@_xwJ&IXsgDJRZDjg~orzXx;0(!)aAv2Ekq75nw*2W0pk&>%XI*8z_&Y&?8bqj6Zj=z5tCU*f?ZL&R-QXO;O}SFVrgq zAf%VM*x|9tp(+qmx();kVY8OZAPBS|etB*ho&jP7J%?(sTcD?Ud!tvUzFIGK_qN`= z9)G)YG#a;qR!lQGEyEr3^<_)>d({N9T3CzjfIzBp^bM=EQ}|Mr^P`2DUJ;LaMZhmG z<6c4AKz)=IuF6O)Wdv<5tJSe+xfrzQV^_F`EfBX2|L$~vvcs$*VO>+Gl{lQltWAQ3 ztD~%HCAG!s4ElE$OKD2Z%`t#r%=b6e0|11|f$LcBDVsoKA4M!k+p~_6Iz6EgWoKYh zI-x+HR>>_^2sMdC>sA0XnFzQe_{Up@^VkhibR6MiRGonY@T;}D)<}g@qUJ}NKk8T- z5c4wOlTaZkmRR|e%v6V$_5czhbX`CUOUkqOsdeGMkOEd# z9^wXwyOgpx?m_pqy|Kr(Q`}qFBhTG}TC7;#mFgp^+@MhR%+@&q;%}@A?M}!Plp+uP zZc;rduc^OdrDKqH@^uka_L5eeFqRL-RFP9?*&%*cTCH5 z0y+b>dkGJnFt{ZIfI)THtbxwn5=mPmr*lx)rp9f1ofnIJXsxeobv>_K1$3vH-jbJZ z!^`q(3$E%iR%fxX7kQoPWo?w(kgE&5E$Vygvbr#LUB6K){eJ2|x&ZhqC+vUhQA9;4 zmq*3>j&k6sO~MO@;?k`ARaL^~)Hyns zh8Qoh`ax7)50pfXXYt*3XyKHA0z%Dws!|;zR%597=5WOntC=*77!}VO9vn{JGevav zqMy!lOqXHGqIPGrOmYF)lez9uHU16{voP z%-F~jGDvkNxJlmK6*PY(h!>4t2u@fh}U zdEIJtmZzf#=)nlp{~Q+c;GxMQASqLY!~GOM@*tCR9YI+RmnVPe38z#F~eBv#0NT{5*~O zBl8+)N8bh6Yw{c3BsImVyZi$z&_XxZCo_$$$KyCB6hRgn$e z!1Zo`MGG9x0~II?Fr%CB723#sX^`JM%D|87#%xqsgaeVLa9Dqw-ae`R)NY<%RGf zR^Kq-WUq@+mUUZQ1s2Y*?+HS|rX5EElV5V0@~- zHGwXCmY?w?fgSa|Q(GZ|%~aJ^QIAyPItz#c0BB!ljO&Q|_;LRDk-Cnu3sgWs4Ra7{ z#S#G&c=U^JkFa%o^40S)&&#;+Rj(#jeqA<-^8Z)Z(C;`^70QOhs^=?9+5Gbunp^*= z7tLAx8JM~%%75ecn9U$4bY5=|^~cvLQTW#D{g;OayN6?E>NpZ8y|(_OoW859yC(Rm YtYt0yYxwQsM;(jzKjVOsHzYd%0MDEC<^#r-+)a~L=!mP*bahp5GSx5JL_zGfoIa} zA{%Tp5@&|7Mw}HXJ6^AW?i=)X{}j7V*hlC&yhxNpNp?2PRie&0JR}dlb0Pi9bdvDs zO#J*Sl}sk|eVPDR2}c|T?%oFo(* zS61l9CC?;@l28b;_fgU;4U^-^2T3ZcF6u7rS?veKqPle3#Nb>AG;Y-PQN~Sy`?{({Y5dK5XGAc-z zBrtlK#DYjpCX_7mNy4I46vO45W*1CQBEuPBizSezBoxFEk92>13p2+8r*-gd1&3Z4 z_;$(`VWI{HYLc&68WTPxp>76hXUJ#pai~ZP^?1$%fj^*;+q-6S8iJB@m89W0hKAH; zM8Y;X^0j`rw2qScb&)5M-3PnEu`d=hmAW0(g!vjiB zN3X}ZwzgIoHfZ{s&(y#N?(FN{zM>f=A^a750gBAS3?`6fpd~D1@d{%DLro(pG?$!* zl9>Gm)-(6jQ`Q7l&oh#jNT?{3oGSAXuw8dRCB-9Pm^n7^( z^Rb%?$rrnhMF9B@%Wx?CoMst|=|LK^a~9`evYpM2(?ewdLCP~&-tncKvQ^TZ=x9#CF0YKqW_d!@T*K#6cz(r%1PMlfLQcy+8ae!1Ua|F_g)m)JHPl@r z8lQx6j=h`7sdoHQXaQSVkWaX1lOC{gCWnQjpP(s8{M`uKkEnHoHsD*^{+KPjTUJ_l_1E3}Hqj6;i>u($Y91g!nq-?j+$E-Prjm z6xJYIfTNee83n`ER1-I^Xk7MDNGSv`5-!5c9DgX=4}{ES7VN1*>FE*=#P`u^v3%?I5QvV4qEtfVU$R zJgzZ`=)tphIP?Sa^2Dlx8-3L%^R(BYfb5OmAl#UE1YsaRj)48-AT`SNb-1Z#G|$jJ z9ZXGOBeeL|8u4m{cJPVn*HwE#B`}QCzbY_ExX3Z$?)S?t#jRcWOK}QOprnOubZmX3 zUGj#|m}Rb`%9Ol7`A99hA?S#uAHs2-L5|gdzf2rJCn1D{$J5|EOmd3p%vs7W(t!(? z=;3zN(vwS`XCy`GL@M!rf#~*MzgB|PlwO>MOBEwHdgvX07>cu9p6KAO(8qY$N}CQL zF)R)c?FsXcVHzqh{nn98Yak3IJ<=hEpE!8wnBNe-aQAC$=JjWbZgj2e2J_JZH zBmtx%L$a4;5cb(Qt9DJco`Orev<7wK4)!79NNg{kN8~tPCKPfj(V={p%^s8p3~%cN z7^v5c!oqsBh~pqEMNJ&1VWzuSS6nwjp_OdEI@lhmb=e00{a_UycHm*%zkmq~1&v2) z%@q^$rzBvQ=UG*kHL+V+S9}W4c;=XHIK$Tt<5(_f8oO~{5sK>v4+};@`r`>$Gb|F% zXLC=`Xp@by@u^1|rY472hZtY*0G7v@WGldWLO=6Kl8f~R4}xWb;l?6PX`DyYI}2?B z%7xfb6F&*Fu6drY8E|ZCgVHa`p!DOwKD06JgBAzt8;^sVQ9gRuPY7Ye>qif_A-qIW z2`0=@#+Xl&)4%GC$2kA10xGGRPcnmwg zq**Pv{pbLJ5*XW%j1@2Ld(gWo!mrRPp3XcV>h!j@wg&bKt{yPu?YmxdLC()Ca0&B8 z)p!rmT_Y!mChK;pbYK}x00ILIlZx1#iWbqZ+$o1Vl7b1ZAF?!u2#)Dhz}A8OuZwZa z4_T5xI8URLt&wv2Si8HRAk7lXr(il9X31Ag`T!8?$Y(3fhM(w2Z4Z8(t+UG zp+b*hmfBznPMS)Gipi;ovz34zabw7gV?Aku%7nEsf_S9ls+SHT_^LA~Op5TV0=aa% zdQ~56lF=atg#dmmVR@B`S2lv$E`8;ysu;{v+E&)+h!6ghSe+@VoI;0YliVB{K*DZxFvcGcTSFVg)@%YO-6Pr+Rv$XQw_}&-eDXU%wiEzk56y zw}VzpGdeB99rX3(oAUSS70ha3ExH2&smjqet=3NAOIglO7HWD$JnR(#PhiHqf_8xV zI4fM0ky^?K+FVwvW6^RkXwloQa1YxcZU_GEc7U?OtRi7uQ>c|VoW!h6f`+T(tZF5- z$?6RH_ZCZOO3uwbfMCoIHa7wQgvy5NnC~f@Kx7|9EJ)k4j*>b(r4eQ4U{gAwK%Z90 zEmjFNiAC#D05q8hxFh(-Q-+J!4N`O*;bc^ufdufYxw=tLg;S#Dq0Jw4Obv*6k?={V zkQ7U-d`f1j!ApAx2@$$3AciUBS^UI0`c~J6)gKBN9K))@mM*dV?K}MKXMk8#6DEE= znzA%by}(l=bxx|=g@aTzey8HzrTYmiBV>s$1y5BCX<5EQKwrLe>_dZEvnq0Q0L6nIRfHuZV2s8$P|7@uD zgt2iM~Ymv9T9o2$~57&@o{)T4jjoP&EpyuBl67SsE<5_jH_YhS>CMW}t2(B{6fh*nt2=YS&HoxYZ`4Dpz{C=s;P5G&k{ z^{8tt<*Scgg~p9~Ir`RTLr)#u>o40zqGT)YQsfUhsc_b^o|AN#g%S9i;oxa1^~E$R z(UaVGuW0pmaG1-_u5{gH>fq(7>NWf7vlZgPl*o185_qdQ8mih$I`+C)iW1dcKZ8>!jt;^dc43}~=hp0K;#f!?!i=XlyV#x7GMUAj}$CmAFz zy6$(^KHtUM=M@iQG+bymkE;0;fE@4Q-Bk~Fy{-|{4>DOR23A7X(vntWDy3jIHmP&|~p_)P*Wdpiw(DSA#9q?o2EvcVI$-UYB| zf!%qa0)?R*<=E_r-X|`qz%rq8qWTFJKzfhU8=Z{Vg7RGcfBHJ5>PA|KQHFC1MnwyH zvpMy*5>&12cQx$!!wo9_mc?47*0nP)TP2ESY-)wJ;O zX%}v+lv@PF1&cls6;2e>t8BB>Sh=IrH@{`D$%f@*b#;vQ=(i`(gm>mMo+PlMzDs1Q zM6j7EsVeG`db!C2Vh4bzH|gUh;y!+yKYFOnitGdxP*B4h#9FaL00ka>9or*p1Mg$? zT$}SUZhq6N$+cgX&7wB{4Zi40ma0T#<5ku2jiqeAMevjD< zA~xr01yMg+Rtb@}Umd(SI@~)NJ445wKh;h)3rA3f}ty#E1D KFN5d7IsgDKxQVy` diff --git a/examples/napi/__tests__/values.spec.ts b/examples/napi/__tests__/values.spec.ts index b0dbf3ae..f4f63705 100644 --- a/examples/napi/__tests__/values.spec.ts +++ b/examples/napi/__tests__/values.spec.ts @@ -55,6 +55,7 @@ import { threadsafeFunctionClosureCapture, tsfnCallWithCallback, tsfnAsyncCall, + tsfnThrowFromJs, asyncPlus100, getGlobal, getUndefined, @@ -843,6 +844,19 @@ Napi4Test('async call ThreadsafeFunction', async (t) => { ) }) +test('Throw from ThreadsafeFunction JavaScript callback', async (t) => { + const errMsg = 'ThrowFromJavaScriptRawCallback' + await t.throwsAsync( + () => + tsfnThrowFromJs(() => { + throw new Error(errMsg) + }), + { + message: errMsg, + }, + ) +}) + Napi4Test('accept ThreadsafeFunction', async (t) => { await new Promise((resolve, reject) => { acceptThreadsafeFunction((err, value) => { @@ -923,7 +937,7 @@ Napi4Test('object only from js', (t) => { }) }) -Napi4Test('promise in either', async (t) => { +test('promise in either', async (t) => { t.is(await promiseInEither(1), false) t.is(await promiseInEither(20), true) t.is(await promiseInEither(Promise.resolve(1)), false) diff --git a/examples/napi/index.d.ts b/examples/napi/index.d.ts index 9dc42e10..5c86600a 100644 --- a/examples/napi/index.d.ts +++ b/examples/napi/index.d.ts @@ -508,6 +508,8 @@ export function tsfnReturnPromise(func: (err: Error | null, value: number) => an export function tsfnReturnPromiseTimeout(func: (err: Error | null, value: number) => any): Promise +export function tsfnThrowFromJs(tsfn: (err: Error | null, value: number) => any): Promise + export function tsRename(a: { foo: number }): string[] export interface TsTypeChanged { diff --git a/examples/napi/src/threadsafe_function.rs b/examples/napi/src/threadsafe_function.rs index 7e52c489..0bbeb2c6 100644 --- a/examples/napi/src/threadsafe_function.rs +++ b/examples/napi/src/threadsafe_function.rs @@ -160,3 +160,8 @@ pub async fn tsfn_return_promise_timeout(func: ThreadsafeFunction) -> Resul } } } + +#[napi] +pub async fn tsfn_throw_from_js(tsfn: ThreadsafeFunction) -> napi::Result { + tsfn.call_async::>(Ok(42)).await?.await +}