Skip to content

Commit c613e1b

Browse files
bpaserobpasero
authored
Merge pull request microsoft#13763 from Microsoft/ben/buffered-process-send
Add a queue for process.send
2 parents 82372ef + 907cc5e commit c613e1b

6 files changed

Lines changed: 172 additions & 12 deletions

File tree

‎src/vs/base/node/processes.ts‎

Lines changed: 43 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,8 @@ import * as cp from 'child_process';
99
import ChildProcess = cp.ChildProcess;
1010
import exec = cp.exec;
1111
import spawn = cp.spawn;
12-
1312
import { PassThrough } from 'stream';
14-
15-
import { fork } from './stdFork';
16-
13+
import { fork } from 'vs/base/node/stdFork';
1714
import nls = require('vs/nls');
1815
import { PPromise, Promise, TPromise, TValueCallback, TProgressCallback, ErrorCallback } from 'vs/base/common/winjs.base';
1916
import * as Types from 'vs/base/common/types';
@@ -22,7 +19,6 @@ import URI from 'vs/base/common/uri';
2219
import * as Objects from 'vs/base/common/objects';
2320
import * as TPath from 'vs/base/common/paths';
2421
import * as Platform from 'vs/base/common/platform';
25-
2622
import { LineDecoder } from 'vs/base/node/decoder';
2723
import { CommandOptions, ForkOptions, SuccessData, Source, TerminateResponse, TerminateResponseCode, Executable } from 'vs/base/common/processes';
2824
export { CommandOptions, ForkOptions, SuccessData, Source, TerminateResponse, TerminateResponseCode };
@@ -447,4 +443,46 @@ export class StreamProcess extends AbstractProcess<StreamData> {
447443
pp({ stdin: childProcess.stdin, stdout: childProcess.stdout, stderr: childProcess.stderr });
448444
}
449445
}
446+
}
447+
448+
export interface IQueuedSender {
449+
send: (msg: any) => void;
450+
}
451+
452+
// Wrapper around process.send() that will queue any messages if the internal node.js
453+
// queue is filled with messages and only continue sending messages when the internal
454+
// queue is free again to consume messages.
455+
// On Windows we always wait for the send() method to return before sending the next message
456+
// to workaround https://github.com/nodejs/node/issues/7657 (IPC can freeze process)
457+
export function createQueuedSender(childProcess: ChildProcess | NodeJS.Process): IQueuedSender {
458+
let msgQueue = [];
459+
let useQueue = false;
460+
461+
const send = function (msg: any): void {
462+
if (useQueue) {
463+
msgQueue.push(msg); // add to the queue if the process cannot handle more messages
464+
return;
465+
}
466+
467+
let result = childProcess.send(msg, error => {
468+
if (error) {
469+
console.error(error); // unlikely to happen, best we can do is log this error
470+
}
471+
472+
useQueue = false; // we are good again to send directly without queue
473+
474+
// now send all the messages that we have in our queue and did not send yet
475+
if (msgQueue.length > 0) {
476+
const msgQueueCopy = msgQueue.slice(0);
477+
msgQueue = [];
478+
msgQueueCopy.forEach(entry => send(entry));
479+
}
480+
});
481+
482+
if (!result || Platform.isWindows /* workaround https://github.com/nodejs/node/issues/7657 */) {
483+
useQueue = true;
484+
}
485+
};
486+
487+
return { send };
450488
}

‎src/vs/base/test/node/processes/fixtures/fork.ts‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
'use strict';
7+
8+
import processes = require('vs/base/node/processes');
9+
10+
const sender = processes.createQueuedSender(process);
11+
12+
process.on('message', msg => {
13+
sender.send(msg);
14+
});
15+
16+
sender.send('ready');

