Skip to content

Commit

Permalink
fix: Handle parent Ctrl-C in child process with setInterval
Browse files Browse the repository at this point in the history
This fixes a bug (#118) which occured when there is a script
with active setInterval call and appmap-node runs this script
indirectly with a node process: npx appmap-node node interval.js.
  • Loading branch information
zermelo-wisen committed Apr 11, 2024
1 parent 9e1048a commit fca31e3
Show file tree
Hide file tree
Showing 16 changed files with 224 additions and 5 deletions.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
12 changes: 9 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,17 @@
".": {
"require": {
"default": "./dist/facade.js",
"types": ["./dist/facade.d.ts"]
"types": [
"./dist/facade.d.ts"
]
},
"import": {
"default": "./dist/facade.js",
"types": ["./dist/facade.d.ts"]
"types": [
"./dist/facade.d.ts"
]
}
}
}
},
"repository": {
"type": "git",
Expand Down Expand Up @@ -58,6 +62,7 @@
"@types/mysql": "^2.15.24",
"@types/node": "^20.5.0",
"@types/pg": "^8.10.9",
"@types/ps-tree": "^1.1.6",
"@types/sqlite3": "^3.1.11",
"@types/tmp": "^0.2.3",
"@typescript-eslint/eslint-plugin": "^6.4.0",
Expand All @@ -72,6 +77,7 @@
"mongodb": "^6.3.0",
"next": "^14.0.4",
"prettier": "^3.0.2",
"ps-tree": "^1.2.0",
"react": "^18",
"react-dom": "^18",
"semantic-release": "^22.0.5",
Expand Down
7 changes: 6 additions & 1 deletion src/util/forwardSignals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@ export default function forwardSignals(
signals = DEFAULT_FORWARD,
) {
const forward = proc.kill.bind(proc);
for (const signal of signals) process.on(signal, forward);
// Without setTimeout, child does not handle forwarded SIGINT
// caused by the parent receiving a Ctrl-C in certain cases.
// See: simple.test.ts "forwards SIGINT (ctrl-c) properly
// to the grandchild having active setInterval"
for (const signal of signals)
process.on(signal, (...args) => setTimeout(() => forward(...args), 100));
}

const DEFAULT_FORWARD: NodeJS.Signals[] = [
Expand Down
68 changes: 68 additions & 0 deletions test/__snapshots__/simple.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,74 @@ exports[`forwarding signals to the child 1`] = `
}
`;

exports[`forwards SIGINT (ctrl-c) properly to the grandchild having active setInterval 1`] = `
{
"classMap": [
{
"children": [
{
"children": [
{
"location": "interval.js:1",
"name": "f",
"static": true,
"type": "function",
},
],
"name": "interval",
"type": "class",
},
],
"name": "simple",
"type": "package",
},
],
"events": [
{
"defined_class": "interval",
"event": "call",
"id": 1,
"lineno": 1,
"method_id": "f",
"parameters": [],
"path": "interval.js",
"static": true,
"thread_id": 0,
},
{
"elapsed": 31.337,
"event": "return",
"id": 2,
"parent_id": 1,
"return_value": {
"class": "Number",
"value": "1",
},
"thread_id": 0,
},
],
"metadata": {
"app": "simple",
"client": {
"name": "appmap-node",
"url": "https://github.com/getappmap/appmap-node",
"version": "test node-appmap version",
},
"language": {
"engine": "Node.js",
"name": "javascript",
"version": "test node version",
},
"name": "test process recording",
"recorder": {
"name": "process",
"type": "process",
},
},
"version": "1.12",
}
`;

exports[`mapping a custom Error class with a message property 1`] = `
{
"classMap": [
Expand Down
4 changes: 4 additions & 0 deletions test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ import fwdSlashPath from "../src/util/fwdSlashPath";

const binPath = resolve(__dirname, "../bin/appmap-node.js");

export function getAppMapBinPath() {
return binPath;
}

export function runAppmapNode(...args: string[]) {
console.debug("Running %s %s", binPath, args.join(" "));
const result = spawnSync(process.argv[0], [binPath, ...args], { cwd: target });
Expand Down
44 changes: 44 additions & 0 deletions test/simple.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import { spawn } from "node:child_process";
import { cpSync, mkdirSync, readFileSync } from "node:fs";
import { join } from "node:path";

import tmp from "tmp";
import psTree from "ps-tree";

import {
getAppMapBinPath,
integrationTest,
readAppmap,
readAppmaps,
Expand Down Expand Up @@ -43,6 +46,47 @@ integrationTestSkipOnWindows("forwarding signals to the child", async () => {
expect(readAppmap()).toMatchSnapshot();
});

integrationTestSkipOnWindows(
"forwards SIGINT (ctrl-c) properly to the grandchild having active setInterval",
async () => {
// This test tests a fix (see: forwardSignals.ts) for a bug which occured
// when there is a script with active setInterval call and appmap-node
// runs this script indirectly with a node process: appmap-node node interval.js.
// We run the commands similarly as reported in:
// https://github.com/getappmap/appmap-node/issues/118
const child = spawn("npx", [getAppMapBinPath(), "node", "interval.js"], {
cwd: resolveTarget(),
shell: true,
});

await new Promise<void>(
(r) =>
child.stdout?.on("data", (chunk: Buffer) => chunk.toString().includes("heartbeat") && r()),
);

child.kill("SIGINT");

// child.kill does not kill all grandchildren, we have to
// send the kill signal to all of them. Otherwise Jest will
// not finish and we are not able to get the SIGINT in
// appmap-node process.
// https://krasimirtsonev.com/blog/article/Nodejs-managing-child-processes-starting-stopping-exec-spawn#killing-stopping-the-command
psTree(child.pid!, function (err, children) {
children.forEach((child) => process.kill(parseInt(child.PID), "SIGINT"));
});

await new Promise<void>((r) => {
const handler = (): void => {
child.removeListener("exit", handler);
r();
};
child.once("exit", handler);
});

expect(readAppmap()).toMatchSnapshot();
},
);

integrationTest("mapping generator functions", () => {
expect(runAppmapNode("generator.js").status).toBe(0);
expect(readAppmap()).toMatchSnapshot();
Expand Down
9 changes: 9 additions & 0 deletions test/simple/interval.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
function f() {
return 1;
}

f();

setInterval(() => {
console.log("heartbeat");
}, 1000);
85 changes: 84 additions & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2226,6 +2226,13 @@ __metadata:
languageName: node
linkType: hard

"@types/ps-tree@npm:^1.1.6":
version: 1.1.6
resolution: "@types/ps-tree@npm:1.1.6"
checksum: bf5b7bb9bd11b8762a8302b93c335728ecb19c85a74c640a3888d476368a03733f11612b9a87b1ad9ea56f95720db23a824c78113b16024dc59264b7f9008df5
languageName: node
linkType: hard

"@types/react@npm:^18":
version: 18.2.45
resolution: "@types/react@npm:18.2.45"
Expand Down Expand Up @@ -2744,6 +2751,7 @@ __metadata:
"@types/mysql": ^2.15.24
"@types/node": ^20.5.0
"@types/pg": ^8.10.9
"@types/ps-tree": ^1.1.6
"@types/sqlite3": ^3.1.11
"@types/tmp": ^0.2.3
"@typescript-eslint/eslint-plugin": ^6.4.0
Expand All @@ -2763,6 +2771,7 @@ __metadata:
mongodb: ^6.3.0
next: ^14.0.4
prettier: ^3.0.2
ps-tree: ^1.2.0
react: ^18
react-dom: ^18
semantic-release: ^22.0.5
Expand Down Expand Up @@ -3990,6 +3999,13 @@ __metadata:
languageName: node
linkType: hard

"duplexer@npm:~0.1.1":
version: 0.1.2
resolution: "duplexer@npm:0.1.2"
checksum: 62ba61a830c56801db28ff6305c7d289b6dc9f859054e8c982abd8ee0b0a14d2e9a8e7d086ffee12e868d43e2bbe8a964be55ddbd8c8957714c87373c7a4f9b0
languageName: node
linkType: hard

"eastasianwidth@npm:^0.2.0":
version: 0.2.0
resolution: "eastasianwidth@npm:0.2.0"
Expand Down Expand Up @@ -4361,6 +4377,21 @@ __metadata:
languageName: node
linkType: hard

"event-stream@npm:=3.3.4":
version: 3.3.4
resolution: "event-stream@npm:3.3.4"
dependencies:
duplexer: ~0.1.1
from: ~0
map-stream: ~0.1.0
pause-stream: 0.0.11
split: 0.3
stream-combiner: ~0.0.4
through: ~2.3.1
checksum: 80b467820b6daf824d9fb4345d2daf115a056e5c104463f2e98534e92d196a27f2df5ea2aa085624db26f4c45698905499e881d13bc7c01f7a13eac85be72a22
languageName: node
linkType: hard

"event-target-shim@npm:^5.0.0":
version: 5.0.1
resolution: "event-target-shim@npm:5.0.1"
Expand Down Expand Up @@ -4718,6 +4749,13 @@ __metadata:
languageName: node
linkType: hard

"from@npm:~0":
version: 0.1.7
resolution: "from@npm:0.1.7"
checksum: b85125b7890489656eb2e4f208f7654a93ec26e3aefaf3bbbcc0d496fc1941e4405834fcc9fe7333192aa2187905510ace70417bbf9ac6f6f4784a731d986939
languageName: node
linkType: hard

"fs-extra@npm:^11.0.0":
version: 11.1.1
resolution: "fs-extra@npm:11.1.1"
Expand Down Expand Up @@ -6763,6 +6801,13 @@ __metadata:
languageName: node
linkType: hard

"map-stream@npm:~0.1.0":
version: 0.1.0
resolution: "map-stream@npm:0.1.0"
checksum: 38abbe4eb883888031e6b2fc0630bc583c99396be16b8ace5794b937b682a8a081f03e8b15bfd4914d1bc88318f0e9ac73ba3512ae65955cd449f63256ddb31d
languageName: node
linkType: hard

"marked-terminal@npm:^6.0.0":
version: 6.0.0
resolution: "marked-terminal@npm:6.0.0"
Expand Down Expand Up @@ -8112,6 +8157,15 @@ __metadata:
languageName: node
linkType: hard

"pause-stream@npm:0.0.11":
version: 0.0.11
resolution: "pause-stream@npm:0.0.11"
dependencies:
through: ~2.3
checksum: 3c4a14052a638b92e0c96eb00c0d7977df7f79ea28395250c525d197f1fc02d34ce1165d5362e2e6ebbb251524b94a76f3f0d4abc39ab8b016d97449fe15583c
languageName: node
linkType: hard

"pg-cloudflare@npm:^1.1.1":
version: 1.1.1
resolution: "pg-cloudflare@npm:1.1.1"
Expand Down Expand Up @@ -8522,6 +8576,17 @@ __metadata:
languageName: node
linkType: hard

"ps-tree@npm:^1.2.0":
version: 1.2.0
resolution: "ps-tree@npm:1.2.0"
dependencies:
event-stream: =3.3.4
bin:
ps-tree: ./bin/ps-tree.js
checksum: e635dd00f53d30d31696cf5f95b3a8dbdf9b1aeb36d4391578ce8e8cd22949b7c5536c73b0dc18c78615ea3ddd4be96101166be59ca2e3e3cb1e2f79ba3c7f98
languageName: node
linkType: hard

"punycode@npm:^2.1.0":
version: 2.3.0
resolution: "punycode@npm:2.3.0"
Expand Down Expand Up @@ -9337,6 +9402,15 @@ __metadata:
languageName: node
linkType: hard

"split@npm:0.3":
version: 0.3.3
resolution: "split@npm:0.3.3"
dependencies:
through: 2
checksum: 2e076634c9637cfdc54ab4387b6a243b8c33b360874a25adf6f327a5647f07cb3bf1c755d515248eb3afee4e382278d01f62c62d87263c118f28065b86f74f02
languageName: node
linkType: hard

"sprintf-js@npm:^1.1.3":
version: 1.1.3
resolution: "sprintf-js@npm:1.1.3"
Expand Down Expand Up @@ -9444,6 +9518,15 @@ __metadata:
languageName: node
linkType: hard

"stream-combiner@npm:~0.0.4":
version: 0.0.4
resolution: "stream-combiner@npm:0.0.4"
dependencies:
duplexer: ~0.1.1
checksum: 844b622cfe8b9de45a6007404f613b60aaf85200ab9862299066204242f89a7c8033b1c356c998aa6cfc630f6cd9eba119ec1c6dc1f93e245982be4a847aee7d
languageName: node
linkType: hard

"streamsearch@npm:^1.1.0":
version: 1.1.0
resolution: "streamsearch@npm:1.1.0"
Expand Down Expand Up @@ -9715,7 +9798,7 @@ __metadata:
languageName: node
linkType: hard

"through@npm:>=2.2.7 <3":
"through@npm:2, through@npm:>=2.2.7 <3, through@npm:~2.3, through@npm:~2.3.1":
version: 2.3.8
resolution: "through@npm:2.3.8"
checksum: a38c3e059853c494af95d50c072b83f8b676a9ba2818dcc5b108ef252230735c54e0185437618596c790bbba8fcdaef5b290405981ffa09dce67b1f1bf190cbd
Expand Down

0 comments on commit fca31e3

Please sign in to comment.