From 03ad2a17b2e516848064e4ec9eacff9799fc286d Mon Sep 17 00:00:00 2001 From: Asher Date: Thu, 28 Mar 2019 17:59:49 -0500 Subject: [PATCH] Handle disconnects (#363) * Make proxies decide how to handle disconnects * Connect to a new terminal instance on disconnect * Use our retry for the watcher * Specify method when proxy doesn't exist * Don't error when closing/killing disconnected proxy * Specify proxy ID when a method doesn't exist * Use our retry for the searcher Also dispose some things for the watcher because it doesn't seem that was done properly. The searcher also now starts immediately so there won't be lag when you perform your first search. * Use our retry for the extension host * Emit error in parent proxy class Reduces duplicate code. Not all items are "supposed" to have an error event according to the original implementation we are filling, but there is no reason why we can't emit our own events (and are already doing so for the "disconnected" event anyway). * Reconnect spdlog * Add error message when shared process disconnects * Pass method resolve to parse * Don't pass method to getProxy It doesn't tell you anything that trace logging wouldn't and has no relation to what the function actually does. * Fix infinite recursion when disposing protocol client in tests --- packages/protocol/src/browser/client.ts | 39 +++++--- .../src/browser/modules/child_process.ts | 5 + packages/protocol/src/browser/modules/fs.ts | 4 + packages/protocol/src/browser/modules/net.ts | 12 ++- .../protocol/src/browser/modules/node-pty.ts | 23 ++++- .../protocol/src/browser/modules/spdlog.ts | 16 +++- .../protocol/src/browser/modules/stream.ts | 15 +++ packages/protocol/src/common/proxy.ts | 35 ++++++- packages/protocol/src/node/server.ts | 2 +- packages/vscode/src/workbench.ts | 2 + scripts/vscode.patch | 96 ++++++++++++++++++- 11 files changed, 219 insertions(+), 30 deletions(-) diff --git a/packages/protocol/src/browser/client.ts b/packages/protocol/src/browser/client.ts index 470c862af..30239739b 100644 --- a/packages/protocol/src/browser/client.ts +++ b/packages/protocol/src/browser/client.ts @@ -134,14 +134,8 @@ export class Client { message.setResponse(stringify(error)); this.failEmitter.emit(message); - this.eventEmitter.emit({ event: "exit", args: [1] }); - this.eventEmitter.emit({ event: "close", args: [] }); - try { - this.eventEmitter.emit({ event: "error", args: [error] }); - } catch (error) { - // If nothing is listening, EventEmitter will throw an error. - } - this.eventEmitter.emit({ event: "done", args: [true] }); + this.eventEmitter.emit({ event: "disconnected", args: [error] }); + this.eventEmitter.emit({ event: "done", args: [] }); }; connection.onDown(() => handleDisconnect()); @@ -149,6 +143,12 @@ export class Client { clearTimeout(this.pingTimeout as any); this.pingTimeout = undefined; handleDisconnect(); + this.proxies.clear(); + this.successEmitter.dispose(); + this.failEmitter.dispose(); + this.eventEmitter.dispose(); + this.initDataEmitter.dispose(); + this.sharedProcessActiveEmitter.dispose(); }); connection.onUp(() => this.disconnected = false); @@ -174,8 +174,17 @@ export class Client { * Make a remote call for a proxy's method using proto. */ private remoteCall(proxyId: number | Module, method: string, args: any[]): Promise { - if (this.disconnected) { - return Promise.reject(new Error("disconnected")); + if (this.disconnected && typeof proxyId === "number") { + // Can assume killing or closing works because a disconnected proxy + // is disposed on the server's side. + switch (method) { + case "close": + case "kill": + return Promise.resolve(); + } + return Promise.reject( + new Error(`Unable to call "${method}" on proxy ${proxyId}: disconnected`), + ); } const message = new MethodMessage(); @@ -223,7 +232,7 @@ export class Client { // The server will send back a fail or success message when the method // has completed, so we listen for that based on the message's unique ID. - const promise = new Promise((resolve, reject): void => { + const promise = new Promise((resolve, reject): void => { const dispose = (): void => { d1.dispose(); d2.dispose(); @@ -237,7 +246,7 @@ export class Client { const d1 = this.successEmitter.event(id, (message) => { dispose(); - resolve(this.parse(message.getResponse())); + resolve(this.parse(message.getResponse(), promise)); }); const d2 = this.failEmitter.event(id, (message) => { @@ -450,12 +459,12 @@ export class Client { callbacks: new Map(), }); - instance.onDone((disconnected: boolean) => { + instance.onDone(() => { const log = (): void => { logger.trace(() => [ typeof proxyId === "number" ? "disposed proxy" : "disposed proxy callbacks", field("proxyId", proxyId), - field("disconnected", disconnected), + field("disconnected", this.disconnected), field("callbacks", Array.from(this.proxies.values()).reduce((count, proxy) => count + proxy.callbacks.size, 0)), field("success listeners", this.successEmitter.counts), field("fail listeners", this.failEmitter.counts), @@ -471,7 +480,7 @@ export class Client { this.eventEmitter.dispose(proxyId); log(); }; - if (!disconnected) { + if (!this.disconnected) { instance.dispose().then(dispose).catch(dispose); } else { dispose(); diff --git a/packages/protocol/src/browser/modules/child_process.ts b/packages/protocol/src/browser/modules/child_process.ts index de13e1718..d0f620076 100644 --- a/packages/protocol/src/browser/modules/child_process.ts +++ b/packages/protocol/src/browser/modules/child_process.ts @@ -87,6 +87,11 @@ export class ChildProcess extends ClientProxy implements cp.C return true; // Always true since we can't get this synchronously. } + + protected handleDisconnect(): void { + this.emit("exit", 1); + this.emit("close"); + } } export class ChildProcessModule { diff --git a/packages/protocol/src/browser/modules/fs.ts b/packages/protocol/src/browser/modules/fs.ts index bb23c6b06..c1a2b8e73 100644 --- a/packages/protocol/src/browser/modules/fs.ts +++ b/packages/protocol/src/browser/modules/fs.ts @@ -41,6 +41,10 @@ class Watcher extends ClientProxy implements fs.FSWatcher { public close(): void { this.proxy.close(); } + + protected handleDisconnect(): void { + this.emit("close"); + } } class WriteStream extends Writable implements fs.WriteStream { diff --git a/packages/protocol/src/browser/modules/net.ts b/packages/protocol/src/browser/modules/net.ts index 723bb1b11..0d8354c4f 100644 --- a/packages/protocol/src/browser/modules/net.ts +++ b/packages/protocol/src/browser/modules/net.ts @@ -126,6 +126,7 @@ export class Socket extends Duplex implements net.Socket { } export class Server extends ClientProxy implements net.Server { + private socketId = 0; private readonly sockets = new Map(); private _listening: boolean = false; @@ -133,7 +134,12 @@ export class Server extends ClientProxy implements net.Server { super(proxyPromise); this.proxy.onConnection((socketProxy) => { - this.emit("connection", new Socket(socketProxy)); + const socket = new Socket(socketProxy); + const socketId = this.socketId++; + this.sockets.set(socketId, socket); + socket.on("error", () => this.sockets.delete(socketId)) + socket.on("close", () => this.sockets.delete(socketId)) + this.emit("connection", socket); }); this.on("listening", () => this._listening = true); @@ -200,6 +206,10 @@ export class Server extends ClientProxy implements net.Server { public getConnections(cb: (error: Error | null, count: number) => void): void { cb(null, this.sockets.size); } + + protected handleDisconnect(): void { + this.emit("close"); + } } type NodeNet = typeof net; diff --git a/packages/protocol/src/browser/modules/node-pty.ts b/packages/protocol/src/browser/modules/node-pty.ts index 7c96859fd..997ba941c 100644 --- a/packages/protocol/src/browser/modules/node-pty.ts +++ b/packages/protocol/src/browser/modules/node-pty.ts @@ -6,11 +6,20 @@ export class NodePtyProcess extends ClientProxy implements private _pid = -1; private _process = ""; - public constructor(proxyPromise: Promise) { - super(proxyPromise); + public constructor( + private readonly moduleProxy: NodePtyModuleProxy, + private readonly file: string, + private readonly args: string[] | string, + private readonly options: pty.IPtyForkOptions, + ) { + super(moduleProxy.spawn(file, args, options)); + this.on("process", (process) => this._process = process); + } + + protected initialize(proxyPromise: Promise) { + super.initialize(proxyPromise); this.proxy.getPid().then((pid) => this._pid = pid); this.proxy.getProcess().then((process) => this._process = process); - this.on("process", (process) => this._process = process); } public get pid(): number { @@ -32,6 +41,12 @@ export class NodePtyProcess extends ClientProxy implements public kill(signal?: string): void { this.proxy.kill(signal); } + + protected handleDisconnect(): void { + this._process += " (disconnected)"; + this.emit("data", "\r\n\nLost connection...\r\n\n"); + this.initialize(this.moduleProxy.spawn(this.file, this.args, this.options)); + } } type NodePty = typeof pty; @@ -40,6 +55,6 @@ export class NodePtyModule implements NodePty { public constructor(private readonly proxy: NodePtyModuleProxy) {} public spawn = (file: string, args: string[] | string, options: pty.IPtyForkOptions): pty.IPty => { - return new NodePtyProcess(this.proxy.spawn(file, args, options)); + return new NodePtyProcess(this.proxy, file, args, options); } } diff --git a/packages/protocol/src/browser/modules/spdlog.ts b/packages/protocol/src/browser/modules/spdlog.ts index 9d10fbab2..80fc5dde2 100644 --- a/packages/protocol/src/browser/modules/spdlog.ts +++ b/packages/protocol/src/browser/modules/spdlog.ts @@ -3,6 +3,16 @@ import { ClientProxy } from "../../common/proxy"; import { RotatingLoggerProxy, SpdlogModuleProxy } from "../../node/modules/spdlog"; class RotatingLogger extends ClientProxy implements spdlog.RotatingLogger { + public constructor( + private readonly moduleProxy: SpdlogModuleProxy, + private readonly name: string, + private readonly filename: string, + private readonly filesize: number, + private readonly filecount: number, + ) { + super(moduleProxy.createLogger(name, filename, filesize, filecount)); + } + public async trace (message: string): Promise { this.proxy.trace(message); } public async debug (message: string): Promise { this.proxy.debug(message); } public async info (message: string): Promise { this.proxy.info(message); } @@ -13,6 +23,10 @@ class RotatingLogger extends ClientProxy implements spdlog. public async clearFormatters (): Promise { this.proxy.clearFormatters(); } public async flush (): Promise { this.proxy.flush(); } public async drop (): Promise { this.proxy.drop(); } + + protected handleDisconnect(): void { + this.initialize(this.moduleProxy.createLogger(this.name, this.filename, this.filesize, this.filecount)); + } } export class SpdlogModule { @@ -21,7 +35,7 @@ export class SpdlogModule { public constructor(private readonly proxy: SpdlogModuleProxy) { this.RotatingLogger = class extends RotatingLogger { public constructor(name: string, filename: string, filesize: number, filecount: number) { - super(proxy.createLogger(name, filename, filesize, filecount)); + super(proxy, name, filename, filesize, filecount); } }; } diff --git a/packages/protocol/src/browser/modules/stream.ts b/packages/protocol/src/browser/modules/stream.ts index f5db46445..c636cf326 100644 --- a/packages/protocol/src/browser/modules/stream.ts +++ b/packages/protocol/src/browser/modules/stream.ts @@ -81,6 +81,11 @@ export class Writable extends ClientPro } }); } + + protected handleDisconnect(): void { + this.emit("close"); + this.emit("finish"); + } } export class Readable extends ClientProxy implements stream.Readable { @@ -154,6 +159,11 @@ export class Readable extends ClientP return this; } + + protected handleDisconnect(): void { + this.emit("close"); + this.emit("end"); + } } export class Duplex extends Writable implements stream.Duplex, stream.Readable { @@ -230,4 +240,9 @@ export class Duplex extends Writable imp return this; } + + protected handleDisconnect(): void { + super.handleDisconnect(); + this.emit("end"); + } } diff --git a/packages/protocol/src/common/proxy.ts b/packages/protocol/src/common/proxy.ts index 6558ea2a4..4a9604e4e 100644 --- a/packages/protocol/src/common/proxy.ts +++ b/packages/protocol/src/common/proxy.ts @@ -29,21 +29,48 @@ const unpromisify = (proxyPromise: Promise): T => { * need a bunch of `then` calls everywhere. */ export abstract class ClientProxy extends EventEmitter { - protected readonly proxy: T; + private _proxy: T | undefined; /** * You can specify not to bind events in order to avoid emitting twice for * duplex streams. */ - public constructor(proxyPromise: Promise | T, bindEvents: boolean = true) { + public constructor( + proxyPromise: Promise | T, + private readonly bindEvents: boolean = true, + ) { super(); - this.proxy = isPromise(proxyPromise) ? unpromisify(proxyPromise) : proxyPromise; - if (bindEvents) { + this.initialize(proxyPromise); + if (this.bindEvents) { + this.on("disconnected", (error) => { + try { + this.emit("error", error); + } catch (error) { + // If nothing is listening, EventEmitter will throw an error. + } + this.handleDisconnect(); + }); + } + } + + protected get proxy(): T { + if (!this._proxy) { + throw new Error("not initialized"); + } + + return this._proxy; + } + + protected initialize(proxyPromise: Promise | T): void { + this._proxy = isPromise(proxyPromise) ? unpromisify(proxyPromise) : proxyPromise; + if (this.bindEvents) { this.proxy.onEvent((event, ...args): void => { this.emit(event, ...args); }); } } + + protected abstract handleDisconnect(): void; } /** diff --git a/packages/protocol/src/node/server.ts b/packages/protocol/src/node/server.ts index 238e88765..489d57115 100644 --- a/packages/protocol/src/node/server.ts +++ b/packages/protocol/src/node/server.ts @@ -140,7 +140,7 @@ export class Server { try { const proxy = this.getProxy(proxyId); if (typeof proxy.instance[method] !== "function") { - throw new Error(`"${method}" is not a function`); + throw new Error(`"${method}" is not a function on proxy ${proxyId}`); } response = proxy.instance[method](...args); diff --git a/packages/vscode/src/workbench.ts b/packages/vscode/src/workbench.ts index 4e023ed14..73fc77e96 100644 --- a/packages/vscode/src/workbench.ts +++ b/packages/vscode/src/workbench.ts @@ -30,6 +30,8 @@ import { ServiceCollection } from "vs/platform/instantiation/common/serviceColle import { URI } from "vs/base/common/uri"; export class Workbench { + public readonly retry = client.retry; + private readonly windowId = parseInt(new Date().toISOString().replace(/[-:.TZ]/g, ""), 10); private _serviceCollection: ServiceCollection | undefined; private _clipboardContextKey: RawContextKey | undefined; diff --git a/scripts/vscode.patch b/scripts/vscode.patch index 7b14fc7c1..b24e38f87 100644 --- a/scripts/vscode.patch +++ b/scripts/vscode.patch @@ -883,7 +883,7 @@ index acb68c8..bee143a 100644 - !isMacintosh || // macOS only + !browser.isMacintosh || // macOS only diff --git a/src/vs/workbench/electron-browser/workbench.ts b/src/vs/workbench/electron-browser/workbench.ts -index 7445d7b..0291dee 100644 +index 7445d7b..ba6bf4b 100644 --- a/src/vs/workbench/electron-browser/workbench.ts +++ b/src/vs/workbench/electron-browser/workbench.ts @@ -19 +19,2 @@ import { Registry } from 'vs/platform/registry/common/platform'; @@ -899,13 +899,15 @@ index 7445d7b..0291dee 100644 @@ -458 +462 @@ export class Workbench extends Disposable implements IPartService { - addClasses(document.body, platformClass); // used by our fonts + addClasses(document.body, platformClass, isWeb ? 'web' : 'native'); // used by our fonts -@@ -633 +637 @@ export class Workbench extends Disposable implements IPartService { +@@ -491,0 +496 @@ export class Workbench extends Disposable implements IPartService { ++ client.onClose(() => this.notificationService.error("Disconnected from shared process. Searching, installing, enabling, and disabling extensions will not work until the page is refreshed.")); +@@ -633 +638 @@ export class Workbench extends Disposable implements IPartService { - if (!isMacintosh && this.useCustomTitleBarStyle()) { + if (isWeb || (!isMacintosh && this.useCustomTitleBarStyle())) { -@@ -1241 +1245 @@ export class Workbench extends Disposable implements IPartService { +@@ -1241 +1246 @@ export class Workbench extends Disposable implements IPartService { - if ((isWindows || isLinux) && this.useCustomTitleBarStyle()) { + if ((isWeb || isWindows || isLinux) && this.useCustomTitleBarStyle()) { -@@ -1397 +1401 @@ export class Workbench extends Disposable implements IPartService { +@@ -1397 +1402 @@ export class Workbench extends Disposable implements IPartService { - } else if (isMacintosh) { + } else if (isNative && isMacintosh) { diff --git a/src/vs/workbench/services/extensions/electron-browser/cachedExtensionScanner.ts b/src/vs/workbench/services/extensions/electron-browser/cachedExtensionScanner.ts @@ -914,6 +916,18 @@ index 0592910..0ce7e35 100644 +++ b/src/vs/workbench/services/extensions/electron-browser/cachedExtensionScanner.ts @@ -33,0 +34 @@ function getSystemExtensionsRoot(): string { + return (require('vs/../../../../packages/vscode/src/fill/paths') as typeof import ('vs/../../../../packages/vscode/src/fill/paths')).getBuiltInExtensionsDirectory(); +diff --git a/src/vs/workbench/services/extensions/electron-browser/extensionService.ts b/src/vs/workbench/services/extensions/electron-browser/extensionService.ts +index 2c2f9c7..69fa321 100644 +--- a/src/vs/workbench/services/extensions/electron-browser/extensionService.ts ++++ b/src/vs/workbench/services/extensions/electron-browser/extensionService.ts +@@ -34,0 +35 @@ import { Schemas } from 'vs/base/common/network'; ++const retry = (require('vs/../../../../packages/vscode/src/workbench') as typeof import ('vs/../../../../packages/vscode/src/workbench')).workbench.retry; +@@ -117,0 +119 @@ export class ExtensionService extends Disposable implements IExtensionService { ++ retry.register('Extension Host', () => this.startExtensionHost()); +@@ -435,0 +438 @@ export class ExtensionService extends Disposable implements IExtensionService { ++ extHostProcessWorker.start().then(() => retry.recover('Extension Host')); +@@ -458,0 +462 @@ export class ExtensionService extends Disposable implements IExtensionService { ++ return retry.run('Extension Host'); diff --git a/src/vs/workbench/services/extensions/node/extensionHostProcess.ts b/src/vs/workbench/services/extensions/node/extensionHostProcess.ts index 484cef9..f728fc8 100644 --- a/src/vs/workbench/services/extensions/node/extensionHostProcess.ts @@ -921,6 +935,46 @@ index 484cef9..f728fc8 100644 @@ -137 +137 @@ function connectToRenderer(protocol: IMessagePassingProtocol): Promise this.startWatching()); +@@ -56,0 +59,2 @@ export class FileWatcher { ++ this.toDispose = dispose(this.toDispose); ++ return retry.run('Watcher'); +@@ -113 +117 @@ export class FileWatcher { +- })); ++ })).then(() => retry.recover('Watcher')); +diff --git a/src/vs/workbench/services/files/node/watcher/unix/watcherService.ts b/src/vs/workbench/services/files/node/watcher/unix/watcherService.ts +index 7e3a324..b9ccd63 100644 +--- a/src/vs/workbench/services/files/node/watcher/unix/watcherService.ts ++++ b/src/vs/workbench/services/files/node/watcher/unix/watcherService.ts +@@ -18,0 +19 @@ import { getPathFromAmdModule } from 'vs/base/common/amd'; ++const retry = (require('vs/../../../../packages/vscode/src/workbench') as typeof import ('vs/../../../../packages/vscode/src/workbench')).workbench.retry; +@@ -36,0 +38 @@ export class FileWatcher { ++ retry.register('Watcher', () => this.startWatching()); +@@ -59,0 +62,2 @@ export class FileWatcher { ++ this.toDispose = dispose(this.toDispose); ++ return retry.run('Watcher'); +@@ -116 +120 @@ export class FileWatcher { +- })); ++ })).then(() => retry.recover('Watcher')); +diff --git a/src/vs/workbench/services/files/node/watcher/win32/csharpWatcherService.ts b/src/vs/workbench/services/files/node/watcher/win32/csharpWatcherService.ts +index 74dad64..34cd83b 100644 +--- a/src/vs/workbench/services/files/node/watcher/win32/csharpWatcherService.ts ++++ b/src/vs/workbench/services/files/node/watcher/win32/csharpWatcherService.ts +@@ -14,0 +15 @@ import { getPathFromAmdModule } from 'vs/base/common/amd'; ++const retry = (require('vs/../../../../packages/vscode/src/workbench') as typeof import ('vs/../../../../packages/vscode/src/workbench')).workbench.retry; +@@ -40,0 +42 @@ export class OutOfProcessWin32FolderWatcher { ++ retry.register('Watcher', () => this.startWatcher()); +@@ -52,0 +55 @@ export class OutOfProcessWin32FolderWatcher { ++ this.handle.stdout.once('data', () => retry.recover('Watcher')); +@@ -110,0 +114 @@ export class OutOfProcessWin32FolderWatcher { ++ return retry.run('Watcher'); diff --git a/src/vs/workbench/services/keybinding/electron-browser/keybindingService.ts b/src/vs/workbench/services/keybinding/electron-browser/keybindingService.ts index 3c78990..545d91a 100644 --- a/src/vs/workbench/services/keybinding/electron-browser/keybindingService.ts @@ -931,6 +985,40 @@ index 3c78990..545d91a 100644 @@ -130 +130 @@ export class KeyboardMapperFactory { - if (OS === OperatingSystem.Windows) { + if (isNative && OS === OperatingSystem.Windows) { +diff --git a/src/vs/workbench/services/search/node/searchService.ts b/src/vs/workbench/services/search/node/searchService.ts +index 3eaafa4..0345bad 100644 +--- a/src/vs/workbench/services/search/node/searchService.ts ++++ b/src/vs/workbench/services/search/node/searchService.ts +@@ -11 +11 @@ import { Event } from 'vs/base/common/event'; +-import { Disposable, IDisposable, toDisposable } from 'vs/base/common/lifecycle'; ++import { Disposable, IDisposable, toDisposable, dispose } from 'vs/base/common/lifecycle'; +@@ -32,0 +33 @@ import { IConfigurationService } from 'vs/platform/configuration/common/configur ++const retry = (require('vs/../../../../packages/vscode/src/workbench') as typeof import ('vs/../../../../packages/vscode/src/workbench')).workbench.retry; +@@ -433,0 +435 @@ export class DiskSearch implements ISearchResultProvider { ++ private toDispose: IDisposable[] = []; +@@ -470,6 +472,16 @@ export class DiskSearch implements ISearchResultProvider { +- const client = new Client( +- getPathFromAmdModule(require, 'bootstrap-fork'), +- opts); +- +- const channel = getNextTickChannel(client.getChannel('search')); +- this.raw = new SearchChannelClient(channel); ++ const connect = (): void => { ++ const client = new Client( ++ getPathFromAmdModule(require, 'bootstrap-fork'), ++ opts); ++ client.onDidProcessExit(() => { ++ this.toDispose = dispose(this.toDispose); ++ retry.run('Searcher'); ++ }, null, this.toDispose); ++ this.toDispose.push(client); ++ ++ const channel = getNextTickChannel(client.getChannel('search')); ++ this.raw = new SearchChannelClient(channel); ++ this.raw.clearCache('test-connectivity').then(() => retry.recover('Searcher')); ++ }; ++ retry.register('Searcher', connect); ++ connect(); diff --git a/src/vs/workbench/services/timer/electron-browser/timerService.ts b/src/vs/workbench/services/timer/electron-browser/timerService.ts index 6e6fbcc..645bd72 100644 --- a/src/vs/workbench/services/timer/electron-browser/timerService.ts