‎src/vs/base/test/node/processes/fixtures/fork_large.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
'use strict';
7+
8+
import processes = require('vs/base/node/processes');
9+
10+
const sender = processes.createQueuedSender(process);
11+
12+
process.on('message', msg => {
13+
sender.send(msg);
14+
sender.send(msg);
15+
sender.send(msg);
16+
sender.send('done');
17+
});
18+
19+
sender.send('ready');

‎src/vs/base/test/node/processes/processes.test.ts‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
* Licensed under the MIT License. See License.txt in the project root for license information.
4+
*--------------------------------------------------------------------------------------------*/
5+
6+
'use strict';
7+
8+
import * as assert from 'assert';
9+
import * as cp from 'child_process';
10+
import * as objects from 'vs/base/common/objects';
11+
import URI from 'vs/base/common/uri';
12+
import processes = require('vs/base/node/processes');
13+
14+
function fork(id: string): cp.ChildProcess {
15+
const opts: any = {
16+
env: objects.mixin(objects.clone(process.env), {
17+
AMD_ENTRYPOINT: id,
18+
PIPE_LOGGING: 'true',
19+
VERBOSE_LOGGING: true
20+
})
21+
};
22+
23+
return cp.fork(URI.parse(require.toUrl('bootstrap')).fsPath, ['--type=processTests'], opts);
24+
}
25+
26+
suite('Processes', () => {
27+
test('buffered sending - simple data', function (done: () => void) {
28+
const child = fork('vs/base/test/node/processes/fixtures/fork');
29+
const sender = processes.createQueuedSender(child);
30+
31+
let counter = 0;
32+
33+
const msg1 = 'Hello One';
34+
const msg2 = 'Hello Two';
35+
const msg3 = 'Hello Three';
36+
37+
child.on('message', msgFromChild => {
38+
if (msgFromChild === 'ready') {
39+
sender.send(msg1);
40+
sender.send(msg2);
41+
sender.send(msg3);
42+
} else {
43+
counter++;
44+
45+
if (counter === 1) {
46+
assert.equal(msgFromChild, msg1);
47+
} else if (counter === 2) {
48+
assert.equal(msgFromChild, msg2);
49+
} else if (counter === 3) {
50+
assert.equal(msgFromChild, msg3);
51+
52+
child.kill();
53+
done();
54+
}
55+
}
56+
});
57+
});
58+
59+
test('buffered sending - lots of data (potential deadlock on win32)', function (done: () => void) {
60+
const child = fork('vs/base/test/node/processes/fixtures/fork_large');
61+
const sender = processes.createQueuedSender(child);
62+
63+
const largeObj = Object.create(null);
64+
for (let i = 0; i < 10000; i++) {
65+
largeObj[i] = 'some data';
66+
}
67+
68+
const msg = JSON.stringify(largeObj);
69+
child.on('message', msgFromChild => {
70+
if (msgFromChild === 'ready') {
71+
sender.send(msg);
72+
sender.send(msg);
73+
sender.send(msg);
74+
} else if (msgFromChild === 'done') {
75+
child.kill();
76+
done();
77+
}
78+
});
79+
});
80+
});

‎src/vs/workbench/node/extensionHostProcess.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { TPromise } from 'vs/base/common/winjs.base';
1010
import { ExtensionHostMain, IInitData, exit } from 'vs/workbench/node/extensionHostMain';
1111
import { create as createIPC, IMainProcessExtHostIPC } from 'vs/platform/extensions/common/ipcRemoteCom';
1212
import marshalling = require('vs/base/common/marshalling');
13+
import { createQueuedSender } from 'vs/base/node/processes';
1314

