node v9 rc breaks test suite of worker-farm

[SimenB]

• Version: v9.0.0-pre (f2f391e)
• Platform: Darwin local.finn.no 16.6.0 Darwin Kernel Version 16.6.0: Fri Apr 14 16:21:16 PDT 2017; root:xnu-3789.60.24~6/RELEASE_X86_64 x86_64
• Subsystem: child_process

Running node from current master breaks the test suite of worker-farm, and by extension jest as it uses worker-farm for parallelisation, with the following stack trace:

Error [ERR_IPC_CHANNEL_CLOSED]: Channel closed
    at ChildProcess.target.send (internal/child_process.js:606:16)
    at Object.send (/Users/simbekkh/repos/node-worker-farm/lib/fork.js:24:17)
    at Farm.stopChild (/Users/simbekkh/repos/node-worker-farm/lib/farm.js:131:11)
    at Farm.<anonymous> (/Users/simbekkh/repos/node-worker-farm/lib/farm.js:96:10)
    at ontimeout (timers.js:478:11)
    at tryOnTimeout (timers.js:302:5)
    at Timer.listOnTimeout (timers.js:262:5)

Reverting f2b01cb makes both worker-farm's and jest's test suites pass.

See #16293
https://github.com/rvagg/node-worker-farm

[addaleax]

/cc reviewers of the linked commit: @tflanagan @cjihrig @santigimeno @bnoordhuis @Trott @imyller

[cjihrig]

It looks like worker farm is wrapping child.send() in a try...catch. Prior to f2b01cb, the synchronously emitted error would be caught.

I think worker farm could improve its error handling there. The alternative is to revert the commit in core.

[bnoordhuis]

It seems like an easy fix on worker-farm's side. Since it seems to want to suppress exceptions, it can attach a no-op 'error' event listener to the child process object.

I don't think we should revert f2b01cb. It was an exception to the rule that run-time errors are emitted asynchronously and it's good it got fixed. Reverting reintroduces runaway recursion risk:

child.on('error', (e) => child.send(e));
child.send('boom');

[SimenB]

I can confirm that the patch submitted above also fixes Jest's tests (using rc.0) 🙂

[Trott]

Aside: Would have running npm test for jest with Node.js 9.0.0-pre caught this issue? If so, should we add jest to lookup.json in CITGM?? @nodejs/citgm

[SimenB]

Would running npm test for jest with Node.js 9.0.0-pre caught this issue?

Yes.

I talked with @cpojer about this, and he is open to it (as there's a requirement maintainers are responsive).

The only issue I see with that is that the jest repo requires yarn - it's a hard dependency (it uses yarn workspaces (https://yarnpkg.com/blog/2017/08/02/introducing-workspaces/ & https://yarnpkg.com/lang/en/docs/workspaces/)). CITGM currently has a hard requirement that projects must be installable using npm.

https://github.com/nodejs/citgm/blob/d21e2a87aaa9e9f50c6175eddc54054a32c64a24/CONTRIBUTING.md#hard-requirements

There is nodejs/citgm#415, though

[SimenB]

@bnoordhuis anything we can do as consumers of worker-farm to work around this other than forking and publishing a version with your fix?

[gibfahn]

I think it's just a case of getting Rod's attention, I know he's been pretty busy lately.

cc/ @rvagg

[rvagg]

👍 sorry for the delay, all good now in 1.5.1