From 88773a7a8edd9b57eac597ee05f4926dc7686e16 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Mon, 10 Apr 2023 17:02:13 +0800 Subject: [PATCH 1/3] 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 +} From 752ffea1d9445fe3a474f00c430c8d3ef4b9a252 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Mon, 10 Apr 2023 17:02:47 +0800 Subject: [PATCH 2/3] fix(napi): revert Promise changes because of the flaky test --- .github/workflows/linux-armv7.yaml | 1 + .../src/bindgen_runtime/js_values/promise.rs | 95 +++++++------------ crates/napi/src/js_values/deferred.rs | 12 ++- 3 files changed, 46 insertions(+), 62 deletions(-) diff --git a/.github/workflows/linux-armv7.yaml b/.github/workflows/linux-armv7.yaml index 173b0456..5042cd3c 100644 --- a/.github/workflows/linux-armv7.yaml +++ b/.github/workflows/linux-armv7.yaml @@ -2,6 +2,7 @@ name: Linux-armv7 env: DEBUG: 'napi:*' + RUST_BACKTRACE: 1 concurrency: group: ${{ github.workflow }}-${{ github.ref }} diff --git a/crates/napi/src/bindgen_runtime/js_values/promise.rs b/crates/napi/src/bindgen_runtime/js_values/promise.rs index 3605c5fa..09fe1fd8 100644 --- a/crates/napi/src/bindgen_runtime/js_values/promise.rs +++ b/crates/napi/src/bindgen_runtime/js_values/promise.rs @@ -1,33 +1,30 @@ -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, AtomicPtr, Ordering}, + atomic::{AtomicBool, Ordering}, Arc, }; -use std::task::{Context, Poll, Waker}; +use std::task::{Context, Poll}; -use crate::{check_status, sys, Error, JsUnknown, NapiValue, Result}; +use tokio::sync::oneshot::{channel, Receiver, Sender}; + +use crate::{check_status, sys, Error, JsUnknown, NapiValue, Result, Status}; use super::{FromNapiValue, TypeName, ValidateNapiValue}; -struct PromiseInner { - value: AtomicPtr>, - waker: AtomicPtr, - aborted: AtomicBool, +pub struct Promise { + value: Pin>>>, + aborted: Arc, } -impl Drop for PromiseInner { +impl Drop for Promise { fn drop(&mut self) { self.aborted.store(true, Ordering::SeqCst); } } -pub struct Promise { - inner: Arc>, -} - impl TypeName for Promise { fn type_name() -> &'static str { "Promise" @@ -103,13 +100,9 @@ impl FromNapiValue for Promise { )?; let mut promise_after_then = ptr::null_mut(); let mut then_js_cb = ptr::null_mut(); - let promise_inner = PromiseInner { - value: AtomicPtr::new(ptr::null_mut()), - waker: AtomicPtr::new(ptr::null_mut()), - aborted: AtomicBool::new(false), - }; - let shared_inner = Arc::new(promise_inner); - let context_ptr = Arc::into_raw(shared_inner.clone()); + let (tx, rx) = channel(); + 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( @@ -117,7 +110,7 @@ impl FromNapiValue for Promise { then_c_string.as_ptr(), 4, Some(then_callback::), - context_ptr as *mut c_void, + tx_ptr.cast(), &mut then_js_cb, ) }, @@ -152,7 +145,7 @@ impl FromNapiValue for Promise { catch_c_string.as_ptr(), 5, Some(catch_callback::), - context_ptr as *mut c_void, + tx_ptr.cast(), &mut catch_js_cb, ) }, @@ -172,7 +165,8 @@ impl FromNapiValue for Promise { "Failed to call catch method" )?; Ok(Promise { - inner: shared_inner, + value: Box::pin(rx), + aborted, }) } } @@ -180,19 +174,13 @@ impl FromNapiValue for Promise { impl future::Future for Promise { type Output = Result; - fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { - if self.inner.value.load(Ordering::Relaxed).is_null() { - if self.inner.waker.load(Ordering::Acquire).is_null() { - self.inner.waker.store( - Box::into_raw(Box::new(cx.waker().clone())), - Ordering::Release, - ); - } - Poll::Pending - } else { - Poll::Ready( - unsafe { Box::from_raw(self.inner.value.load(Ordering::Relaxed)) }.map_err(Error::from), - ) + fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { + match self.value.as_mut().poll(cx) { + Poll::Pending => Poll::Pending, + Poll::Ready(v) => Poll::Ready( + v.map_err(|e| Error::new(Status::GenericFailure, format!("{}", e))) + .and_then(|v| unsafe { *Box::from_raw(v) }.map_err(Error::from)), + ), } } } @@ -218,20 +206,15 @@ unsafe extern "C" fn then_callback( get_cb_status == sys::Status::napi_ok, "Get callback info from Promise::then failed" ); - let PromiseInner { - value, - waker, - aborted, - } = &*unsafe { Arc::from_raw(data as *mut PromiseInner) }; + 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]) }); - value.store(Box::into_raw(resolve_value_t), Ordering::Relaxed); - let waker = waker.load(Ordering::Acquire); - if !waker.is_null() { - unsafe { Box::from_raw(waker) }.wake(); - } + sender + .send(Box::into_raw(resolve_value_t)) + .expect("Send Promise resolved value error"); this } @@ -258,23 +241,15 @@ unsafe extern "C" fn catch_callback( "Get callback info from Promise::catch failed" ); let rejected_value = rejected_value[0]; - let PromiseInner { - value, - waker, - aborted, - } = &*unsafe { Arc::from_raw(data as *mut PromiseInner) }; + let (sender, aborted) = + *unsafe { Box::from_raw(data as *mut (Sender<*mut Result>, Arc)) }; if aborted.load(Ordering::SeqCst) { return this; } - value.store( - Box::into_raw(Box::new(Err(Error::from(unsafe { + sender + .send(Box::into_raw(Box::new(Err(Error::from(unsafe { JsUnknown::from_raw_unchecked(env, rejected_value) - })))), - Ordering::Relaxed, - ); - let waker = waker.load(Ordering::Acquire); - if !waker.is_null() { - unsafe { Box::from_raw(waker) }.wake(); - } + }))))) + .expect("Send Promise resolved value error"); this } diff --git a/crates/napi/src/js_values/deferred.rs b/crates/napi/src/js_values/deferred.rs index 2660e283..23ac087a 100644 --- a/crates/napi/src/js_values/deferred.rs +++ b/crates/napi/src/js_values/deferred.rs @@ -115,12 +115,20 @@ extern "C" fn napi_resolve_deferred match result { Ok(res) => { let status = unsafe { sys::napi_resolve_deferred(env, deferred, res) }; - debug_assert!(status == sys::Status::napi_ok, "Resolve promise failed"); + debug_assert!( + status == sys::Status::napi_ok, + "Resolve promise failed {:?}", + crate::Status::from(status) + ); } Err(e) => { let status = unsafe { sys::napi_reject_deferred(env, deferred, JsError::from(e).into_value(env)) }; - debug_assert!(status == sys::Status::napi_ok, "Reject promise failed"); + debug_assert!( + status == sys::Status::napi_ok, + "Reject promise failed {:?}", + crate::Status::from(status) + ); } } } From 66ef64bdc780a10f6a0b9c10caf15b51ade4de52 Mon Sep 17 00:00:00 2001 From: LongYinan Date: Mon, 10 Apr 2023 17:03:18 +0800 Subject: [PATCH 3/3] style(napi): use cast() instread as --- crates/napi/src/bindgen_runtime/iterator.rs | 42 ++++++++------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/crates/napi/src/bindgen_runtime/iterator.rs b/crates/napi/src/bindgen_runtime/iterator.rs index 3f71291d..5ed75b22 100644 --- a/crates/napi/src/bindgen_runtime/iterator.rs +++ b/crates/napi/src/bindgen_runtime/iterator.rs @@ -50,12 +50,7 @@ pub fn create_iterator( check_status_or_throw!( env, unsafe { - sys::napi_get_named_property( - env, - global, - "Symbol\0".as_ptr() as *const c_char, - &mut symbol_object, - ) + sys::napi_get_named_property(env, global, "Symbol\0".as_ptr().cast(), &mut symbol_object) }, "Get global object failed", ); @@ -66,7 +61,7 @@ pub fn create_iterator( sys::napi_get_named_property( env, symbol_object, - "iterator\0".as_ptr() as *const c_char, + "iterator\0".as_ptr().cast(), &mut iterator_symbol, ) }, @@ -78,7 +73,7 @@ pub fn create_iterator( unsafe { sys::napi_create_function( env, - "Iterator\0".as_ptr() as *const c_char, + "Iterator\0".as_ptr().cast(), 8, Some(symbol_generator::), generator_ptr as *mut c_void, @@ -129,7 +124,7 @@ pub unsafe extern "C" fn symbol_generator( unsafe { sys::napi_create_function( env, - "next\0".as_ptr() as *const c_char, + "next\0".as_ptr().cast(), 4, Some(generator_next::), generator_ptr, @@ -144,7 +139,7 @@ pub unsafe extern "C" fn symbol_generator( unsafe { sys::napi_create_function( env, - "return\0".as_ptr() as *const c_char, + "return\0".as_ptr().cast(), 6, Some(generator_return::), generator_ptr, @@ -159,7 +154,7 @@ pub unsafe extern "C" fn symbol_generator( unsafe { sys::napi_create_function( env, - "throw\0".as_ptr() as *const c_char, + "throw\0".as_ptr().cast(), 5, Some(generator_throw::), generator_ptr, @@ -175,7 +170,7 @@ pub unsafe extern "C" fn symbol_generator( sys::napi_set_named_property( env, generator_object, - "next\0".as_ptr() as *const c_char, + "next\0".as_ptr().cast(), next_function, ) }, @@ -188,7 +183,7 @@ pub unsafe extern "C" fn symbol_generator( sys::napi_set_named_property( env, generator_object, - "return\0".as_ptr() as *const c_char, + "return\0".as_ptr().cast(), return_function, ) }, @@ -201,7 +196,7 @@ pub unsafe extern "C" fn symbol_generator( sys::napi_set_named_property( env, generator_object, - "throw\0".as_ptr() as *const c_char, + "throw\0".as_ptr().cast(), throw_function, ) }, @@ -216,7 +211,7 @@ pub unsafe extern "C" fn symbol_generator( ); let properties = vec![sys::napi_property_descriptor { - utf8name: GENERATOR_STATE_KEY.as_ptr() as *const c_char, + utf8name: GENERATOR_STATE_KEY.as_ptr().cast(), name: ptr::null_mut(), method: None, getter: None, @@ -264,7 +259,7 @@ extern "C" fn generator_next( sys::napi_get_named_property( env, this, - GENERATOR_STATE_KEY.as_ptr() as *const c_char, + GENERATOR_STATE_KEY.as_ptr().cast(), &mut generator_state, ) }, @@ -385,7 +380,7 @@ extern "C" fn generator_return( sys::napi_set_named_property( env, this, - GENERATOR_STATE_KEY.as_ptr() as *const c_char, + GENERATOR_STATE_KEY.as_ptr().cast(), generator_state, ) }, @@ -498,7 +493,7 @@ extern "C" fn generator_throw( sys::napi_set_named_property( env, this, - GENERATOR_STATE_KEY.as_ptr() as *const c_char, + GENERATOR_STATE_KEY.as_ptr().cast(), generator_state, ) }, @@ -530,7 +525,7 @@ extern "C" fn generator_throw( sys::napi_set_named_property( env, this, - GENERATOR_STATE_KEY.as_ptr() as *const c_char, + GENERATOR_STATE_KEY.as_ptr().cast(), generator_state, ) }, @@ -538,14 +533,7 @@ extern "C" fn generator_throw( ); check_status_or_throw!( env, - unsafe { - sys::napi_set_named_property( - env, - result, - "done\0".as_ptr() as *const c_char, - generator_state, - ) - }, + unsafe { sys::napi_set_named_property(env, result, "done\0".as_ptr().cast(), generator_state) }, "Get generator state failed" );