1415
interface IRendererConnection {
1516
remoteCom: IMainProcessExtHostIPC;
@@ -22,6 +23,9 @@ let onTerminate = function () {
2223
exit();
2324
};
2425

26+
// Utility to not flood the process.send() with messages if it is busy catching up
27+
const queuedSender = createQueuedSender(process);
28+
2529
function connectToRenderer(): TPromise<IRendererConnection> {
2630
return new TPromise<IRendererConnection>((c, e) => {
2731
const stats: number[] = [];
@@ -32,7 +36,7 @@ function connectToRenderer(): TPromise<IRendererConnection> {
3236
let msg = marshalling.parse(raw);
3337

3438
const remoteCom = createIPC(data => {
35-
process.send(data);
39+
queuedSender.send(data);
3640
stats.push(data.length);
3741
});
3842

@@ -92,13 +96,13 @@ function connectToRenderer(): TPromise<IRendererConnection> {
9296
}, 1000);
9397

9498
// Tell the outside that we are initialized
95-
process.send('initialized');
99+
queuedSender.send('initialized');
96100

97101
c({ remoteCom, initData: msg });
98102
});
99103

100104
// Tell the outside that we are ready to receive messages
101-
process.send('ready');
105+
queuedSender.send('ready');
102106
});
103107
}
104108

‎src/vs/workbench/services/extensions/electron-browser/extensionHost.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import { ExtensionScanner, MessagesCollector } from 'vs/workbench/node/extension
2929
import { IMessagePassingProtocol } from 'vs/base/parts/ipc/common/ipc';
3030
import Event, { Emitter } from 'vs/base/common/event';
3131
import { IStorageService, StorageScope } from 'vs/platform/storage/common/storage';
32+
import { createQueuedSender, IQueuedSender } from 'vs/base/node/processes';
3233

3334
export const EXTENSION_LOG_BROADCAST_CHANNEL = 'vscode:extensionLog';
3435
export const EXTENSION_ATTACH_BROADCAST_CHANNEL = 'vscode:extensionAttach';
@@ -47,6 +48,7 @@ export interface ILogEntry {
4748
export class ExtensionHostProcessWorker {
4849
private initializeExtensionHostProcess: TPromise<ChildProcess>;
4950
private extensionHostProcessHandle: ChildProcess;
51+
private extensionHostProcessQueuedSender: IQueuedSender;
5052
private extensionHostProcessReady: boolean;
5153
private initializeTimer: number;
5254

@@ -126,6 +128,7 @@ export class ExtensionHostProcessWorker {
126128

127129
// Run Extension Host as fork of current process
128130
this.extensionHostProcessHandle = fork(URI.parse(require.toUrl('bootstrap')).fsPath, ['--type=extensionHost'], opts);
131+
this.extensionHostProcessQueuedSender = createQueuedSender(this.extensionHostProcessHandle);
129132

130133
// Notify debugger that we are ready to attach to the process if we run a development extension
131134
if (this.isExtensionDevelopmentHost && port) {
@@ -221,7 +224,7 @@ export class ExtensionHostProcessWorker {
221224
workspaceStoragePath: this.storageService.getStoragePath(StorageScope.WORKSPACE),
222225
extensions: extensionDescriptors
223226
});
224-
this.extensionHostProcessHandle.send(initPayload);
227+
this.extensionHostProcessQueuedSender.send(initPayload);
225228
});
226229
}
227230

@@ -351,9 +354,9 @@ export class ExtensionHostProcessWorker {
351354

352355
public send(msg: any): void {
353356
if (this.extensionHostProcessReady) {
354-
this.extensionHostProcessHandle.send(msg);
357+
this.extensionHostProcessQueuedSender.send(msg);
355358
} else if (this.initializeExtensionHostProcess) {
356-
this.initializeExtensionHostProcess.done(p => p.send(msg));
359+
this.initializeExtensionHostProcess.done(() => this.extensionHostProcessQueuedSender.send(msg));
357360
} else {
358361
this.unsentMessages.push(msg);
359362
}
@@ -363,7 +366,7 @@ export class ExtensionHostProcessWorker {
363366
this.terminating = true;
364367

365368
if (this.extensionHostProcessHandle) {
366-
this.extensionHostProcessHandle.send({
369+
this.extensionHostProcessQueuedSender.send({
367370
type: '__$terminate'
368371
});
369372
}

0 commit comments

Comments
 (0)