Repository navigation
weak-napi broken in Node 14.7.0 (working in 14.6.0) #34636
Description
Activity
- added a commit that references this issue
on Aug 5, 2020 I haven't bisected, but #34386 seems like the obvious candidate looking at the changelog
I’ve confirmed that reverting it fixes the
weak-napitest suite, yes.(
weak-napicould probably be added to CITGM)@nodejs/citgm I’d be 👍 on this.
That being said, I don’t think the
weak-napibreakage qualifies as a bug in Node.js or N-API. Theweak-napitests are/were simply expecting stricter relative timing guarantees than what Node.js provides, and fixing the tests up (node-ffi-napi/weak-napi@fdafbde) seems like the right thing to do here, at least with my N-API andweak-napimaintainer hat on.Reacted by Simen Bekkhus and YonyHmm, interesting! We'll have to do something similar in Jest then, as it has 2 failing tests on 14.7.
Is 4
setImmediatecalls a safe number, or should we use more?@SimenB There isn’t really any strong guarantee, partly because V8 itself also doesn’t give us any strong guarantees. 2 ×
setImmediateis currently enough, so you can pick 10 or so if you want to be reasonably safe (that is, as long assetImmediate()works at all).Reacted by Simen BekkhusCool, thanks! Using 4 fixed both failing tests, but I can do 10 just to be safe
- added a commit that references this issue
on Aug 5, 2020 Yeah, unless this is causing any trouble besides the timing difference, I think there’s nothing actionable here. Let us know if we should reopen :)
--detect-leaksin Jest is broken, but I just landed a fix on master and will make a release soon ish. Not a huge issue I believe, certainly not worth reverting in Node
Darwin Simens-MacBook-Pro.local 18.7.0 Darwin Kernel Version 18.7.0: Thu Jun 18 20:50:10 PDT 2020; root:xnu-4903.278.43~1/RELEASE_X86_64 x86_64What steps will reproduce the bug?
Clone https://github.com/node-ffi-napi/weak-napi, run install and run the tests. They fail on node 14.7.0, but pass on node 14.6.0.
I discovered this via Jest's tests failing (which use
weak-napi). I assume Jest's--detect-leaksfeature is broken for this version of Node.(
weak-napicould probably be added to CITGM)How often does it reproduce? Is there a required condition?
It always fails
What is the expected behavior?
Tests should pass 🙂
What do you see instead?
Tests fail 🙁
Additional information
I haven't bisected, but #34386 seems like the obvious candidate looking at the changelog
/cc @addaleax