From b7aa21828158c6c619d09d648295cfb49b241cf5 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 5 Sep 2017 15:47:54 -0700 Subject: [PATCH 1/4] Document ThrottledOperations.schedule --- src/server/utilities.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/server/utilities.ts b/src/server/utilities.ts index e2e2a67eaf12d..353c5bcec2158 100644 --- a/src/server/utilities.ts +++ b/src/server/utilities.ts @@ -179,6 +179,12 @@ namespace ts.server { constructor(private readonly host: ServerHost) { } + /** + * Wait `number` milliseconds and then invoke `cb`. If, while waiting, schedule + * is called again with the same `operationId`, cancel this operation in favor + * of the new one. (Note that the amount of time the canceled operation had been + * waiting does not affect the amount of time that the new operation waits.) + */ public schedule(operationId: string, delay: number, cb: () => void) { const pendingTimeout = this.pendingTimeouts.get(operationId); if (pendingTimeout) { From 7fad32617a7e75ad3fa9b4483732e934da226e69 Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Tue, 5 Sep 2017 16:00:19 -0700 Subject: [PATCH 2/4] Limit the number of unanswered typings installer requests If we send them all at once, we (apparently) hit a buffer limit in the node IPC channel and both TS Server and the typings installer become unresponsive. --- src/server/server.ts | 62 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 57 insertions(+), 5 deletions(-) diff --git a/src/server/server.ts b/src/server/server.ts index 0fe37dd8ba0d5..f4a8d1064e389 100644 --- a/src/server/server.ts +++ b/src/server/server.ts @@ -235,24 +235,34 @@ namespace ts.server { return `${d.getHours()}:${d.getMinutes()}:${d.getSeconds()}.${d.getMilliseconds()}`; } + interface QueuedOperation { + operationId: string; + operation: () => void; + } + class NodeTypingsInstaller implements ITypingsInstaller { private installer: NodeChildProcess; private installerPidReported = false; private socket: NodeSocket; private projectService: ProjectService; - private throttledOperations: ThrottledOperations; private eventSender: EventSender; + private activeRequestCount = 0; + private requestQueue: QueuedOperation[] = []; + private requestMap = createMap(); // Maps operation ID to newest requestQueue entry with that ID + + private static readonly maxActiveRequestCount = 10; + private static readonly requestDelayMillis = 100; + constructor( private readonly telemetryEnabled: boolean, private readonly logger: server.Logger, - host: ServerHost, + private readonly host: ServerHost, eventPort: number, readonly globalTypingsCacheLocation: string, readonly typingSafeListLocation: string, private readonly npmLocation: string | undefined, private newLine: string) { - this.throttledOperations = new ThrottledOperations(host); if (eventPort) { const s = net.connect({ port: eventPort }, () => { this.socket = s; @@ -333,12 +343,26 @@ namespace ts.server { this.logger.info(`Scheduling throttled operation: ${JSON.stringify(request)}`); } } - this.throttledOperations.schedule(project.getProjectName(), /*ms*/ 250, () => { + + const operationId = project.getProjectName(); + const operation = () => { if (this.logger.hasLevel(LogLevel.verbose)) { this.logger.info(`Sending request: ${JSON.stringify(request)}`); } this.installer.send(request); - }); + }; + const queuedRequest: QueuedOperation = { operationId, operation }; + + if (this.activeRequestCount < NodeTypingsInstaller.maxActiveRequestCount) { + this.scheduleRequest(queuedRequest); + } + else { + if (this.logger.hasLevel(LogLevel.verbose)) { + this.logger.info(`Deferring request for: ${operationId}`); + } + this.requestQueue.push(queuedRequest); + this.requestMap.set(operationId, queuedRequest); + } } private handleMessage(response: SetTypings | InvalidateCachedTypings | BeginInstallTypes | EndInstallTypes | InitializationFailedResponse) { @@ -399,11 +423,39 @@ namespace ts.server { return; } + if (this.activeRequestCount > 0) { + this.activeRequestCount--; + } + else { + Debug.fail("Received too many responses"); + } + + while (this.requestQueue.length > 0) { + const queuedRequest = this.requestQueue.shift(); + if (this.requestMap.get(queuedRequest.operationId) == queuedRequest) { + this.requestMap.delete(queuedRequest.operationId); + this.scheduleRequest(queuedRequest); + break; + } + + if (this.logger.hasLevel(LogLevel.verbose)) { + this.logger.info(`Skipping defunct request for: ${queuedRequest.operationId}`); + } + } + this.projectService.updateTypingsForProject(response); if (response.kind === ActionSet && this.socket) { this.sendEvent(0, "setTypings", response); } } + + private scheduleRequest(request: QueuedOperation) { + if(this.logger.hasLevel(LogLevel.verbose)) { + this.logger.info(`Scheduling request for: ${request.operationId}`); + } + this.activeRequestCount++; + this.host.setTimeout(request.operation, NodeTypingsInstaller.requestDelayMillis); + } } class IOSession extends Session { From 1afd1fb94008230d52096285e105245ec076ab3b Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 6 Sep 2017 15:44:00 -0700 Subject: [PATCH 3/4] Fix lint issues --- src/server/server.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/server/server.ts b/src/server/server.ts index f4a8d1064e389..26f6171086b92 100644 --- a/src/server/server.ts +++ b/src/server/server.ts @@ -432,7 +432,7 @@ namespace ts.server { while (this.requestQueue.length > 0) { const queuedRequest = this.requestQueue.shift(); - if (this.requestMap.get(queuedRequest.operationId) == queuedRequest) { + if (this.requestMap.get(queuedRequest.operationId) === queuedRequest) { this.requestMap.delete(queuedRequest.operationId); this.scheduleRequest(queuedRequest); break; @@ -450,7 +450,7 @@ namespace ts.server { } private scheduleRequest(request: QueuedOperation) { - if(this.logger.hasLevel(LogLevel.verbose)) { + if (this.logger.hasLevel(LogLevel.verbose)) { this.logger.info(`Scheduling request for: ${request.operationId}`); } this.activeRequestCount++; From 7fcd7a47d79cc55fe795068302f2a0de49ca901c Mon Sep 17 00:00:00 2001 From: Andrew Casey Date: Wed, 6 Sep 2017 15:46:59 -0700 Subject: [PATCH 4/4] Add explanatory comment --- src/server/server.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/server/server.ts b/src/server/server.ts index 26f6171086b92..0552d86e7a04b 100644 --- a/src/server/server.ts +++ b/src/server/server.ts @@ -250,6 +250,11 @@ namespace ts.server { private requestQueue: QueuedOperation[] = []; private requestMap = createMap(); // Maps operation ID to newest requestQueue entry with that ID + // This number is essentially arbitrary. Processing more than one typings request + // at a time makes sense, but having too many in the pipe results in a hang + // (see https://github.com/nodejs/node/issues/7657). + // It would be preferable to base our limit on the amount of space left in the + // buffer, but we have yet to find a way to retrieve that value. private static readonly maxActiveRequestCount = 10; private static readonly requestDelayMillis = 100;