Skip to content

Use advanced serialization (when available) for worker communication #2560

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ exports.run = async () => { // eslint-disable-line complexity
reporter.startRun(plan);

if (process.env.AVA_EMIT_RUN_STATUS_OVER_IPC === 'I\'ll find a payphone baby / Take some time to talk to you') {
if (process.versions.node >= '12.16.0') {
if (process.versions.node >= '12.17.0') {
plan.status.on('stateChange', evt => {
process.send(evt);
});
Expand Down
20 changes: 14 additions & 6 deletions lib/fork.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const childProcess = require('child_process');
const path = require('path');
const fs = require('fs');
const Emittery = require('emittery');
const {controlFlow} = require('./ipc-flow-control');

if (fs.realpathSync(__filename) !== __filename) {
console.warn('WARNING: `npm link ava` and the `--preserve-symlink` flag are incompatible. We have detected that AVA is linked via `npm link`, and that you are using either an early version of Node 6, or the `--preserve-symlink` flag. This breaks AVA. You should upgrade to Node 6.2.0+, avoid the `--preserve-symlink` flag, or avoid using `npm link ava`.');
Expand All @@ -14,6 +15,12 @@ const AVA_PATH = path.resolve(__dirname, '..');

const workerPath = require.resolve('./worker/subprocess');

const useAdvanced = process.versions.node >= '12.17.0';
// FIXME: Fix this in api.js or cli.js.
const serializeOptions = useAdvanced ?
options => JSON.parse(JSON.stringify(options)) : // Use JSON serialization to remove non-clonable values.
options => options;

module.exports = (file, options, execArgv = process.execArgv) => {
let finished = false;

Expand All @@ -34,7 +41,8 @@ module.exports = (file, options, execArgv = process.execArgv) => {
cwd: options.projectDir,
silent: true,
env: {NODE_ENV: 'test', ...process.env, ...options.environmentVariables, AVA_PATH},
execArgv
execArgv,
serialization: useAdvanced ? 'advanced' : 'json'
});

subprocess.stdout.on('data', chunk => {
Expand All @@ -45,12 +53,12 @@ module.exports = (file, options, execArgv = process.execArgv) => {
emitStateChange({type: 'worker-stderr', chunk});
});

const bufferedSend = controlFlow(subprocess);

let forcedExit = false;
const send = evt => {
if (subprocess.connected && !finished && !forcedExit) {
subprocess.send({ava: evt}, () => {
// Disregard errors.
});
if (!finished && !forcedExit) {
bufferedSend({ava: evt});
}
};

Expand All @@ -66,7 +74,7 @@ module.exports = (file, options, execArgv = process.execArgv) => {
}

if (message.ava.type === 'ready-for-options') {
send({type: 'options', options});
send({type: 'options', options: serializeOptions(options)});
return;
}

Expand Down
40 changes: 40 additions & 0 deletions lib/ipc-flow-control.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// Manage how quickly messages are delivered to the channel. In theory, we
// should be able to call `send()` until it returns `false` but this leads to
// crashes with advanced serialization, see
// <https://github.com/nodejs/node/issues/34797>.
//
// Even if that's fixed (and the Node.js versions with the fixes are the
// minimally supported versions) we need flow control based on `send()`'s return
// value.

function controlFlow(channel) {
let sending = false;

const buffer = [];
const deliverNext = () => {
if (!channel.connected) {
buffer.length = 0;
}

if (buffer.length === 0) {
sending = false;
return;
}

channel.send(buffer.shift(), deliverNext);
};

return message => {
if (!channel.connected) {
return;
}

buffer.push(message);
if (!sending) {
sending = true;
setImmediate(deliverNext);
}
};
}

exports.controlFlow = controlFlow;
6 changes: 3 additions & 3 deletions lib/worker/ipc.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';
const Emittery = require('emittery');
const {controlFlow} = require('../ipc-flow-control');

const emitter = new Emittery();
process.on('message', message => {
Expand All @@ -25,10 +26,9 @@ process.on('message', message => {
exports.options = emitter.once('options');
exports.peerFailed = emitter.once('peerFailed');

const bufferedSend = controlFlow(process);
function send(evt) {
if (process.connected) {
process.send({ava: evt});
}
bufferedSend({ava: evt});
}

exports.send = send;
Expand Down
2 changes: 1 addition & 1 deletion test-tap/fixture/report/regular/uncaught-exception.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const test = require('../../../..');

test('passes', t => {
setTimeout(() => {
setImmediate(() => {
throw new Error('Can’t catch me');
});
t.pass();
Expand Down
4 changes: 2 additions & 2 deletions test-tap/reporters/mini.regular.v10.log
Original file line number Diff line number Diff line change
Expand Up @@ -515,13 +515,13 @@

uncaught-exception.js:5

4: setTimeout(() => {
4: setImmediate(() => {
 5: throw new Error('Can’t catch me');
6: });

Error: Can’t catch me

› Timeout.setTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
› Immediate.setImmediate (test-tap/fixture/report/regular/uncaught-exception.js:5:9)



Expand Down
7 changes: 3 additions & 4 deletions test-tap/reporters/mini.regular.v12.log
Original file line number Diff line number Diff line change
Expand Up @@ -497,15 +497,14 @@

uncaught-exception.js:5

4: setTimeout(() => {
4: setImmediate(() => {
 5: throw new Error('Can’t catch me');
6: });

Error: Can’t catch me

› Timeout._onTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
› listOnTimeout (internal/timers.js:549:17)
› processTimers (internal/timers.js:492:7)
› Immediate.<anonymous> (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
› processImmediate (internal/timers.js:456:21)



Expand Down
7 changes: 3 additions & 4 deletions test-tap/reporters/mini.regular.v14.log
Original file line number Diff line number Diff line change
Expand Up @@ -497,15 +497,14 @@

uncaught-exception.js:5

4: setTimeout(() => {
4: setImmediate(() => {
 5: throw new Error('Can’t catch me');
6: });

Error: Can’t catch me

› Timeout._onTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
› listOnTimeout (internal/timers.js:551:17)
› processTimers (internal/timers.js:494:7)
› Immediate.<anonymous> (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
› processImmediate (internal/timers.js:458:21)



Expand Down
4 changes: 3 additions & 1 deletion test-tap/reporters/tap.regular.v10.log
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,9 @@ not ok 25 - Error: Can’t catch me
---
name: Error
message: Can’t catch me
at: 'Timeout.setTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)'
at: >-
Immediate.setImmediate
(test-tap/fixture/report/regular/uncaught-exception.js:5:9)
...
---tty-stream-chunk-separator
# uncaught-exception.js exited with a non-zero exit code: 1
Expand Down
9 changes: 5 additions & 4 deletions test-tap/reporters/tap.regular.v12.log
Original file line number Diff line number Diff line change
Expand Up @@ -296,10 +296,11 @@ not ok 25 - Error: Can’t catch me
---
name: Error
message: Can’t catch me
at: |-
Timeout._onTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
listOnTimeout (internal/timers.js:549:17)
processTimers (internal/timers.js:492:7)
at: >-
Immediate.<anonymous>
(test-tap/fixture/report/regular/uncaught-exception.js:5:9)

processImmediate (internal/timers.js:456:21)
...
---tty-stream-chunk-separator
# uncaught-exception.js exited with a non-zero exit code: 1
Expand Down
9 changes: 5 additions & 4 deletions test-tap/reporters/tap.regular.v14.log
Original file line number Diff line number Diff line change
Expand Up @@ -296,10 +296,11 @@ not ok 25 - Error: Can’t catch me
---
name: Error
message: Can’t catch me
at: |-
Timeout._onTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
listOnTimeout (internal/timers.js:551:17)
processTimers (internal/timers.js:494:7)
at: >-
Immediate.<anonymous>
(test-tap/fixture/report/regular/uncaught-exception.js:5:9)

processImmediate (internal/timers.js:458:21)
...
---tty-stream-chunk-separator
# uncaught-exception.js exited with a non-zero exit code: 1
Expand Down
4 changes: 2 additions & 2 deletions test-tap/reporters/verbose.regular.v10.log
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,13 @@

uncaught-exception.js:5

4: setTimeout(() => {
4: setImmediate(() => {
 5: throw new Error('Can’t catch me');
6: });

Error: Can’t catch me

› Timeout.setTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
› Immediate.setImmediate (test-tap/fixture/report/regular/uncaught-exception.js:5:9)

---tty-stream-chunk-separator
✖ uncaught-exception.js exited with a non-zero exit code: 1
Expand Down
7 changes: 3 additions & 4 deletions test-tap/reporters/verbose.regular.v12.log
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,14 @@

uncaught-exception.js:5

4: setTimeout(() => {
4: setImmediate(() => {
 5: throw new Error('Can’t catch me');
6: });

Error: Can’t catch me

› Timeout._onTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
› listOnTimeout (internal/timers.js:549:17)
› processTimers (internal/timers.js:492:7)
› Immediate.<anonymous> (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
› processImmediate (internal/timers.js:456:21)

---tty-stream-chunk-separator
✖ uncaught-exception.js exited with a non-zero exit code: 1
Expand Down
7 changes: 3 additions & 4 deletions test-tap/reporters/verbose.regular.v14.log
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,14 @@

uncaught-exception.js:5

4: setTimeout(() => {
4: setImmediate(() => {
 5: throw new Error('Can’t catch me');
6: });

Error: Can’t catch me

› Timeout._onTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
› listOnTimeout (internal/timers.js:551:17)
› processTimers (internal/timers.js:494:7)
› Immediate.<anonymous> (test-tap/fixture/report/regular/uncaught-exception.js:5:9)
› processImmediate (internal/timers.js:458:21)

---tty-stream-chunk-separator
✖ uncaught-exception.js exited with a non-zero exit code: 1
Expand Down
2 changes: 1 addition & 1 deletion test/helpers/exec.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const defaultsDeep = require('lodash/defaultsDeep');
const cliPath = path.resolve(__dirname, '../../cli.js');
const ttySimulator = path.join(__dirname, './simulate-tty.js');

const serialization = process.versions.node >= '12.16.0' ? 'advanced' : 'json';
const serialization = process.versions.node >= '12.17.0' ? 'advanced' : 'json';

const normalizePath = (root, file) => path.posix.normalize(path.relative(root, file));

Expand Down