Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Running heavy filesystem operations in separate processes #899

Merged
merged 5 commits into from
Dec 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ examples/*/webpack.config.js
.browser_modules
**/docs/api
package-backup.json
.history
.history
.Trash-*
2 changes: 1 addition & 1 deletion dev-packages/application-package/src/rebuild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import cp = require('child_process');
export function rebuild(target: 'electron' | 'browser', modules: string[]) {
const nodeModulesPath = path.join(process.cwd(), 'node_modules');
const browserModulesPath = path.join(process.cwd(), '.browser_modules');
const modulesToProcess = modules || ['node-pty'];
const modulesToProcess = modules || ['node-pty', 'nsfw', 'find-git-repositories'];

if (target === 'electron' && !fs.existsSync(browserModulesPath)) {
const dependencies: {
Expand Down
4 changes: 2 additions & 2 deletions examples/browser/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
"build": "theia build",
"watch": "yarn build --watch",
"start": "theia start",
"start:debug": "yarn start --loglevel=debug",
"start:debug": "yarn start --logLevel=debug",
"test": "wdio",
"coverage:compile": "yarn build --config coverage-webpack.config.js",
"coverage:remap": "remap-istanbul -i coverage/coverage.json -o coverage/coverage-final.json --exclude 'frontend/index.js' && rimraf coverage/coverage.json",
Expand All @@ -43,4 +43,4 @@
"devDependencies": {
"@theia/cli": "^0.2.2"
}
}
}
2 changes: 2 additions & 0 deletions packages/core/src/browser/tree/tree.ts
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,8 @@ export class Tree implements ITree {
if (ICompositeTreeNode.is(parent)) {
this.resolveChildren(parent).then(children => this.setChildren(parent, children));
}
// FIXME: it shoud not be here
// if the idea was to support refreshing of all kind of nodes, then API should be adapted
this.fireChanged();
}

Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/common/disposable.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,19 @@ export class DisposableCollection implements Disposable {
push(disposable: Disposable): Disposable {
const disposables = this.disposables;
disposables.push(disposable);
return Disposable.create(() => {
const originalDispose = disposable.dispose.bind(disposable);
const toRemove = Disposable.create(() => {
const index = disposables.indexOf(disposable);
if (index !== -1) {
disposables.splice(index, 1);
}
this.checkDisposed();
});
disposable.dispose = () => {
toRemove.dispose();
originalDispose();
};
return toRemove;
}

pushAll(disposables: Disposable[]): Disposable[] {
Expand Down
65 changes: 65 additions & 0 deletions packages/core/src/common/messaging/connection-error-handler.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
* Copyright (C) 2017 TypeFox and others.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License.
* You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
*/

import { Message } from "vscode-jsonrpc";
import { ILogger } from "../../common";

export interface ResolvedConnectionErrorHandlerOptions {
readonly serverName: string
readonly logger: ILogger
/**
* The maximum amout of errors allowed before stopping the server.
*/
readonly maxErrors: number
/**
* The maimum amount of restarts allowed in the restart interval.
*/
readonly maxRestarts: number
/**
* In minutes.
*/
readonly restartInterval: number
}

export type ConnectionErrorHandlerOptions = Partial<ResolvedConnectionErrorHandlerOptions> & {
readonly serverName: string
readonly logger: ILogger
};

export class ConnectionErrorHandler {

protected readonly options: ResolvedConnectionErrorHandlerOptions;
constructor(options: ConnectionErrorHandlerOptions) {
this.options = {
maxErrors: 3,
maxRestarts: 5,
restartInterval: 3,
...options
};
}

shouldStop(error: Error, message?: Message, count?: number): boolean {
return !count || count > this.options.maxErrors;
}

protected readonly restarts: number[] = [];
shouldRestart(): boolean {
this.restarts.push(Date.now());
if (this.restarts.length <= this.options.maxRestarts) {
return true;
}
const diff = this.restarts[this.restarts.length - 1] - this.restarts[0];
if (diff <= this.options.restartInterval * 60 * 1000) {
// tslint:disable-next-line:max-line-length
this.options.logger.error(`The ${this.options.serverName} server crashed ${this.options.maxRestarts} times in the last ${this.options.restartInterval} minutes. The server will not be restarted.`);
return false;
}
this.restarts.shift();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will leak restarts we should always discard all restarts < last restart - interval
We should also do this all the time before return.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There never will be restarts more than maxRestarts. After maxRestarts reached for each push there is one shift or a server should be stopped completely.

return true;
}

}
1 change: 1 addition & 0 deletions packages/core/src/common/messaging/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@

export * from './handler';
export * from './proxy-factory';
export * from './connection-error-handler';
3 changes: 3 additions & 0 deletions packages/core/src/node/backend-application-module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { MessageClient, DispatchingMessageClient, messageServicePath } from '../
import { BackendApplication, BackendApplicationContribution, BackendApplicationCliContribution } from './backend-application';
import { CliManager, CliContribution } from './cli';
import { ServerProcess, RemoteMasterProcessFactory, clusterRemoteMasterProcessFactory } from './cluster';
import { IPCConnectionProvider } from "./messaging";

export function bindServerProcess(bind: interfaces.Bind, masterFactory: RemoteMasterProcessFactory): void {
bind(RemoteMasterProcessFactory).toConstantValue(masterFactory);
Expand Down Expand Up @@ -41,4 +42,6 @@ export const backendApplicationModule = new ContainerModule(bind => {
})
).inSingletonScope();
bind(MessageService).toSelf().inSingletonScope();

bind(IPCConnectionProvider).toSelf().inSingletonScope();
});
1 change: 1 addition & 0 deletions packages/core/src/node/messaging/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
*/

export * from './connection';
export * from './ipc-connection-provider';
41 changes: 41 additions & 0 deletions packages/core/src/node/messaging/ipc-bootstrap.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright (C) 2017 TypeFox and others.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License.
* You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
*/

import 'reflect-metadata';
import { ConsoleLogger } from 'vscode-ws-jsonrpc/lib/logger';
import { createMessageConnection, IPCMessageReader, IPCMessageWriter, Trace } from 'vscode-jsonrpc';
import { THEIA_PARENT_PID, THEIA_ENTRY_POINT, IPCEntryPoint } from './ipc-protocol';

/**
* Exit the current process if the parent process is not alive.
* Relevant only for some OS, like Windows
*/
if (process.env[THEIA_PARENT_PID]) {
const parentPid = Number(process.env[THEIA_PARENT_PID]);

if (typeof parentPid === 'number' && !isNaN(parentPid)) {
setInterval(function () {
try {
// throws an exception if the main process doesn't exist anymore.
process.kill(parentPid, 0);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ChildProcess object may emit an 'error' event if the signal cannot be delivered. Sending a signal to a child process that has already exited is not an error but may have unforeseen consequences. Specifically, if the process identifier (PID) has been reassigned to another process, the signal will be delivered to that process instead which can have unexpected results.

From: https://nodejs.org/dist/latest-v8.x/docs/api/child_process.html#child_process_subprocess_kill_signal

Moreover 0 is not a valid signal number on linux at least, not sure what is supposed to happen.

The parent should just notify the child as it exists no ?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think similar work was done for the LSPs recently with onStop() ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code is copied from vscode, they use it to ensure that child processes exit if the main process crashed and did not kill children, make sense only on windows, other OSs take care about child processes

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thx could you add a comment in the code about it please? it being windows specific?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note I don't think child processes exit when the parent exists on linux. You have to do it yourself like in the LSP case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will add, so far we experience it only on windows

} catch (e) {
process.exit();
}
}, 5000);
}
}

const reader = new IPCMessageReader(process);
const writer = new IPCMessageWriter(process);
const logger = new ConsoleLogger();
const connection = createMessageConnection(reader, writer, logger);
connection.trace(Trace.Off, {
log: (message, data) => console.log(`${message} ${data}`)
});

const entryPoint = require(process.env[THEIA_ENTRY_POINT]!).default as IPCEntryPoint;
entryPoint(connection);
110 changes: 110 additions & 0 deletions packages/core/src/node/messaging/ipc-connection-provider.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/*
* Copyright (C) 2017 TypeFox and others.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License.
* You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
*/

import * as path from "path";
import * as cp from "child_process";
import { injectable, inject } from "inversify";
import { Trace, IPCMessageReader, IPCMessageWriter, createMessageConnection, MessageConnection, Message } from "vscode-jsonrpc";
import { ILogger, ConnectionErrorHandler, DisposableCollection, Disposable } from "../../common";
import { THEIA_PARENT_PID, THEIA_ENTRY_POINT } from './ipc-protocol';

export interface ResolvedIPCConnectionOptions {
readonly serverName: string
readonly entryPoint: string
readonly logger: ILogger
readonly args: string[]
readonly debug?: number
readonly debugBrk?: number
readonly errorHandler?: ConnectionErrorHandler
}
export type IPCConnectionOptions = Partial<ResolvedIPCConnectionOptions> & {
readonly serverName: string
readonly entryPoint: string
};

@injectable()
export class IPCConnectionProvider {

@inject(ILogger)
protected readonly logger: ILogger;

listen(options: IPCConnectionOptions, acceptor: (connection: MessageConnection) => void): Disposable {
return this.doListen({
logger: this.logger,
args: [],
...options
}, acceptor);
}

protected doListen(options: ResolvedIPCConnectionOptions, acceptor: (connection: MessageConnection) => void): Disposable {
const childProcess = this.fork(options);
const connection = this.createConnection(childProcess, options);
const toStop = new DisposableCollection();
const toCancelStop = toStop.push(Disposable.create(() => childProcess.kill()));
const errorHandler = options.errorHandler;
if (errorHandler) {
connection.onError((e: [Error, Message | undefined, number | undefined]) => {
if (errorHandler.shouldStop(e[0], e[1], e[2])) {
toStop.dispose();
}
});
connection.onClose(() => {
if (toStop.disposed) {
return;
}
if (errorHandler.shouldRestart()) {
toCancelStop.dispose();
toStop.push(this.doListen(options, acceptor));
}
});
}
acceptor(connection);
return toStop;
}

protected createConnection(childProcess: cp.ChildProcess, options: ResolvedIPCConnectionOptions): MessageConnection {
const reader = new IPCMessageReader(childProcess);
const writer = new IPCMessageWriter(childProcess);
const connection = createMessageConnection(reader, writer, {
error: (message: string) => this.logger.error(`[${options.serverName}: ${childProcess.pid}] ${message}`),
warn: (message: string) => this.logger.warn(`[${options.serverName}: ${childProcess.pid}] ${message}`),
info: (message: string) => this.logger.info(`[${options.serverName}: ${childProcess.pid}] ${message}`),
log: (message: string) => this.logger.info(`[${options.serverName}: ${childProcess.pid}] ${message}`)
});
connection.trace(Trace.Off, {
log: (message, data) => this.logger.info(`[${options.serverName}: ${childProcess.pid}] ${message} ${data}`)
});
return connection;
}

protected fork(options: ResolvedIPCConnectionOptions): cp.ChildProcess {
const forkOptions: cp.ForkOptions = {
silent: true,
env: {
...process.env
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just asking to squeeze some additional knowledge from you: it is the same as env: process.env, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it copies properties to a new object, later 2 more properties added to the new object but not to process.env

execArgv: []
};
forkOptions.env[THEIA_PARENT_PID] = String(process.pid);
forkOptions.env[THEIA_ENTRY_POINT] = options.entryPoint;
if (typeof options.debug === 'number') {
forkOptions.execArgv = ['--nolazy', '--inspect=' + options.debug];
}
if (typeof options.debugBrk === 'number') {
forkOptions.execArgv = ['--nolazy', '--inspect-brk=' + options.debugBrk];
}
const childProcess = cp.fork(path.resolve(__dirname, 'ipc-bootstrap.js'), options.args, forkOptions);
childProcess.stdout.on('data', data => this.logger.info(`[${options.serverName}: ${childProcess.pid}] ${data.toString()}`));
childProcess.stderr.on('data', data => this.logger.error(`[${options.serverName}: ${childProcess.pid}] ${data.toString()}`));

this.logger.debug(`[${options.serverName}: ${childProcess.pid}] IPC started`);
childProcess.once('exit', () => this.logger.debug(`[${options.serverName}: ${childProcess.pid}] IPC exited`));

return childProcess;
}

}
14 changes: 14 additions & 0 deletions packages/core/src/node/messaging/ipc-protocol.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@

/*
* Copyright (C) 2017 TypeFox and others.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License.
* You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
*/

import { MessageConnection } from "vscode-jsonrpc";

export const THEIA_PARENT_PID = 'THEIA_PARENT_PID';
export const THEIA_ENTRY_POINT = 'THEIA_ENTRY_POINT';

export type IPCEntryPoint = (connection: MessageConnection) => void;
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ import { ILoggerServer } from "@theia/core/lib/common/logger-protocol";
import { stubRemoteMasterProcessFactory } from "@theia/core/lib/node";
import { bindServerProcess } from "@theia/core/lib/node/backend-application-module";
import { bindLogger } from "@theia/core/lib/node/logger-backend-module";
import { bindFileSystem, bindFileSystemWatcherServer } from "@theia/filesystem/lib/node/filesystem-backend-module";
import { bindFileSystem } from "@theia/filesystem/lib/node/filesystem-backend-module";
import { FileSystemWatcherServer } from "@theia/filesystem/lib/common/filesystem-watcher-protocol";
import { NsfwFileSystemWatcherServer } from "@theia/filesystem/lib/node/nsfw-watcher/nsfw-filesystem-watcher";
import { ApplicationProjectArgs } from "../application-project-cli";
import { bindNodeExtensionServer } from '../extension-backend-module';

Expand All @@ -22,7 +24,7 @@ export const extensionNodeTestContainer = (args: ApplicationProjectArgs) => {
bindServerProcess(bind, stubRemoteMasterProcessFactory);
container.rebind(ILoggerServer).to(ConsoleLoggerServer).inSingletonScope();
bindFileSystem(bind);
bindFileSystemWatcherServer(bind);
bind(FileSystemWatcherServer).toConstantValue(new NsfwFileSystemWatcherServer());
bindNodeExtensionServer(bind, args);
return container;
};
Expand Down
6 changes: 3 additions & 3 deletions packages/filesystem/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@
"dependencies": {
"@theia/core": "^0.2.3",
"@theia/preferences-api": "^0.2.3",
"@types/chokidar": "^1.7.0",
"@types/fs-extra": "^4.0.2",
"@types/touch": "0.0.1",
"chokidar": "^1.7.0",
"fs-extra": "^4.0.2",
"mv": "^2.1.1",
"nsfw": "^1.0.16",
"touch": "^3.1.0",
"trash": "^4.0.1"
},
Expand Down Expand Up @@ -45,6 +44,7 @@
"build": "theiaext build",
"watch": "theiaext watch",
"test": "theiaext test",
"test:watch": "theiaext test:watch",
"docs": "theiaext docs"
},
"devDependencies": {
Expand All @@ -53,4 +53,4 @@
"nyc": {
"extends": "../../configs/nyc.json"
}
}
}
2 changes: 0 additions & 2 deletions packages/filesystem/src/common/filesystem-preferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,10 @@ export function createFileSystemPreferences(preferences: PreferenceService): Fil
}

export function bindFileSystemPreferences(bind: interfaces.Bind): void {

bind(FileSystemPreferences).toDynamicValue(ctx => {
const preferences = ctx.container.get(PreferenceService);
return createFileSystemPreferences(preferences);
});

bind(PreferenceContribution).toConstantValue({ schema: filesystemPreferenceSchema });

}
Loading