diff --git a/.eslintrc b/.eslintrc index 7da33e7bc0..f493260df1 100644 --- a/.eslintrc +++ b/.eslintrc @@ -20,6 +20,7 @@ "MouseEvent": true, "IDBKeyRange": true, "beforeEach": true, + "after": true, "afterEach": true, "describe": true, "fdescribe": true, diff --git a/lib/async-queue.js b/lib/async-queue.js index e2579dc22c..cfcef0bc86 100644 --- a/lib/async-queue.js +++ b/lib/async-queue.js @@ -39,9 +39,6 @@ export default class AsyncQueue { } push(fn, {parallel} = {parallel: true}) { - if (this.disposed) { - throw new Error('AsyncQueue is disposed'); - } const task = new Task(fn, parallel); this.queue.push(task); this.processQueue(); diff --git a/lib/controllers/git-tab-controller.js b/lib/controllers/git-tab-controller.js index 6f043a970a..e74813c925 100644 --- a/lib/controllers/git-tab-controller.js +++ b/lib/controllers/git-tab-controller.js @@ -226,7 +226,7 @@ export default class GitTabController { const pathsToIgnore = []; const repository = this.getActiveRepository(); for (const filePath of filePaths) { - if (await repository.pathHasMergeMarkers(filePath)) { // eslint-disable-line babel/no-await-in-loop + if (await repository.pathHasMergeMarkers(filePath)) { // eslint-disable-line no-await-in-loop const choice = atom.confirm({ message: 'File contains merge markers: ', detailedMessage: `Do you still want to stage this file?\n${filePath}`, diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index b45c025c63..68aa38b8b1 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -2,14 +2,14 @@ import path from 'path'; import os from 'os'; import {CompositeDisposable} from 'event-kit'; - import {GitProcess} from 'dugite'; import {parse as parseDiff} from 'what-the-diff'; import GitPromptServer from './git-prompt-server'; import AsyncQueue from './async-queue'; -import {getPackageRoot, getDugitePath, readFile, fileExists, writeFile, isFileExecutable} from './helpers'; +import {getPackageRoot, getDugitePath, readFile, fileExists, fsStat, writeFile, isFileExecutable} from './helpers'; import GitTimingsView from './views/git-timings-view'; +import WorkerManager from './worker-manager'; const LINE_ENDING_REGEX = /\r?\n/; @@ -57,6 +57,7 @@ export default class GitShellOutStrategy { } this.prompt = options.prompt || (query => Promise.reject()); + this.workerManager = options.workerManager; } /* @@ -131,6 +132,7 @@ export default class GitShellOutStrategy { const options = { env, processCallback: child => { + // TODO: move callback to renderer process. send child.pid back to add cancel listener child.on('error', err => { console.warn('Error executing: ' + formattedArgs + ':'); console.warn(err.stack); @@ -156,51 +158,68 @@ export default class GitShellOutStrategy { if (process.env.PRINT_GIT_TIMES) { console.time(`git:${formattedArgs}`); } - return new Promise(resolve => { - timingMarker.mark('nexttick'); - setImmediate(() => { - timingMarker.mark('execute'); - resolve(GitProcess.exec(args, this.workingDir, options) - .then(({stdout, stderr, exitCode}) => { - timingMarker.finalize(); - if (process.env.PRINT_GIT_TIMES) { - console.timeEnd(`git:${formattedArgs}`); - } - if (gitPromptServer) { - gitPromptServer.terminate(); - } - subscriptions.dispose(); - - if (diagnosticsEnabled) { - const headerStyle = 'font-weight: bold; color: blue;'; - - console.groupCollapsed(`git:${formattedArgs}`); - console.log('%cexit status%c %d', headerStyle, 'font-weight: normal; color: black;', exitCode); - console.log('%cstdout', headerStyle); - console.log(stdout); - console.log('%cstderr', headerStyle); - console.log(stderr); - console.groupEnd(); - } - - if (exitCode) { - const err = new GitError( - `${formattedArgs} exited with code ${exitCode}\nstdout: ${stdout}\nstderr: ${stderr}`, - ); - err.code = exitCode; - err.stdErr = stderr; - err.stdOut = stdout; - err.command = formattedArgs; - return Promise.reject(err); - } - return stdout; - })); - }); + return new Promise(async (resolve, reject) => { + const {stdout, stderr, exitCode, timing} = await this.executeGitCommand(args, options, timingMarker); + if (timing) { + const {execTime, spawnTime, ipcTime} = timing; + const now = performance.now(); + timingMarker.mark('nexttick', now - execTime - spawnTime - ipcTime); + timingMarker.mark('execute', now - execTime - ipcTime); + timingMarker.mark('ipc', now - ipcTime); + } + timingMarker.finalize(); + if (process.env.PRINT_GIT_TIMES) { + console.timeEnd(`git:${formattedArgs}`); + } + if (gitPromptServer) { + gitPromptServer.terminate(); + } + subscriptions.dispose(); + + if (diagnosticsEnabled) { + const headerStyle = 'font-weight: bold; color: blue;'; + + console.groupCollapsed(`git:${formattedArgs}`); + console.log('%cexit status%c %d', headerStyle, 'font-weight: normal; color: black;', exitCode); + console.log('%cstdout', headerStyle); + console.log(stdout); + console.log('%cstderr', headerStyle); + console.log(stderr); + console.groupEnd(); + } + + if (exitCode) { + const err = new GitError( + `${formattedArgs} exited with code ${exitCode}\nstdout: ${stdout}\nstderr: ${stderr}`, + ); + err.code = exitCode; + err.stdErr = stderr; + err.stdOut = stdout; + err.command = formattedArgs; + reject(err); + } + resolve(stdout); }); }, {parallel: !writeOperation}); /* eslint-enable no-console */ } + executeGitCommand(args, options, marker = null) { + if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC || !WorkerManager.getInstance().isReady()) { + marker && marker.mark('nexttick'); + const promise = GitProcess.exec(args, this.workingDir, options); + marker && marker.mark('execute'); + return promise; + } else { + const workerManager = this.workerManager || WorkerManager.getInstance(); + return workerManager.request({ + args, + workingDir: this.workingDir, + options, + }); + } + } + /** * Execute a git command that may create a commit. If the command fails because the GPG binary was invoked and unable * to acquire a passphrase (because the pinentry program attempted to use a tty), retry with a `GitPromptServer`. @@ -220,6 +239,7 @@ export default class GitShellOutStrategy { async isGitRepository() { try { + await fsStat(this.workingDir); // fails if folder doesn't exist await this.exec(['rev-parse', '--resolve-git-dir', path.join(this.workingDir, '.git')]); return true; } catch (e) { diff --git a/lib/github-package.js b/lib/github-package.js index b5f473f435..d6d06f0d77 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -19,6 +19,7 @@ import Switchboard from './switchboard'; import yardstick from './yardstick'; import GitTimingsView from './views/git-timings-view'; import AsyncQueue from './async-queue'; +import WorkerManager from './worker-manager'; const defaultState = { firstRun: true, @@ -216,6 +217,7 @@ export default class GithubPackage { this.subscriptions.dispose(); this.contextPool.clear(); await yardstick.flush(); + WorkerManager.reset(); } @autobind diff --git a/lib/renderer.html b/lib/renderer.html new file mode 100644 index 0000000000..ea43dbef4d --- /dev/null +++ b/lib/renderer.html @@ -0,0 +1,19 @@ + + +
+ ++ Hi there! I'm a window used by the GitHub package to execute Git commands in the background. My PID is . +
+Last command:
+ + diff --git a/lib/views/git-timings-view.js b/lib/views/git-timings-view.js index bb5ed0c775..3def6953e0 100644 --- a/lib/views/git-timings-view.js +++ b/lib/views/git-timings-view.js @@ -43,8 +43,8 @@ class Marker { return this.end; } - mark(sectionName) { - this.markers.push({name: sectionName, start: performance.now()}); + mark(sectionName, start) { + this.markers.push({name: sectionName, start: start || performance.now()}); } finalize() { @@ -98,6 +98,7 @@ const COLORS = { prepare: 'cyan', nexttick: 'yellow', execute: 'green', + ipc: 'pink', }; class MarkerSpan extends React.Component { static propTypes = { diff --git a/lib/worker-manager.js b/lib/worker-manager.js new file mode 100644 index 0000000000..91f4105873 --- /dev/null +++ b/lib/worker-manager.js @@ -0,0 +1,383 @@ +import path from 'path'; +import querystring from 'querystring'; + +import {remote, ipcRenderer as ipc} from 'electron'; +const {BrowserWindow} = remote; +import {Emitter, Disposable, CompositeDisposable} from 'event-kit'; +import {autobind} from 'core-decorators'; + +import {getPackageRoot} from './helpers'; + +export default class WorkerManager { + static instance = null; + + static getInstance() { + if (!this.instance) { + this.instance = new WorkerManager(); + } + return this.instance; + } + + static reset() { + if (this.instance) { this.instance.destroy(); } + this.instance = null; + } + + constructor() { + this.workers = new Set(); + this.activeWorker = null; + this.createNewWorker(); + } + + isReady() { + return this.activeWorker.isReady(); + } + + request(data) { + if (this.destroyed) { throw new Error('Worker is destroyed'); } + let operation; + const requestPromise = new Promise(resolve => { + operation = new Operation(data, resolve); + return this.activeWorker.executeOperation(operation); + }); + operation.setPromise(requestPromise); + return requestPromise; + } + + createNewWorker({operationCountLimit} = {operationCountLimit: 10}) { + if (this.destroyed) { return; } + this.activeWorker = new Worker({ + operationCountLimit, + onDestroyed: this.onDestroyed, + onCrashed: this.onCrashed, + onSick: this.onSick, + }); + this.workers.add(this.activeWorker); + } + + @autobind + onDestroyed(destroyedWorker) { + this.workers.delete(destroyedWorker); + } + + @autobind + onCrashed(crashedWorker) { + if (crashedWorker === this.getActiveWorker()) { + this.createNewWorker({operationCountLimit: crashedWorker.getOperationCountLimit()}); + } + crashedWorker.getRemainingOperations().forEach(operation => this.activeWorker.executeOperation(operation)); + } + + @autobind + onSick(sickWorker) { + if (!atom.inSpecMode()) { + // eslint-disable-next-line no-console + console.warn(`Sick worker detected. + operationCountLimit: ${sickWorker.getOperationCountLimit()}, + completed operation count: ${sickWorker.getCompletedOperationCount()}`); + } + const operationCountLimit = this.calculateNewOperationCountLimit(sickWorker); + return this.createNewWorker({operationCountLimit}); + } + + calculateNewOperationCountLimit(lastWorker) { + let operationCountLimit = 10; + if (lastWorker.getOperationCountLimit() >= lastWorker.getCompletedOperationCount()) { + operationCountLimit = Math.min(lastWorker.getOperationCountLimit() * 2, 100); + } + return operationCountLimit; + } + + getActiveWorker() { + return this.activeWorker; + } + + getReadyPromise() { + return this.activeWorker.getReadyPromise(); + } + + destroy(force) { + this.destroyed = true; + this.workers.forEach(worker => worker.destroy(force)); + } +} + + +export class Worker { + static channelName = 'github:renderer-ipc'; + + constructor({operationCountLimit, onSick, onCrashed, onDestroyed}) { + this.operationCountLimit = operationCountLimit; + this.onSick = onSick; + this.onCrashed = onCrashed; + this.onDestroyed = onDestroyed; + + this.operationsById = new Map(); + this.completedOperationCount = 0; + this.sick = false; + + this.rendererProcess = new RendererProcess({ + loadUrl: this.getLoadUrl(operationCountLimit), + onData: this.handleDataReceived, + onExecStarted: this.handleExecStarted, + onSick: this.handleSick, + onCrashed: this.handleCrashed, + onDestroyed: this.destroy, + }); + } + + isReady() { + return this.rendererProcess.isReady(); + } + + getLoadUrl(operationCountLimit) { + const htmlPath = path.join(getPackageRoot(), 'lib', 'renderer.html'); + const rendererJsPath = path.join(getPackageRoot(), 'lib', 'worker.js'); + const qs = querystring.stringify({ + js: rendererJsPath, + managerWebContentsId: this.getWebContentsId(), + operationCountLimit, + channelName: Worker.channelName, + }); + return `file://${htmlPath}?${qs}`; + } + + getWebContentsId() { + return remote.getCurrentWebContents().id; + } + + executeOperation(operation) { + this.operationsById.set(operation.id, operation); + operation.onComplete(this.onOperationComplete); + return this.rendererProcess.executeOperation(operation); + } + + @autobind + handleDataReceived({id, results}) { + const operation = this.operationsById.get(id); + operation.complete(results, data => { + const {timing} = data; + const totalInternalTime = timing.execTime + timing.spawnTime; + const ipcTime = operation.getExecutionTime() - totalInternalTime; + data.timing.ipcTime = ipcTime; + return data; + }); + } + + @autobind + onOperationComplete(operation) { + this.completedOperationCount++; + this.operationsById.delete(operation.id); + + if (this.sick && this.operationsById.size === 0) { + this.destroy(); + } + } + + @autobind + handleExecStarted({id}) { + const operation = this.operationsById.get(id); + operation.setInProgress(); + } + + @autobind + handleSick() { + this.sick = true; + this.onSick(this); + } + + @autobind + handleCrashed() { + this.onCrashed(this); + this.destroy(); + } + + getOperationCountLimit() { + return this.operationCountLimit; + } + + getCompletedOperationCount() { + return this.completedOperationCount; + } + + getRemainingOperations() { + return Array.from(this.operationsById.values()); + } + + getPid() { + return this.rendererProcess.getPid(); + } + + getReadyPromise() { + return this.rendererProcess.getReadyPromise(); + } + + async destroy(force) { + this.onDestroyed(this); + if (this.operationsById.size > 0 && !force) { + const remainingOperationPromises = this.getRemainingOperations() + .map(operation => operation.getPromise().catch(() => null)); + await Promise.all(remainingOperationPromises); + } + this.rendererProcess.destroy(); + } +} + + +/* +Sends operations to renderer processes +*/ +export class RendererProcess { + constructor({loadUrl, onDestroyed, onCrashed, onSick, onData, onExecStarted}) { + this.onDestroyed = onDestroyed; + this.onCrashed = onCrashed; + this.onSick = onSick; + this.onData = onData; + this.onExecStarted = onExecStarted; + + this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); + this.webContents = this.win.webContents; + // this.webContents.openDevTools(); + + this.emitter = new Emitter(); + this.subscriptions = new CompositeDisposable(); + this.registerListeners(); + + this.win.loadURL(loadUrl); + this.win.webContents.on('crashed', this.handleDestroy); + this.win.webContents.on('destroyed', this.handleDestroy); + this.subscriptions.add( + new Disposable(() => { + if (!this.win.isDestroyed()) { + this.win.webContents.removeListener('crashed', this.handleDestroy); + this.win.webContents.removeListener('destroyed', this.handleDestroy); + this.win.destroy(); + } + }), + this.emitter, + ); + + this.ready = false; + this.readyPromise = new Promise(resolve => { this.resolveReady = resolve; }); + } + + isReady() { + return this.ready; + } + + @autobind + handleDestroy(...args) { + this.destroy(); + this.onCrashed(...args); + } + + registerListeners() { + const handleMessages = (event, {sourceWebContentsId, type, data}) => { + if (sourceWebContentsId === this.win.webContents.id) { + this.emitter.emit(type, data); + } + }; + + ipc.on(Worker.channelName, handleMessages); + this.emitter.on('renderer-ready', ({pid}) => { + this.pid = pid; + this.ready = true; + this.resolveReady(); + }); + this.emitter.on('git-data', this.onData); + this.emitter.on('slow-spawns', this.onSick); + + // not currently used to avoid clogging up ipc channel + // keeping it around as it's potentially useful for avoiding duplicate write operations upon renderer crashing + this.emitter.on('exec-started', this.onExecStarted); + + this.subscriptions.add( + new Disposable(() => ipc.removeListener(Worker.channelName, handleMessages)), + ); + } + + executeOperation(operation) { + return operation.execute(payload => { + if (this.destroyed) { return null; } + return this.webContents.send(Worker.channelName, { + type: 'git-exec', + data: payload, + }); + }); + } + + getPid() { + return this.pid; + } + + getReadyPromise() { + return this.readyPromise; + } + + destroy() { + this.destroyed = true; + this.subscriptions.dispose(); + } +} + + +export class Operation { + static status = { + PENDING: Symbol('pending'), + INPROGRESS: Symbol('in-progress'), + COMPLETE: Symbol('complete'), + } + + static id = 0; + + constructor(data, resolve) { + this.id = Operation.id++; + this.data = data; + this.resolve = resolve; + this.promise = null; + this.startTime = null; + this.endTime = null; + this.status = Operation.status.PENDING; + this.results = null; + this.emitter = new Emitter(); + } + + onComplete(cb) { + return this.emitter.on('complete', cb); + } + + setPromise(promise) { + this.promise = promise; + } + + getPromise() { + return this.promise; + } + + setInProgress() { + // after exec has been called but before results a received + this.status = Operation.status.INPROGRESS; + } + + getExecutionTime() { + if (!this.startTime || !this.endTime) { + return NaN; + } else { + return this.endTime - this.startTime; + } + } + + complete(results, mutate = data => data) { + this.endTime = performance.now(); + this.results = results; + this.resolve(mutate(results)); + this.status = Operation.status.COMPLETE; + this.emitter.emit('complete', this); + this.emitter.dispose(); + } + + execute(execFn) { + this.startTime = performance.now(); + return execFn({...this.data, id: this.id}); + } +} diff --git a/lib/worker.js b/lib/worker.js new file mode 100644 index 0000000000..da5f35aadf --- /dev/null +++ b/lib/worker.js @@ -0,0 +1,91 @@ +const qs = require('querystring'); + +const {remote, ipcRenderer: ipc} = require('electron'); +const {GitProcess} = require('dugite'); + + +class AverageTracker { + constructor({limit} = {limit: 10}) { + // for now this serves a dual purpose - # of values tracked AND # discarded prior to starting avg calculation + this.limit = limit; + this.sum = 0; + this.values = []; + } + + addValue(value) { + if (this.values.length >= this.limit) { + const discardedValue = this.values.shift(); + this.sum -= discardedValue; + } + this.values.push(value); + this.sum += value; + } + + getAverage() { + if (this.enoughData()) { + return this.sum / this.limit; + } else { + return null; + } + } + + getLimit() { + return this.limit; + } + + enoughData() { + return this.values.length === this.limit; + } +} + +const query = qs.parse(window.location.search.substr(1)); +const sourceWebContentsId = remote.getCurrentWindow().webContents.id; +const operationCountLimit = parseInt(query.operationCountLimit, 10); +const averageTracker = new AverageTracker({limit: operationCountLimit}); + +const destroyRenderer = () => { remote.BrowserWindow.fromWebContents(remote.getCurrentWebContents()).destroy(); }; +const managerWebContentsId = parseInt(query.managerWebContentsId, 10); +const managerWebContents = remote.webContents.fromId(managerWebContentsId); +managerWebContents.on('crashed', () => { destroyRenderer(); }); +managerWebContents.on('destroyed', () => { destroyRenderer(); }); + +const channelName = query.channelName; +ipc.on(channelName, (event, {type, data}) => { + if (type === 'git-exec') { + const {args, workingDir, options, id} = data; + if (args) { + document.getElementById('command').textContent = `git ${args.join(' ')}`; + } + const spawnStart = performance.now(); + GitProcess.exec(args, workingDir, options) + .then(({stdout, stderr, exitCode}) => { + const timing = { + spawnTime: spawnEnd - spawnStart, + execTime: performance.now() - spawnEnd, + }; + event.sender.sendTo(managerWebContentsId, channelName, { + sourceWebContentsId, + type: 'git-data', + data: { + id, + average: averageTracker.getAverage(), + results: {stdout, stderr, exitCode, timing}, + }, + }); + }); + const spawnEnd = performance.now(); + averageTracker.addValue(spawnEnd - spawnStart); + + // TODO: consider using this to avoid duplicate write operations upon crashing. + // For now we won't do this to avoid clogging up ipc channel + // event.sender.sendTo(managerWebContentsId, channelName, {sourceWebContentsId, type: 'exec-started', data: {id}}); + + if (averageTracker.enoughData() && averageTracker.getAverage() > 20) { + event.sender.sendTo(managerWebContentsId, channelName, {type: 'slow-spawns'}); + } + } else { + throw new Error(`Could not identify type ${type}`); + } +}); + +ipc.sendTo(managerWebContentsId, channelName, {sourceWebContentsId, type: 'renderer-ready', data: {pid: process.pid}}); diff --git a/package.json b/package.json index 79c3680d35..4971d93e92 100644 --- a/package.json +++ b/package.json @@ -72,7 +72,7 @@ "yubikiri": "1.0.0" }, "devDependencies": { - "atom-mocha-test-runner": "^1.0.0", + "atom-mocha-test-runner": "^1.0.1", "babel-eslint": "^7.1.1", "chai": "^3.5.0", "chai-as-promised": "^5.3.0", diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index 3b2022e7c5..36dde4a2ea 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -3,10 +3,11 @@ import path from 'path'; import mkdirp from 'mkdirp'; import dedent from 'dedent-js'; +import {GitProcess} from 'dugite'; import CompositeGitStrategy from '../lib/composite-git-strategy'; import GitShellOutStrategy from '../lib/git-shell-out-strategy'; -import {GitProcess} from 'dugite'; +import WorkerManager from '../lib/worker-manager'; import {cloneRepository, initRepository, assertDeepPropertyVals, setUpLocalAndRemoteRepositories} from './helpers'; import {fsStat} from '../lib/helpers'; @@ -639,22 +640,21 @@ import {fsStat} from '../lib/helpers'; operations.forEach(op => { it(`temporarily overrides gpg.program when ${op.progressiveTense}`, async function() { - const execStub = sinon.stub(GitProcess, 'exec'); + const execStub = sinon.stub(git, 'executeGitCommand'); execStub.returns(Promise.resolve({stdout: '', stderr: '', exitCode: 0})); await op.action(); - const [args, workingDir, options] = execStub.getCall(0).args; + const [args, options] = execStub.getCall(0).args; assertGitConfigSetting(args, op.command, 'gpg.program', '.*gpg-no-tty\\.sh$'); assert.equal(options.env.ATOM_GITHUB_SOCK_PATH === undefined, !op.usesPromptServerAlready); - assert.equal(workingDir, git.workingDir); }); if (!op.usesPromptServerAlready) { it(`retries a ${op.command} with a GitPromptServer when GPG signing fails`, async function() { - const execStub = sinon.stub(GitProcess, 'exec'); + const execStub = sinon.stub(git, 'executeGitCommand'); execStub.onCall(0).returns(Promise.resolve({ stdout: '', stderr: 'stderr includes "gpg failed"', @@ -665,15 +665,13 @@ import {fsStat} from '../lib/helpers'; // Should not throw await op.action(); - const [args0, workingDir0, options0] = execStub.getCall(0).args; + const [args0, options0] = execStub.getCall(0).args; assertGitConfigSetting(args0, op.command, 'gpg.program', '.*gpg-no-tty\\.sh$'); assert.equal(options0.env.ATOM_GITHUB_SOCK_PATH === undefined, !op.usesPromptServerAlready); - assert.equal(workingDir0, git.workingDir); - const [args1, workingDir1, options1] = execStub.getCall(1).args; + const [args1, options1] = execStub.getCall(1).args; assertGitConfigSetting(args1, op.command, 'gpg.program', '.*gpg-no-tty\\.sh$'); assert.isDefined(options1.env.ATOM_GITHUB_SOCK_PATH); - assert.equal(workingDir1, git.workingDir); }); } }); @@ -720,7 +718,7 @@ import {fsStat} from '../lib/helpers'; operations.forEach(op => { it(`temporarily supplements credential.helper when ${op.progressiveTense}`, async function() { - const execStub = sinon.stub(GitProcess, 'exec'); + const execStub = sinon.stub(git, 'executeGitCommand'); execStub.returns(Promise.resolve({stdout: '', stderr: '', exitCode: 0})); if (op.configureStub) { op.configureStub(git); @@ -734,9 +732,7 @@ import {fsStat} from '../lib/helpers'; await op.action(); - const [args, workingDir, options] = execStub.getCall(0).args; - - assert.equal(workingDir, git.workingDir); + const [args, options] = execStub.getCall(0).args; // Used by https remotes assertGitConfigSetting(args, op.command, 'credential.helper', '.*git-credential-atom\\.sh'); @@ -919,6 +915,37 @@ import {fsStat} from '../lib/helpers'; }); }); }); + + describe('executeGitCommand', function() { + it('shells out in process until WorkerManager instance is ready', async function() { + const workingDirPath = await cloneRepository('three-files'); + const git = createTestStrategy(workingDirPath); + const workerManager = WorkerManager.getInstance(); + sinon.stub(workerManager, 'isReady'); + sinon.stub(GitProcess, 'exec'); + sinon.stub(workerManager, 'request'); + + workerManager.isReady.returns(false); + git.executeGitCommand(); + assert.equal(GitProcess.exec.callCount, 1); + assert.equal(workerManager.request.callCount, 0); + + workerManager.isReady.returns(true); + git.executeGitCommand(); + assert.equal(GitProcess.exec.callCount, 1); + assert.equal(workerManager.request.callCount, 1); + + workerManager.isReady.returns(false); + git.executeGitCommand(); + assert.equal(GitProcess.exec.callCount, 2); + assert.equal(workerManager.request.callCount, 1); + + workerManager.isReady.returns(true); + git.executeGitCommand(); + assert.equal(GitProcess.exec.callCount, 2); + assert.equal(workerManager.request.callCount, 2); + }); + }); }); }); diff --git a/test/helpers.js b/test/helpers.js index 8f5cf2dcfe..d0d3c14411 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -8,6 +8,7 @@ import sinon from 'sinon'; import Repository from '../lib/models/repository'; import GitShellOutStrategy from '../lib/git-shell-out-strategy'; +import WorkerManager from '../lib/worker-manager'; assert.autocrlfEqual = (actual, expected, ...args) => { const newActual = actual.replace(/\r\n/g, '\n'); @@ -89,6 +90,11 @@ export async function getHeadCommitOnRemote(remotePath) { export async function buildRepository(workingDirPath) { const repository = new Repository(workingDirPath); await repository.getLoadPromise(); + // eslint-disable-next-line jasmine/no-global-setup + afterEach(async () => { + const repo = await repository; + repo && repo.destroy(); + }); return repository; } @@ -158,6 +164,19 @@ export function createRenderer() { return renderer; } +export function isProcessAlive(pid) { + if (typeof pid !== 'number') { + throw new Error(`PID must be a number. Got ${pid}`); + } + let alive = true; + try { + return process.kill(pid, 0); + } catch (e) { + alive = false; + } + return alive; +} + // eslint-disable-next-line jasmine/no-global-setup beforeEach(function() { global.sinon = sinon.sandbox.create(); @@ -169,3 +188,10 @@ afterEach(function() { activeRenderers = []; global.sinon.restore(); }); + +// eslint-disable-next-line jasmine/no-global-setup +after(() => { + if (!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW) { + WorkerManager.reset(true); + } +}); diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js new file mode 100644 index 0000000000..9491114c46 --- /dev/null +++ b/test/worker-manager.test.js @@ -0,0 +1,179 @@ +import {remote} from 'electron'; +const {BrowserWindow} = remote; + +import WorkerManager, {Operation, Worker} from '../lib/worker-manager'; +import {isProcessAlive} from './helpers'; + +describe('WorkerManager', function() { + let workerManager; + beforeEach(() => { + workerManager = new WorkerManager(); + }); + + afterEach(() => { + workerManager.destroy(true); + }); + + describe('isReady()', function() { + it('returns true if its worker is ready', async function() { + assert.isFalse(workerManager.isReady()); + await workerManager.getReadyPromise(); + assert.isTrue(workerManager.isReady()); + + workerManager.onSick(workerManager.getActiveWorker()); + assert.isFalse(workerManager.isReady()); + await workerManager.getReadyPromise(); + assert.isTrue(workerManager.isReady()); + }); + }); + + describe('when a worker process crashes', function() { + it('creates a new worker process (with the same operation count limit) and executes remaining operations', async function() { + workerManager.createNewWorker({operationCountLimit: 40}); + sinon.stub(Operation.prototype, 'execute'); + + const worker1 = workerManager.getActiveWorker(); + await worker1.getReadyPromise(); + workerManager.request(); + workerManager.request(); + workerManager.request(); + const worker1OperationsInFlight = worker1.getRemainingOperations(); + assert.lengthOf(worker1OperationsInFlight, 3); + + const worker1Pid = worker1.getPid(); + process.kill(worker1Pid, 'SIGKILL'); + + await assert.async.notEqual(worker1, workerManager.getActiveWorker()); + const worker2 = workerManager.getActiveWorker(); + await worker2.getReadyPromise(); + assert.notEqual(worker2.getPid(), worker1Pid); + assert.equal(worker2.getOperationCountLimit(), worker1.getOperationCountLimit()); + assert.deepEqual(worker2.getRemainingOperations(), worker1OperationsInFlight); + }); + }); + + describe('when a worker process is sick', function() { + it('creates a new worker with a new operation count limit that is based on the limit and completed operation count of the last worker', function() { + + function createSickWorker(operationCountLimit, completedOperationCount) { + const sickWorker = workerManager.getActiveWorker(); + sinon.stub(sickWorker, 'getOperationCountLimit').returns(operationCountLimit); + sinon.stub(sickWorker, 'getCompletedOperationCount').returns(completedOperationCount); + return sickWorker; + } + + // when the last worker operation count limit was greater than or equal to the completed operation count + // this means that the average spawn time for the first operationCountLimit operations was already higher than the threshold + // the system is likely just slow, so we should increase the operationCountLimit so next time we do more operations before creating a new process + const sickWorker1 = createSickWorker(10, 9); + workerManager.onSick(sickWorker1); + assert.notEqual(sickWorker1, workerManager.getActiveWorker()); + assert.equal(workerManager.getActiveWorker().getOperationCountLimit(), 20); + + const sickWorker2 = createSickWorker(50, 50); + workerManager.onSick(sickWorker2); + assert.notEqual(sickWorker2, workerManager.getActiveWorker()); + assert.equal(workerManager.getActiveWorker().getOperationCountLimit(), 100); + + const sickWorker3 = createSickWorker(100, 100); + workerManager.onSick(sickWorker3); + assert.notEqual(sickWorker3, workerManager.getActiveWorker()); + assert.equal(workerManager.getActiveWorker().getOperationCountLimit(), 100); + + // when the last worker operation count limit was less than the completed operation count + // this means that the system is performing better and we can drop the operationCountLimit back down to the base limit + const sickWorker4 = createSickWorker(100, 150); + workerManager.onSick(sickWorker4); + assert.notEqual(sickWorker4, workerManager.getActiveWorker()); + assert.equal(workerManager.getActiveWorker().getOperationCountLimit(), 10); + }); + + describe('when the sick process crashes', function() { + it('completes remaining operations in existing active process', function() { + const sickWorker = workerManager.getActiveWorker(); + + sinon.stub(Operation.prototype, 'execute'); + workerManager.request(); + workerManager.request(); + workerManager.request(); + + const operationsInFlight = sickWorker.getRemainingOperations(); + assert.equal(operationsInFlight.length, 3); + + workerManager.onSick(sickWorker); + assert.notEqual(sickWorker, workerManager.getActiveWorker()); + const newWorker = workerManager.getActiveWorker(); + assert.equal(newWorker.getRemainingOperations(), 0); + + workerManager.onCrashed(sickWorker); + assert.equal(workerManager.getActiveWorker(), newWorker); + assert.equal(newWorker.getRemainingOperations().length, 3); + }); + }); + }); + + describe('destroy', function() { + it('destroys the renderer processes created after they have completed their operations', async function() { + const worker1 = workerManager.getActiveWorker(); + await worker1.getReadyPromise(); + + sinon.stub(Operation.prototype, 'execute'); + workerManager.request(); + workerManager.request(); + workerManager.request(); + const worker1Operations = worker1.getRemainingOperations(); + assert.equal(worker1Operations.length, 3); + + workerManager.onSick(worker1); + const worker2 = workerManager.getActiveWorker(); + await worker2.getReadyPromise(); + workerManager.request(); + workerManager.request(); + const worker2Operations = worker2.getRemainingOperations(); + assert.equal(worker2Operations.length, 2); + + workerManager.destroy(); + assert.isTrue(isProcessAlive(worker1.getPid())); + assert.isTrue(isProcessAlive(worker2.getPid())); + + [...worker1Operations, ...worker2Operations].forEach(operation => operation.complete()); + await assert.async.isFalse(isProcessAlive(worker1.getPid())); + await assert.async.isFalse(isProcessAlive(worker2.getPid())); + }); + }); + + describe('when the manager process is destroyed', function() { + it('destroys all the renderer processes that were created', async function() { + const browserWindow = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); + browserWindow.loadURL('about:blank'); + sinon.stub(Worker.prototype, 'getWebContentsId').returns(browserWindow.webContents.id); + + const script = ` + const ipc = require('electron').ipcRenderer; + ipc.on('${Worker.channelName}', function() { + const args = Array.prototype.slice.apply(arguments) + args.shift(); + + args.unshift('${Worker.channelName}'); + args.unshift(${remote.getCurrentWebContents().id}) + ipc.sendTo.apply(ipc, args); + }); + `; + + await new Promise(resolve => browserWindow.webContents.executeJavaScript(script, resolve)); + + workerManager.destroy(true); + workerManager = new WorkerManager(); + + const worker1 = workerManager.getActiveWorker(); + await worker1.getReadyPromise(); + workerManager.onSick(worker1); + const worker2 = workerManager.getActiveWorker(); + await worker2.getReadyPromise(); + + browserWindow.destroy(); + await assert.async.isFalse(isProcessAlive(worker1.getPid())); + await assert.async.isFalse(isProcessAlive(worker2.getPid())); + }); + }); +});