Skip to content

Commit 5fd1e45

Browse files
committed
Do not restart workers more than once
1 parent 28dd457 commit 5fd1e45

File tree

4 files changed

+36
-10
lines changed

4 files changed

+36
-10
lines changed

Diff for: lib/worker_wrapper.js

+4-2
Original file line numberDiff line numberDiff line change
@@ -392,7 +392,7 @@ class WorkerWrapper extends EventEmitterEx {
392392
// this call of setup sets inspectPort for node js debugger
393393
// it should be here so that every worker can receive the same debugger port after restarting
394394
if (this.inspectPort) {
395-
this._master.setup({inspectPort: this.inspectPort})
395+
this._master.setup({inspectPort: this.inspectPort});
396396
}
397397
/** @private */
398398
this._worker = cluster.fork({
@@ -432,6 +432,7 @@ class WorkerWrapper extends EventEmitterEx {
432432

433433
this.emit('stop');
434434
this.stopping = true;
435+
const stopPid = this._worker && this.pid;
435436

436437
setImmediate(() => {
437438
// state can be changed before function call
@@ -440,7 +441,8 @@ class WorkerWrapper extends EventEmitterEx {
440441
command: RPC.fns.worker.suspend,
441442
callback: () => {
442443
// Worker could die while suspend was in-flight, i.e. on endless loop, and _onStateStopped will drop _worker
443-
if (this._worker) {
444+
// Or a new one could be started already
445+
if (this._worker && (this.pid === stopPid)) {
444446
this._worker.disconnect();
445447
}
446448
}

Diff for: test/func/fixtures/suspend/master.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@ if (proc.isMaster) {
2626
case 'register suspend 200':
2727
proc.remoteCallToAll('register suspend', 200);
2828
break;
29-
case 'register suspend 100000':
30-
proc.remoteCallToAll('register suspend', 100000);
29+
case 'register suspend 3000':
30+
proc.remoteCallToAll('register suspend', 3000);
3131
break;
3232
case 'soft-restart':
3333
pEvent(proc, 'restarted').then(() => process.send('restarted'));

Diff for: test/func/fixtures/suspend/worker.js

+4
Original file line numberDiff line numberDiff line change
@@ -15,3 +15,7 @@ worker.registerRemoteCommand('register suspend', (_, timeout) => {
1515
worker.on('disconnect', () => {
1616
console.log('Got disconnect');
1717
});
18+
19+
worker.on('ready', () => {
20+
console.log('Got ready');
21+
});

Diff for: test/func/test/suspend.js

+26-6
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ describe('suspend before stop', () => {
1414
it('master calls suspend and waits for it to finish before stop', async () => {
1515
await instance.sendWaitTimeout('register suspend 100', 10);
1616
await instance.sendWaitAnswer('soft-restart', 'restarted');
17-
const expected = `Waiting 100ms in suspend function
17+
const expected = `Got ready
18+
Waiting 100ms in suspend function
1819
Finished waiting 100ms in suspend function
1920
Got disconnect
2021
`;
@@ -23,15 +24,16 @@ Got disconnect
2324

2425
it('master disconnected worker if no suspend function was registered', async () => {
2526
await instance.sendWaitAnswer('soft-restart', 'restarted');
26-
const expected = 'Got disconnect\n';
27+
const expected = 'Got ready\nGot disconnect\n';
2728
assert.equal(instance.output(), expected);
2829
});
2930

3031
it('worker waits for all registered suspend functions', async () => {
3132
await instance.sendWaitTimeout('register suspend 100', 10);
3233
await instance.sendWaitTimeout('register suspend 200', 10);
3334
await instance.sendWaitAnswer('soft-restart', 'restarted');
34-
const expected = `Waiting 100ms in suspend function
35+
const expected = `Got ready
36+
Waiting 100ms in suspend function
3537
Waiting 200ms in suspend function
3638
Finished waiting 100ms in suspend function
3739
Finished waiting 200ms in suspend function
@@ -41,9 +43,26 @@ Got disconnect
4143
});
4244

4345
it('master kills worker if suspend did not finish in stopTimeout', async () => {
44-
await instance.sendWaitTimeout('register suspend 100000', 10);
46+
await instance.sendWaitTimeout('register suspend 3000', 10);
4547
await instance.sendWaitAnswer('soft-restart', 'restarted');
46-
const expected = 'Waiting 100000ms in suspend function\n';
48+
const expected = 'Got ready\nWaiting 3000ms in suspend function\n';
49+
assert.equal(instance.output(), expected);
50+
});
51+
52+
it('master does not disconnect already killed worker', async function () {
53+
// eslint-disable-next-line no-invalid-this
54+
this.timeout(15000);
55+
56+
await instance.sendWaitTimeout('register suspend 3000', 10);
57+
await instance.sendWaitAnswer('soft-restart', 'restarted');
58+
// No "Got disconnect" is expected
59+
const expected = `Got ready
60+
Waiting 3000ms in suspend function
61+
Got ready
62+
`;
63+
64+
await new Promise(resolve => setTimeout(resolve, 10000));
65+
4766
assert.equal(instance.output(), expected);
4867
});
4968

@@ -53,7 +72,8 @@ Got disconnect
5372
const exitCode = await instance.sendWaitExit('shutdown');
5473

5574
// Keep those two 'shutting down' to make sure master process got our message and called 'shutdown' twice
56-
const expected = `Shutting down
75+
const expected = `Got ready
76+
Shutting down
5777
Shutting down
5878
Waiting 100ms in suspend function
5979
Finished waiting 100ms in suspend function

0 commit comments

Comments
 (0)