Skip to content

Commit f9defa6

Browse files
authored
lib: fix benchmark record acknowledgement race
The child can receive an acknowledgement and send its next record before the parent's send callback runs. Clear recordPending before sending the acknowledgement to avoid rejecting that record as an invalid sequence. Add a regression test that delays send callbacks until the child sends its next record or closes. Signed-off-by: Filip Skokan <panva.ip@gmail.com> Assisted-by: Codex PR-URL: #66430 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 6d5e309 commit f9defa6

3 files changed

Lines changed: 58 additions & 3 deletions

File tree

‎lib/internal/bench_runner/cli.js‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -956,10 +956,11 @@ async function runChild(path, options, scope, onRecord) {
956956
}
957957
const pending = handleRecord(record);
958958
if (protocolError === undefined) {
959-
const acknowledged = PromisePrototypeThen(
960-
PromiseResolve(pending), () => sendAck(child, message.id));
961-
trackPending(PromisePrototypeThen(acknowledged, () => {
959+
trackPending(PromisePrototypeThen(PromiseResolve(pending), () => {
960+
// The child can receive the acknowledgement and send its next
961+
// record before the send callback runs.
962962
recordPending = false;
963+
return sendAck(child, message.id);
963964
}));
964965
}
965966
} catch (error) {
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
'use strict';
2+
3+
const common = require('../../common');
4+
const childProcess = require('child_process');
5+
const { spawn } = childProcess;
6+
7+
childProcess.spawn = (...args) => {
8+
const child = spawn(...args);
9+
const { send } = child;
10+
child.send = function(message, handle, options, callback) {
11+
if (message?.type === 'node:bench:ack') {
12+
// Report send completion only after the child has responded to the ack.
13+
const onComplete = common.mustCall(() => {
14+
child.removeListener('message', onComplete);
15+
child.removeListener('close', onComplete);
16+
callback(null);
17+
});
18+
child.once('message', onComplete);
19+
child.once('close', onComplete);
20+
return send.call(this, message, handle, options, (error) => {
21+
if (error) {
22+
child.removeListener('message', onComplete);
23+
child.removeListener('close', onComplete);
24+
callback(error);
25+
}
26+
});
27+
}
28+
return send.call(this, message, handle, options, callback);
29+
};
30+
return child;
31+
};
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const { spawnSync } = require('child_process');
6+
const fixtures = require('../common/fixtures');
7+
8+
const result = spawnSync(process.execPath, [
9+
'--no-warnings', '--experimental-bench', '--bench',
10+
'--require', fixtures.path('bench-runner/delayed-ack.cjs'),
11+
'--bench-reporter=json',
12+
fixtures.path('bench-runner/acknowledged-records.mjs'),
13+
], { encoding: 'utf8', timeout: common.platformTimeout(30_000) });
14+
15+
assert.ifError(result.error);
16+
assert.strictEqual(result.signal, null);
17+
assert.strictEqual(result.status, 0, result.stdout + result.stderr);
18+
assert.strictEqual(result.stderr, '');
19+
const records = result.stdout.trim().split('\n').map((line) => JSON.parse(line));
20+
const diagnostics = records.filter(({ type }) => type === 'bench:diagnostic');
21+
assert.strictEqual(diagnostics.length, 32);
22+
assert(diagnostics.every(({ data }) => /^acknowledged 10\d{3}$/.test(data.message)));
23+
assert.strictEqual(records.at(-1).data.success, true);

0 commit comments

Comments
 (0)