Use advanced serialization (when available) for worker communication

* Use advanced serialization (when available) for test worker communication Use advanced IPC on 12.17.0. This is currently our future minimal Node.js 12 version. * Buffer IPC sends to avoid crashes Workaround for nodejs/node#34797 * Tweak reporter integration tests Maybe it's the IPC changes, but they started to fail on Windows.
avajs · Aug 16, 2020 · 0f879f4 · 0f879f4
1 parent 20bc781
commit 0f879f4
Show file tree

Hide file tree

Showing 15 changed files with 89 additions and 41 deletions.
diff --git a/lib/cli.js b/lib/cli.js
@@ -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);
 				});

diff --git a/lib/fork.js b/lib/fork.js
@@ -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`.');
@@ -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;
 
@@ -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 => {
@@ -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});
 		}
 	};
 
@@ -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;
 			}
 

diff --git a/lib/ipc-flow-control.js b/lib/ipc-flow-control.js
@@ -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;
diff --git a/lib/worker/ipc.js b/lib/worker/ipc.js
@@ -1,5 +1,6 @@
 'use strict';
 const Emittery = require('emittery');
+const {controlFlow} = require('../ipc-flow-control');
 
 const emitter = new Emittery();
 process.on('message', message => {
@@ -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;

diff --git a/test-tap/fixture/report/regular/uncaught-exception.js b/test-tap/fixture/report/regular/uncaught-exception.js
@@ -1,7 +1,7 @@
 const test = require('../../../..');
 
 test('passes', t => {
-	setTimeout(() => {
+	setImmediate(() => {
 		throw new Error('Can’t catch me');
 	});
 	t.pass();

diff --git a/test-tap/reporters/mini.regular.v10.log b/test-tap/reporters/mini.regular.v10.log
@@ -515,13 +515,13 @@
 
   [90muncaught-exception.js:5[39m
 
-   [90m4:[39m   setTimeout(() => {                  
+   [90m4:[39m   setImmediate(() => {                
   [41m 5:     throw new Error('Can’t catch me');[49m
    [90m6:[39m   });                                 
 
   Error: Can’t catch me
 
-  [90m› Timeout.setTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
+  [90m› Immediate.setImmediate (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
 
 
 

diff --git a/test-tap/reporters/mini.regular.v12.log b/test-tap/reporters/mini.regular.v12.log
@@ -497,15 +497,14 @@
 
   [90muncaught-exception.js:5[39m
 
-   [90m4:[39m   setTimeout(() => {                  
+   [90m4:[39m   setImmediate(() => {                
   [41m 5:     throw new Error('Can’t catch me');[49m
    [90m6:[39m   });                                 
 
   Error: Can’t catch me
 
-  [90m› Timeout._onTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
-  [90m[2m› listOnTimeout (internal/timers.js:549:17)[22m[39m
-  [90m[2m› processTimers (internal/timers.js:492:7)[22m[39m
+  [90m› Immediate.<anonymous> (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
+  [90m[2m› processImmediate (internal/timers.js:456:21)[22m[39m
 
 
 

diff --git a/test-tap/reporters/mini.regular.v14.log b/test-tap/reporters/mini.regular.v14.log
@@ -497,15 +497,14 @@
 
   [90muncaught-exception.js:5[39m
 
-   [90m4:[39m   setTimeout(() => {                  
+   [90m4:[39m   setImmediate(() => {                
   [41m 5:     throw new Error('Can’t catch me');[49m
    [90m6:[39m   });                                 
 
   Error: Can’t catch me
 
-  [90m› Timeout._onTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
-  [90m[2m› listOnTimeout (internal/timers.js:551:17)[22m[39m
-  [90m[2m› processTimers (internal/timers.js:494:7)[22m[39m
+  [90m› Immediate.<anonymous> (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
+  [90m[2m› processImmediate (internal/timers.js:458:21)[22m[39m
 
 
 

diff --git a/test-tap/reporters/tap.regular.v10.log b/test-tap/reporters/tap.regular.v10.log
@@ -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

diff --git a/test-tap/reporters/tap.regular.v12.log b/test-tap/reporters/tap.regular.v12.log
@@ -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

diff --git a/test-tap/reporters/tap.regular.v14.log b/test-tap/reporters/tap.regular.v14.log
@@ -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

diff --git a/test-tap/reporters/verbose.regular.v10.log b/test-tap/reporters/verbose.regular.v10.log
@@ -87,13 +87,13 @@
 
   [90muncaught-exception.js:5[39m
 
-   [90m4:[39m   setTimeout(() => {                  
+   [90m4:[39m   setImmediate(() => {                
   [41m 5:     throw new Error('Can’t catch me');[49m
    [90m6:[39m   });                                 
 
   Error: Can’t catch me
 
-  [90m› Timeout.setTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
+  [90m› Immediate.setImmediate (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
 
 ---tty-stream-chunk-separator
   [31m✖ uncaught-exception.js exited with a non-zero exit code: 1[39m

diff --git a/test-tap/reporters/verbose.regular.v12.log b/test-tap/reporters/verbose.regular.v12.log
@@ -87,15 +87,14 @@
 
   [90muncaught-exception.js:5[39m
 
-   [90m4:[39m   setTimeout(() => {                  
+   [90m4:[39m   setImmediate(() => {                
   [41m 5:     throw new Error('Can’t catch me');[49m
    [90m6:[39m   });                                 
 
   Error: Can’t catch me
 
-  [90m› Timeout._onTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
-  [90m[2m› listOnTimeout (internal/timers.js:549:17)[22m[39m
-  [90m[2m› processTimers (internal/timers.js:492:7)[22m[39m
+  [90m› Immediate.<anonymous> (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
+  [90m[2m› processImmediate (internal/timers.js:456:21)[22m[39m
 
 ---tty-stream-chunk-separator
   [31m✖ uncaught-exception.js exited with a non-zero exit code: 1[39m

diff --git a/test-tap/reporters/verbose.regular.v14.log b/test-tap/reporters/verbose.regular.v14.log
@@ -87,15 +87,14 @@
 
   [90muncaught-exception.js:5[39m
 
-   [90m4:[39m   setTimeout(() => {                  
+   [90m4:[39m   setImmediate(() => {                
   [41m 5:     throw new Error('Can’t catch me');[49m
    [90m6:[39m   });                                 
 
   Error: Can’t catch me
 
-  [90m› Timeout._onTimeout (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
-  [90m[2m› listOnTimeout (internal/timers.js:551:17)[22m[39m
-  [90m[2m› processTimers (internal/timers.js:494:7)[22m[39m
+  [90m› Immediate.<anonymous> (test-tap/fixture/report/regular/uncaught-exception.js:5:9)[39m
+  [90m[2m› processImmediate (internal/timers.js:458:21)[22m[39m
 
 ---tty-stream-chunk-separator
   [31m✖ uncaught-exception.js exited with a non-zero exit code: 1[39m

diff --git a/test/helpers/exec.js b/test/helpers/exec.js
@@ -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));