From 5da7d52a9ffb9df47601849173342382fa290f2a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 11 May 2020 00:25:49 +0200 Subject: [PATCH] test: regression tests for async_hooks + Promise + Worker interaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add regression tests for the case in which an async_hook is enabled inside a Worker thread and `process.exit()` is called during the async part of an async function. This commit includes multiple tests that seem like they should all crash in a similar way, but interestingly don’t. In particular, it’s surprising that the presence of a statement after `process.exit()` in a function has an effect on the kind of crash that’s being exhibited (V8 DCHECK vs. assertion in our own code) and the circumstances under which it crashes (e.g. the -1 and -2 tests can be “fixed” by reverting 13c5a1629cd025b, although they should have the same behavior as the -3 and -4 tests). PR-URL: https://github.com/nodejs/node/pull/33347 Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Benjamin Gruenbaum Reviewed-By: David Carlier --- ...-async-hooks-worker-asyncfn-terminate-1.js | 15 ++++++++++++ ...-async-hooks-worker-asyncfn-terminate-2.js | 21 ++++++++++++++++ ...-async-hooks-worker-asyncfn-terminate-3.js | 20 ++++++++++++++++ ...-async-hooks-worker-asyncfn-terminate-4.js | 24 +++++++++++++++++++ 4 files changed, 80 insertions(+) create mode 100644 test/parallel/test-async-hooks-worker-asyncfn-terminate-1.js create mode 100644 test/parallel/test-async-hooks-worker-asyncfn-terminate-2.js create mode 100644 test/parallel/test-async-hooks-worker-asyncfn-terminate-3.js create mode 100644 test/parallel/test-async-hooks-worker-asyncfn-terminate-4.js diff --git a/test/parallel/test-async-hooks-worker-asyncfn-terminate-1.js b/test/parallel/test-async-hooks-worker-asyncfn-terminate-1.js new file mode 100644 index 00000000000000..eb1664591308a3 --- /dev/null +++ b/test/parallel/test-async-hooks-worker-asyncfn-terminate-1.js @@ -0,0 +1,15 @@ +'use strict'; +const common = require('../common'); +const { Worker } = require('worker_threads'); + +const w = new Worker(` +const { createHook } = require('async_hooks'); + +setImmediate(async () => { + createHook({ init() {} }).enable(); + await 0; + process.exit(); +}); +`, { eval: true }); + +w.on('exit', common.mustCall()); diff --git a/test/parallel/test-async-hooks-worker-asyncfn-terminate-2.js b/test/parallel/test-async-hooks-worker-asyncfn-terminate-2.js new file mode 100644 index 00000000000000..049264d3e8aef5 --- /dev/null +++ b/test/parallel/test-async-hooks-worker-asyncfn-terminate-2.js @@ -0,0 +1,21 @@ +'use strict'; +const common = require('../common'); +const { Worker } = require('worker_threads'); + +// Like test-async-hooks-worker-promise.js but with the `await` and `createHook` +// lines switched, because that resulted in different assertion failures +// (one a Node.js assertion and one a V8 DCHECK) and it seems prudent to +// cover both of those failures. + +const w = new Worker(` +const { createHook } = require('async_hooks'); + +setImmediate(async () => { + await 0; + createHook({ init() {} }).enable(); + process.exit(); +}); +`, { eval: true }); + +w.postMessage({}); +w.on('exit', common.mustCall()); diff --git a/test/parallel/test-async-hooks-worker-asyncfn-terminate-3.js b/test/parallel/test-async-hooks-worker-asyncfn-terminate-3.js new file mode 100644 index 00000000000000..40c7d85835ae44 --- /dev/null +++ b/test/parallel/test-async-hooks-worker-asyncfn-terminate-3.js @@ -0,0 +1,20 @@ +'use strict'; +const common = require('../common'); +const { Worker } = require('worker_threads'); + +// Like test-async-hooks-worker-promise.js but with an additional statement +// after the `process.exit()` call, that shouldn’t really make a difference +// but apparently does. + +const w = new Worker(` +const { createHook } = require('async_hooks'); + +setImmediate(async () => { + createHook({ init() {} }).enable(); + await 0; + process.exit(); + process._rawDebug('THIS SHOULD NEVER BE REACHED'); +}); +`, { eval: true }); + +w.on('exit', common.mustCall()); diff --git a/test/parallel/test-async-hooks-worker-asyncfn-terminate-4.js b/test/parallel/test-async-hooks-worker-asyncfn-terminate-4.js new file mode 100644 index 00000000000000..dc641471b691e0 --- /dev/null +++ b/test/parallel/test-async-hooks-worker-asyncfn-terminate-4.js @@ -0,0 +1,24 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const { Worker } = require('worker_threads'); + +// Like test-async-hooks-worker-promise.js but doing a trivial counter increase +// after process.exit(). This should not make a difference, but apparently it +// does. This is *also* different from test-async-hooks-worker-promise-3.js, +// in that the statement is an ArrayBuffer access rather than a full method, +// which *also* makes a difference even though it shouldn’t. + +const workerData = new Int32Array(new SharedArrayBuffer(4)); +const w = new Worker(` +const { createHook } = require('async_hooks'); + +setImmediate(async () => { + createHook({ init() {} }).enable(); + await 0; + process.exit(); + workerData[0]++; +}); +`, { eval: true, workerData }); + +w.on('exit', common.mustCall(() => assert.strictEqual(workerData[0], 0)));