node v9 rc breaks test suite of worker-farm

Created on 19 Oct 2017 · 9Comments · Source: nodejs/node

Version: v9.0.0-pre (https://github.com/nodejs/node/commit/f2f391e575fc8072d10e1ad1601ef3f67f13a4db)
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 https://github.com/nodejs/node/commit/f2b01cba7b48c2410a692fe1cb29b2e88a6c5cab makes both worker-farm's and jest's test suites pass.

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

child_process

Source

SimenB

Most helpful comment

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');

bnoordhuis on 19 Oct 2017

👍2

All 9 comments

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

addaleax on 19 Oct 2017

It looks like worker farm is wrapping child.send() in a try...catch. Prior to f2b01cba7b48c2410a692fe1cb29b2e88a6c5cab, 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.

cjihrig on 19 Oct 2017

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');

bnoordhuis on 19 Oct 2017

👍2

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

SimenB on 19 Oct 2017

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

Trott on 22 Oct 2017

👍1

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 https://github.com/nodejs/citgm/issues/415, though

SimenB on 22 Oct 2017

👍1

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

SimenB on 29 Oct 2017

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

cc/ @rvagg