weak-napi broken in Node 14.7.0 (working in 14.6.0)

[SimenB]

• Version: 14.7.0
• Platform: 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_64
• Subsystem: N-API (I think)

What 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-leaks feature is broken for this version of Node.

(weak-napi could 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

[addaleax]

I haven't bisected, but #34386 seems like the obvious candidate looking at the changelog

I’ve confirmed that reverting it fixes the weak-napi test suite, yes.

(weak-napi could probably be added to CITGM)

@nodejs/citgm I’d be 👍 on this.

---

That being said, I don’t think the weak-napi breakage qualifies as a bug in Node.js or N-API. The weak-napi tests 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 and weak-napi maintainer hat on.

[SimenB]

Hmm, interesting! We'll have to do something similar in Jest then, as it has 2 failing tests on 14.7.

https://github.com/facebook/jest/blob/96258265991450be8298264e7521d163a5295969/packages/jest-leak-detector/src/index.ts#L49-L55

Is 4 setImmediate calls a safe number, or should we use more?

[addaleax]

@SimenB There isn’t really any strong guarantee, partly because V8 itself also doesn’t give us any strong guarantees. 2 × setImmediate is currently enough, so you can pick 10 or so if you want to be reasonably safe (that is, as long as setImmediate() works at all).

[SimenB]

Cool, thanks! Using 4 fixed both failing tests, but I can do 10 just to be safe

[SimenB]

@addaleax close this then? CI is passing with the added setImmediates, so if this is expected behavior there's nothing to do here I believe

[addaleax]

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 :)

[SimenB]

--detect-leaks in 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