From f305f5fa9f32ac8bdba480f000d6d52464b3fb30 Mon Sep 17 00:00:00 2001 From: Zack Newsham Date: Thu, 23 Nov 2023 23:03:16 -0500 Subject: [PATCH 1/9] watch: enable passthrough ipc in watch mode --- lib/internal/main/watch_mode.js | 32 ++++++---- lib/internal/watch_mode/files_watcher.js | 23 +++++++ test/sequential/test-watch-mode.mjs | 80 ++++++++++++++++++++++++ 3 files changed, 124 insertions(+), 11 deletions(-) diff --git a/lib/internal/main/watch_mode.js b/lib/internal/main/watch_mode.js index 4381cf0d381461..d70908e6e3cd9b 100644 --- a/lib/internal/main/watch_mode.js +++ b/lib/internal/main/watch_mode.js @@ -81,6 +81,7 @@ function start() { process.stdout.write(`${red}Failed running ${kCommandStr}${white}\n`); } }); + return child; } async function killAndWait(signal = kKillSignal, force = false) { @@ -113,7 +114,9 @@ function reportGracefulTermination() { }; } -async function stop() { +async function stop(child) { + // Without this line, the child process is still able to receive IPC, but is unable to send additional messages + watcher.destroyIPC(child); watcher.clearFileFilters(); const clearGraceReport = reportGracefulTermination(); await killAndWait(); @@ -121,26 +124,33 @@ async function stop() { } let restarting = false; -async function restart() { +async function restart(child) { if (restarting) return; restarting = true; try { if (!kPreserveOutput) process.stdout.write(clear); process.stdout.write(`${green}Restarting ${kCommandStr}${white}\n`); - await stop(); - start(); + await stop(child); + return start(); } finally { restarting = false; } } -start(); -watcher - .on('changed', restart) - .on('error', (error) => { - watcher.off('changed', restart); - triggerUncaughtException(error, true /* fromPromise */); - }); +async function init() { + let child = start(); + const restartChild = async () => { + child = await restart(child); + }; + watcher + .on('changed', restartChild) + .on('error', (error) => { + watcher.off('changed', restartChild); + triggerUncaughtException(error, true /* fromPromise */); + }); +} + +init(); // Exiting gracefully to avoid stdout/stderr getting written after // parent process is killed. diff --git a/lib/internal/watch_mode/files_watcher.js b/lib/internal/watch_mode/files_watcher.js index e3f37557a627dc..176e1156c37510 100644 --- a/lib/internal/watch_mode/files_watcher.js +++ b/lib/internal/watch_mode/files_watcher.js @@ -31,6 +31,7 @@ class FilesWatcher extends EventEmitter { #debounce; #mode; #signal; + #wantsPassthroughIPC = false; constructor({ debounce = 200, mode = 'filter', signal } = kEmptyObject) { super({ __proto__: null, captureRejections: true }); @@ -40,6 +41,7 @@ class FilesWatcher extends EventEmitter { this.#debounce = debounce; this.#mode = mode; this.#signal = signal; + this.#wantsPassthroughIPC = !!process.send; if (signal) { addAbortListener(signal, () => this.clear()); @@ -128,7 +130,28 @@ class FilesWatcher extends EventEmitter { this.#ownerDependencies.set(owner, dependencies); } } + + + #setupIPC(child) { + child._ipcMessages = { + parentToChild: (message) => child.send(message), + childToParent: (message) => process.send(message), + }; + process.on('message', child._ipcMessages.parentToChild); + child.on('message', child._ipcMessages.childToParent); + } + + destroyIPC(child) { + if (this.#wantsPassthroughIPC) { + process.off('message', child._ipcMessages.parentToChild); + child.off('message', child._ipcMessages.childToParent); + } + } + watchChildProcessModules(child, key = null) { + if (this.#wantsPassthroughIPC) { + this.#setupIPC(child); + } if (this.#mode !== 'filter') { return; } diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index 8bf0bdff7899d2..34093c2c84f291 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -8,6 +8,7 @@ import { spawn } from 'node:child_process'; import { writeFileSync, readFileSync, mkdirSync } from 'node:fs'; import { inspect } from 'node:util'; import { pathToFileURL } from 'node:url'; +import { once } from 'node:events'; import { createInterface } from 'node:readline'; if (common.isIBMi) @@ -342,6 +343,7 @@ console.log(values.random); ]); }); + // TODO: Remove skip after https://github.com/nodejs/node/pull/45271 lands it('should not watch when running an missing file', { skip: !supportsRecursive @@ -574,4 +576,82 @@ console.log(values.random); `Completed running ${inspect(file)}`, ]); }); + it('should pass IPC messages from a spawning parent to the child and back', async () => { + const file = createTmpFile(`console.log('running'); +process.on('message', (message) => { + if (message === 'exit') { + process.exit(0); + } else { + console.log('Received:', message); + process.send(message); + } +})`); + + const child = spawn( + execPath, + [ + '--watch', + '--no-warnings', + file, + ], + { + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe', 'ipc'], + }, + ); + + let stderr = ''; + let stdout = ''; + + child.stdout.on('data', (data) => stdout += data); + child.stderr.on('data', (data) => stderr += data); + async function waitForEcho(msg) { + const receivedPromise = new Promise((resolve) => { + const fn = (message) => { + if (message === msg) { + child.off('message', fn); + resolve(); + } + }; + child.on('message', fn); + }); + child.send(msg); + await receivedPromise; + } + + async function waitForText(text) { + const seenPromise = new Promise((resolve) => { + const fn = (data) => { + if (data.toString().includes(text)) { + resolve(); + child.stdout.off('data', fn); + } + }; + child.stdout.on('data', fn); + }); + await seenPromise; + } + + await waitForEcho('first message'); + const stopRestarts = restart(file); + await waitForText('running'); + stopRestarts(); + await waitForEcho('second message'); + const exitedPromise = once(child, 'exit'); + child.send('exit'); + await waitForText('Completed'); + child.disconnect(); + child.kill(); + await exitedPromise; + assert.strictEqual(stderr, ''); + const lines = stdout.split(/\r?\n/).filter(Boolean); + assert.deepStrictEqual(lines, [ + 'running', + 'Received: first message', + `Restarting '${file}'`, + 'running', + 'Received: second message', + `Completed running ${inspect(file)}`, + ]); + }); }); From bf4089b1d8efaa73e3031cfbfba2a68247243095 Mon Sep 17 00:00:00 2001 From: Zack Newsham Date: Sat, 25 Nov 2023 18:25:06 -0500 Subject: [PATCH 2/9] watch: use SafeWeakMap instead of modifying process --- lib/internal/watch_mode/files_watcher.js | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/internal/watch_mode/files_watcher.js b/lib/internal/watch_mode/files_watcher.js index 176e1156c37510..094d9cc089f6c0 100644 --- a/lib/internal/watch_mode/files_watcher.js +++ b/lib/internal/watch_mode/files_watcher.js @@ -5,6 +5,7 @@ const { ArrayPrototypeForEach, SafeMap, SafeSet, + SafeWeakMap, StringPrototypeStartsWith, } = primordials; @@ -32,6 +33,7 @@ class FilesWatcher extends EventEmitter { #mode; #signal; #wantsPassthroughIPC = false; + #ipcHandlers = new SafeWeakMap(); constructor({ debounce = 200, mode = 'filter', signal } = kEmptyObject) { super({ __proto__: null, captureRejections: true }); @@ -41,7 +43,7 @@ class FilesWatcher extends EventEmitter { this.#debounce = debounce; this.#mode = mode; this.#signal = signal; - this.#wantsPassthroughIPC = !!process.send; + this.#wantsPassthroughIPC = Boolean(process.send); if (signal) { addAbortListener(signal, () => this.clear()); @@ -133,18 +135,21 @@ class FilesWatcher extends EventEmitter { #setupIPC(child) { - child._ipcMessages = { + const handlers = { + __proto__: null, parentToChild: (message) => child.send(message), childToParent: (message) => process.send(message), }; - process.on('message', child._ipcMessages.parentToChild); - child.on('message', child._ipcMessages.childToParent); + this.#ipcHandlers.set(child, handlers); + process.on('message', handlers.parentToChild); + child.on('message', handlers.childToParent); } destroyIPC(child) { - if (this.#wantsPassthroughIPC) { - process.off('message', child._ipcMessages.parentToChild); - child.off('message', child._ipcMessages.childToParent); + const handlers = this.#ipcHandlers.get(child); + if (this.#wantsPassthroughIPC && handlers !== undefined) { + process.off('message', handlers.parentToChild); + child.off('message', handlers.childToParent); } } From 038d9aba64485f804b585b22aa77ade8fb3de6ca Mon Sep 17 00:00:00 2001 From: Zack Newsham Date: Sun, 26 Nov 2023 11:54:27 -0500 Subject: [PATCH 3/9] watch: use better name for passthroughIPC --- lib/internal/watch_mode/files_watcher.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/internal/watch_mode/files_watcher.js b/lib/internal/watch_mode/files_watcher.js index 094d9cc089f6c0..f659bc1696c7f6 100644 --- a/lib/internal/watch_mode/files_watcher.js +++ b/lib/internal/watch_mode/files_watcher.js @@ -32,7 +32,7 @@ class FilesWatcher extends EventEmitter { #debounce; #mode; #signal; - #wantsPassthroughIPC = false; + #passthroughIPC = false; #ipcHandlers = new SafeWeakMap(); constructor({ debounce = 200, mode = 'filter', signal } = kEmptyObject) { @@ -43,7 +43,7 @@ class FilesWatcher extends EventEmitter { this.#debounce = debounce; this.#mode = mode; this.#signal = signal; - this.#wantsPassthroughIPC = Boolean(process.send); + this.#passthroughIPC = Boolean(process.send); if (signal) { addAbortListener(signal, () => this.clear()); @@ -147,14 +147,14 @@ class FilesWatcher extends EventEmitter { destroyIPC(child) { const handlers = this.#ipcHandlers.get(child); - if (this.#wantsPassthroughIPC && handlers !== undefined) { + if (this.#passthroughIPC && handlers !== undefined) { process.off('message', handlers.parentToChild); child.off('message', handlers.childToParent); } } watchChildProcessModules(child, key = null) { - if (this.#wantsPassthroughIPC) { + if (this.#passthroughIPC) { this.#setupIPC(child); } if (this.#mode !== 'filter') { From 305a201eee3cc7d7b0b28f620b328c4c0729586d Mon Sep 17 00:00:00 2001 From: Zack Date: Sun, 3 Dec 2023 22:01:48 -0500 Subject: [PATCH 4/9] net: import Boolean Co-authored-by: Benjamin Gruenbaum --- lib/internal/watch_mode/files_watcher.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/internal/watch_mode/files_watcher.js b/lib/internal/watch_mode/files_watcher.js index f659bc1696c7f6..372a7c1bd4caa8 100644 --- a/lib/internal/watch_mode/files_watcher.js +++ b/lib/internal/watch_mode/files_watcher.js @@ -3,6 +3,7 @@ const { ArrayIsArray, ArrayPrototypeForEach, + Boolean, SafeMap, SafeSet, SafeWeakMap, From 3214ce2a6933bd3881ffec55db7e2a5a44fb46e8 Mon Sep 17 00:00:00 2001 From: Zack Date: Thu, 18 Jan 2024 11:12:37 -0500 Subject: [PATCH 5/9] watch: attempt to fix failing test --- test/sequential/test-watch-mode.mjs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index 34093c2c84f291..a7cc987c50560c 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -632,6 +632,7 @@ process.on('message', (message) => { await seenPromise; } + await waitForText("running"); await waitForEcho('first message'); const stopRestarts = restart(file); await waitForText('running'); From cd26c42502ed44353dd528d4917c159f563de2eb Mon Sep 17 00:00:00 2001 From: Zack Newsham Date: Fri, 19 Jan 2024 13:49:58 -0500 Subject: [PATCH 6/9] watch: fix lint --- test/sequential/test-watch-mode.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index a7cc987c50560c..f6eb86e28f9b03 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -632,7 +632,7 @@ process.on('message', (message) => { await seenPromise; } - await waitForText("running"); + await waitForText('running'); await waitForEcho('first message'); const stopRestarts = restart(file); await waitForText('running'); From 89367362c6c3be149e3747798507261188531bfc Mon Sep 17 00:00:00 2001 From: Zack Newsham Date: Wed, 31 Jan 2024 16:58:55 -0500 Subject: [PATCH 7/9] watch: fix tests --- test/sequential/test-watch-mode.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index f6eb86e28f9b03..9e192f9cf06472 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -649,7 +649,7 @@ process.on('message', (message) => { assert.deepStrictEqual(lines, [ 'running', 'Received: first message', - `Restarting '${file}'`, + `Restarting '${inspect(file)}'`, 'running', 'Received: second message', `Completed running ${inspect(file)}`, From b4775b095edcdf364501f430ef001baf79f5f388 Mon Sep 17 00:00:00 2001 From: Zack Newsham Date: Tue, 7 May 2024 08:45:16 -0400 Subject: [PATCH 8/9] watch: fix linter errors --- test/sequential/test-watch-mode.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index 9e192f9cf06472..8f611b12f10690 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -343,7 +343,6 @@ console.log(values.random); ]); }); - // TODO: Remove skip after https://github.com/nodejs/node/pull/45271 lands it('should not watch when running an missing file', { skip: !supportsRecursive @@ -576,6 +575,7 @@ console.log(values.random); `Completed running ${inspect(file)}`, ]); }); + it('should pass IPC messages from a spawning parent to the child and back', async () => { const file = createTmpFile(`console.log('running'); process.on('message', (message) => { From 2369dc8a1eabd428c0938516e07ba3b615d47126 Mon Sep 17 00:00:00 2001 From: Zack Newsham Date: Wed, 8 May 2024 15:47:44 -0400 Subject: [PATCH 9/9] watch: remove extraneous quote from failing test --- test/sequential/test-watch-mode.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index 8f611b12f10690..d1dbd75323ddc5 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -649,7 +649,7 @@ process.on('message', (message) => { assert.deepStrictEqual(lines, [ 'running', 'Received: first message', - `Restarting '${inspect(file)}'`, + `Restarting ${inspect(file)}`, 'running', 'Received: second message', `Completed running ${inspect(file)}`,