From c07d65b048d2a58eece749c507125e91a57ba0b9 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 12 Apr 2017 18:39:34 -0700 Subject: [PATCH 01/78] WIP compare renderer and child process IPC times --- lib/git-child-process.js | 40 ++++++++++++++++++++++++++++++++++++++++ lib/renderer.html | 11 +++++++++++ lib/renderer.js | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+) create mode 100644 lib/git-child-process.js create mode 100644 lib/renderer.html create mode 100644 lib/renderer.js diff --git a/lib/git-child-process.js b/lib/git-child-process.js new file mode 100644 index 0000000000..ab1b099dfb --- /dev/null +++ b/lib/git-child-process.js @@ -0,0 +1,40 @@ +function log(...args) { + process.send({type: 'log', data: args}); +} + +const {GitProcess} = require('dugite'); + +log('forking'); +let tock = 0; +const tick = function() { + log('tick ', tock++); + setTimeout(tick, 250); +}; +// tick(); + +process.on('message', amount => { + const startTime = process.uptime(); + const str = 'a'.repeat(amount); + const execTime = (process.uptime() - startTime) * 1000; + process.send({ + type: 'git', + data: {exitCode: 0, stdout: str, stderr: '', time: execTime}, + }); + // GitProcess.exec(args, '/Users/kuychaco/src/test-repo') + // .then(({stdout, stderr, exitCode}) => { + // const execTime = (process.uptime() - startTime) * 1000; + // process.send({ + // type: 'git', + // data: {exitCode, stdout, stderr, time: execTime}, + // }); + // }); +}); + + +// console.log('forkin!'); +// +// process.on('message', msg => { +// console.log('child got message:', msg); +// process.send(msg); +// // process.send('Do I have electron? ' + !!require('fs')); +// }); diff --git a/lib/renderer.html b/lib/renderer.html new file mode 100644 index 0000000000..a78ec44049 --- /dev/null +++ b/lib/renderer.html @@ -0,0 +1,11 @@ + + + + + + + + +

HIIII

+ + diff --git a/lib/renderer.js b/lib/renderer.js new file mode 100644 index 0000000000..b5be823536 --- /dev/null +++ b/lib/renderer.js @@ -0,0 +1,32 @@ +// console.log('wooooo'); +// +// const renderer = require('electron').ipcRenderer; + +// renderer.on('data', event => { +// console.log('renderer got message:', event); +// const startTime = process.uptime(); +// const str = '1234567890'.repeat(400000); +// const execTime = (process.uptime() - startTime) * 1000; +// const {BrowserWindow} = require('electron').remote; +// debugger; +// const myWindow = BrowserWindow.fromWebContents(event.sender); +// // console.log('browser window', myWindow); +// // myWindow.webContents.send('data'); + // myWindow.webContents.send('data', { + // type: 'git', + // data: {exitCode: 0, stdout: str, stderr: '', time: execTime}, + // }); +// }); + + +const ipc = require('electron').ipcRenderer; + +ipc.on('ping', (event, webContentsId, amount) => { + const startTime = process.uptime(); + const str = 'a'.repeat(amount); + const execTime = (process.uptime() - startTime) * 1000; + event.sender.sendTo(webContentsId, 'pong', { + type: 'git', + data: {exitCode: 0, stdout: str, stderr: '', time: execTime}, + }); +}); From 53384de28acdc827aef5c75c0411258c85eb123a Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 13 Apr 2017 00:43:50 -0700 Subject: [PATCH 02/78] WIP shell out to git in renderer process --- lib/git-shell-out-strategy.js | 130 +++++++++++++++++++++++----------- lib/renderer.js | 77 +++++++++++++------- lib/views/git-timings-view.js | 7 +- 3 files changed, 148 insertions(+), 66 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 213ba09e71..e067d8de18 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -23,6 +23,39 @@ export class GitError extends Error { } } +const remote = require('electron').remote; +const {BrowserWindow} = remote; +const ipc = require('electron').ipcRenderer; + +const webContentsId = remote.getCurrentWebContents().id; +const win = new BrowserWindow(); +win.webContents.openDevTools(); +win.loadURL('file:///Users/kuychaco/github/github/lib/renderer.html'); + +const resolveByOperationId = new Map(); +let operationId = 0; +global.requestCounter = 0; + +ipc.on('pong', (event, {type, data}) => { + console.log('operationId', data.operationId); + if (type === 'git') { + const resolve = resolveByOperationId.get(data.operationId); + global.requestCounter--; + if (data.err) { + const error = new GitError(data.err.message); + Object.assign(error, data.err); + return resolve(Promise.reject(error)); // TODO: check this + } + resolve(data.result); + const execTime = data.execTime; + const totalTime = performance.now() - data.timeSent; + console.debug(`git data for ${data.formattedArgs}`, data); + console.debug('times (total, exec, ipc)', Math.round(totalTime), Math.round(execTime), Math.round(totalTime - execTime)); + } else { + console.log('unrecognized type'); + } +}); + /* * Convert a Windows-style "C:\foo\bar\baz" path to a "/c/foo/bar/baz" UNIX-y * path that the sh.exe used to execute git's credential helpers will @@ -78,7 +111,6 @@ export default class GitShellOutStrategy { const formattedArgs = `git ${args.join(' ')} in ${this.workingDir}`; const timingMarker = GitTimingsView.generateMarker(`git ${args.join(' ')}`); timingMarker.mark('queued'); - return this.commandQueue.push(async () => { timingMarker.mark('prepare'); let gitPromptServer; @@ -131,6 +163,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.error('Error executing: ' + formattedArgs); @@ -154,49 +187,61 @@ export default class GitShellOutStrategy { options.stdinEncoding = 'utf8'; } - if (process.env.PRINT_GIT_TIMES) { - console.time(`git:${formattedArgs}`); - } + // if (process.env.PRINT_GIT_TIMES) { + // console.time(`git:${formattedArgs}`); + // } return new Promise(resolve => { + global.requestCounter++; 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; - })); + timingMarker.mark('execute'); + + const timeSent = performance.now(); + resolveByOperationId.set(++operationId, resolve); + win.webContents.send('ping', webContentsId, { + args, + workingDir: this.workingDir, + options, + operationId, + timeSent, }); + + // setImmediate(() => { + // 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; + // })); + // }); }); }, {parallel: !writeOperation}); /* eslint-enable no-console */ @@ -554,8 +599,11 @@ export default class GitShellOutStrategy { let args = ['config']; if (local) { args.push('--local'); } args = args.concat(option); + console.warn(args); output = await this.exec(args); + console.warn(output); } catch (err) { + console.warn(err); if (err.code === 1) { // No matching config found return null; diff --git a/lib/renderer.js b/lib/renderer.js index b5be823536..6571908b22 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -1,32 +1,61 @@ -// console.log('wooooo'); -// -// const renderer = require('electron').ipcRenderer; +const {GitProcess} = require('dugite'); -// renderer.on('data', event => { -// console.log('renderer got message:', event); -// const startTime = process.uptime(); -// const str = '1234567890'.repeat(400000); -// const execTime = (process.uptime() - startTime) * 1000; -// const {BrowserWindow} = require('electron').remote; -// debugger; -// const myWindow = BrowserWindow.fromWebContents(event.sender); -// // console.log('browser window', myWindow); -// // myWindow.webContents.send('data'); - // myWindow.webContents.send('data', { +const ipc = require('electron').ipcRenderer; + +ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, timeSent}) => { + const formattedArgs = `git ${args.join(' ')} in ${workingDir}`; + console.log('operationId', operationId, formattedArgs); + const startTime = process.uptime(); + // For profiling purposes + // const str = 'a'.repeat(amount); + // const execTime = (process.uptime() - startTime) * 1000; + // event.sender.sendTo(webContentsId, 'pong', { // type: 'git', // data: {exitCode: 0, stdout: str, stderr: '', time: execTime}, // }); -// }); + if (process.env.PRINT_GIT_TIMES) { + console.time(`git:${formattedArgs}`); + } + GitProcess.exec(args, workingDir, options) + .then(({stdout, stderr, exitCode}) => { + // timingMarker.finalize(); + // if (process.env.PRINT_GIT_TIMES) { + // console.timeEnd(`git:${formattedArgs}`); + // } + // if (gitPromptServer) { + // gitPromptServer.terminate(); + // } + // subscriptions.dispose(); -const ipc = require('electron').ipcRenderer; + // 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(); + // } -ipc.on('ping', (event, webContentsId, amount) => { - const startTime = process.uptime(); - const str = 'a'.repeat(amount); - const execTime = (process.uptime() - startTime) * 1000; - event.sender.sendTo(webContentsId, 'pong', { - type: 'git', - data: {exitCode: 0, stdout: str, stderr: '', time: execTime}, - }); + let err = null; + if (exitCode) { + err = { + message: `${formattedArgs} exited with code ${exitCode}\nstdout: ${stdout}\nstderr: ${stderr}`, + code: exitCode, + stdErr: stderr, + stdOut: stdout, + command: formattedArgs, + }; + } + const execTime = (process.uptime() - startTime) * 1000; + console.log(operationId); + console.log(stdout); + event.sender.sendTo(webContentsId, 'pong', { + type: 'git', + data: {result: stdout, err, execTime, operationId, timeSent, formattedArgs}, + }); + }); }); diff --git a/lib/views/git-timings-view.js b/lib/views/git-timings-view.js index 2aa7cf9c5e..10aaffb938 100644 --- a/lib/views/git-timings-view.js +++ b/lib/views/git-timings-view.js @@ -348,6 +348,8 @@ export default class GitTimingsView extends React.Component { static emitter = new Emitter(); + static markerByOperationId = new Map(); + static createPaneItem() { let element; return { @@ -368,10 +370,13 @@ export default class GitTimingsView extends React.Component { return this.createPaneItem(); } - static generateMarker(label) { + static generateMarker(label, operationId) { + const existingMarker = GitTimingsView.markerByOperationId.get(operationId); + if (existingMarker) { return existingMarker; } const marker = new Marker(label, () => { GitTimingsView.scheduleUpdate(); }); + GitTimingsView.markerByOperationId.set(operationId, marker); const now = performance.now(); if (!markers || (lastMarkerTime && Math.abs(now - lastMarkerTime) >= 5000)) { groupId++; From 519ebfc37f8f8684c88aa291473d39e174f721e3 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 13 Apr 2017 20:02:39 -0700 Subject: [PATCH 03/78] WIP Create RendererProcessManager --- lib/git-shell-out-strategy.js | 80 ++++++++++++---------- lib/renderer-process-manager.js | 116 ++++++++++++++++++++++++++++++++ lib/renderer.js | 46 ++++++++++++- 3 files changed, 204 insertions(+), 38 deletions(-) create mode 100644 lib/renderer-process-manager.js diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index e067d8de18..c67f14c06a 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -3,13 +3,13 @@ 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 GitTimingsView from './views/git-timings-view'; +import RendererProcessManager from './renderer-process-manager'; const LINE_ENDING_REGEX = /\r?\n/; @@ -23,38 +23,39 @@ export class GitError extends Error { } } -const remote = require('electron').remote; -const {BrowserWindow} = remote; -const ipc = require('electron').ipcRenderer; - -const webContentsId = remote.getCurrentWebContents().id; -const win = new BrowserWindow(); -win.webContents.openDevTools(); -win.loadURL('file:///Users/kuychaco/github/github/lib/renderer.html'); - -const resolveByOperationId = new Map(); -let operationId = 0; -global.requestCounter = 0; - -ipc.on('pong', (event, {type, data}) => { - console.log('operationId', data.operationId); - if (type === 'git') { - const resolve = resolveByOperationId.get(data.operationId); - global.requestCounter--; - if (data.err) { - const error = new GitError(data.err.message); - Object.assign(error, data.err); - return resolve(Promise.reject(error)); // TODO: check this - } - resolve(data.result); - const execTime = data.execTime; - const totalTime = performance.now() - data.timeSent; - console.debug(`git data for ${data.formattedArgs}`, data); - console.debug('times (total, exec, ipc)', Math.round(totalTime), Math.round(execTime), Math.round(totalTime - execTime)); - } else { - console.log('unrecognized type'); - } -}); + +// const remote = require('electron').remote; +// const {BrowserWindow} = remote; +// const ipc = require('electron').ipcRenderer; +// +// const webContentsId = remote.getCurrentWebContents().id; +// const win = new BrowserWindow(); +// win.webContents.openDevTools(); +// win.loadURL('file:///Users/kuychaco/github/github/lib/renderer.html'); +// +// const resolveByOperationId = new Map(); +// const operationId = 0; +// global.requestCounter = 0; +// +// ipc.on('pong', (event, {type, data}) => { +// console.log('operationId', data.operationId); +// if (type === 'git') { +// const resolve = resolveByOperationId.get(data.operationId); +// global.requestCounter--; +// if (data.err) { +// const error = new GitError(data.err.message); +// Object.assign(error, data.err); +// return resolve(Promise.reject(error)); // TODO: check this +// } +// resolve(data.result); +// const execTime = data.execTime; +// const totalTime = performance.now() - data.timeSent; +// console.debug(`git data for ${data.formattedArgs}`, data); +// console.debug('times (total, exec, ipc)', Math.round(totalTime), Math.round(execTime), Math.round(totalTime - execTime)); +// } else { +// console.log('unrecognized type'); +// } +// }); /* * Convert a Windows-style "C:\foo\bar\baz" path to a "/c/foo/bar/baz" UNIX-y @@ -90,6 +91,7 @@ export default class GitShellOutStrategy { } this.prompt = options.prompt || (query => Promise.reject()); + this.rendererProcessManager = new RendererProcessManager(); } /* @@ -196,14 +198,20 @@ export default class GitShellOutStrategy { timingMarker.mark('execute'); const timeSent = performance.now(); - resolveByOperationId.set(++operationId, resolve); - win.webContents.send('ping', webContentsId, { + this.rendererProcessManager.send(resolve, { args, workingDir: this.workingDir, options, - operationId, timeSent, }); + // resolveByOperationId.set(++operationId, resolve); + // win.webContents.send('ping', webContentsId, { + // args, + // workingDir: this.workingDir, + // options, + // operationId, + // timeSent, + // }); // setImmediate(() => { // resolve(GitProcess.exec(args, this.workingDir, options) diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js new file mode 100644 index 0000000000..90f7f5afb3 --- /dev/null +++ b/lib/renderer-process-manager.js @@ -0,0 +1,116 @@ +const remote = require('electron').remote; +const {BrowserWindow} = remote; +const ipc = require('electron').ipcRenderer; + +const {GitProcess} = require('dugite'); + +import GitError from './git-shell-out-strategy'; + + +export default class RendererProcessManager { + constructor() { + this.resolveByOperationId = new Map(); + this.operationId = 0; + this.webContentsId = remote.getCurrentWebContents().id; + global.requestCounter = 0; + this.renderer = null; + this.createNewRenderer(); + + ipc.on('pong', (event, {type, data}) => { + console.log('operationId', data.operationId); + if (type === 'git') { + const resolve = this.resolveByOperationId.get(data.operationId); + global.requestCounter--; + if (data.err) { + const error = new GitError(data.err.message); + Object.assign(error, data.err); + resolve(Promise.reject(error)); // TODO: check this + } else { + resolve(data.result); + } + const execTime = data.execTime; + const totalTime = performance.now() - data.timeSent; + console.debug(`git data for ${data.formattedArgs}`, data); + console.debug('times (total, exec, ipc)', Math.round(totalTime), Math.round(execTime), Math.round(totalTime - execTime)); + } else { + console.log('unrecognized type'); + } + }); + } + + send(resolve, data) { + if (this.renderer && this.renderer.isReady()) { + console.log('renderer ready!'); + this.resolveByOperationId.set(++this.operationId, resolve); + this.renderer.send({...data, operationId: this.operationId}); + } else { + console.log('shell out until renderer is ready'); + resolve(GitProcess.exec(data.args, data.workingDir, data.options) + .then(({stdout, stderr, exitCode}) => { + const formattedArgs = `git ${data.args.join(' ')} in ${data.workingDir}`; + // 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; + })); + } + } + + createNewRenderer() { + if (this.renderer) { this.renderer.destroy(); } + this.renderer = new RendererProcess(this.webContentsId); + } +} + +class RendererProcess { + constructor(webContentsId) { + console.log('create renderer process'); + this.webContentsId = webContentsId; + this.win = new BrowserWindow(); + this.win.webContents.openDevTools(); + this.win.loadURL('file:///Users/kuychaco/github/github/lib/renderer.html'); + this.ready = false; + this.win.webContents.once('dom-ready', () => this.ready = true); + } + + send(data) { + console.log('send data'); + this.win.webContents.send('ping', this.webContentsId, data); + } + + isReady() { + return this.ready; + } + + destroy() { + // after last response is received, shut down + } +} diff --git a/lib/renderer.js b/lib/renderer.js index 6571908b22..16f8c4c5ba 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -2,10 +2,47 @@ const {GitProcess} = require('dugite'); const ipc = require('electron').ipcRenderer; +/* +Calculate running averages + +Store time that exec was called. Or the difference between that and the last time it was called. +Ignore the time for write operations that are non-parallelizable +Take an average of the last 3 times +And if it is above XX ms create a new renderer to send future messages to. +Once the current renderer finishes up its work kill it +Timing concerns? the async queue will take care of everything for us +*/ + + +class AverageTracker { + constructor({limit} = {limit: 10}) { + 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.values.length < this.limit) { return null; } + return this.sum / this.limit; + } +} + +const averageTracker = new AverageTracker({limit: 10}); + + ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, timeSent}) => { const formattedArgs = `git ${args.join(' ')} in ${workingDir}`; console.log('operationId', operationId, formattedArgs); - const startTime = process.uptime(); + const startTime = performance.now(); // For profiling purposes // const str = 'a'.repeat(amount); // const execTime = (process.uptime() - startTime) * 1000; @@ -17,6 +54,7 @@ ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, t if (process.env.PRINT_GIT_TIMES) { console.time(`git:${formattedArgs}`); } + const execStart = performance.now(); GitProcess.exec(args, workingDir, options) .then(({stdout, stderr, exitCode}) => { // timingMarker.finalize(); @@ -50,7 +88,7 @@ ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, t command: formattedArgs, }; } - const execTime = (process.uptime() - startTime) * 1000; + const execTime = performance.now() - startTime; console.log(operationId); console.log(stdout); event.sender.sendTo(webContentsId, 'pong', { @@ -58,4 +96,8 @@ ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, t data: {result: stdout, err, execTime, operationId, timeSent, formattedArgs}, }); }); + const execEnd = performance.now(); + averageTracker.addValue(execEnd - execStart); + console.error(averageTracker.values); + console.error(averageTracker.getAverage()); }); From c9b2978578e9230c300d74c1b757773d148afd2a Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 17 Apr 2017 16:47:00 -0700 Subject: [PATCH 04/78] :arrow_up: atom-mocha-test-runner@1.0.1 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index f47b168d56..612d5946f9 100644 --- a/package.json +++ b/package.json @@ -68,7 +68,7 @@ "yubikiri": "0.0.2" }, "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", From f91e8cdf6d4d2e43277411aeb6d3bbeaf290850a Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 17 Apr 2017 16:53:14 -0700 Subject: [PATCH 05/78] Make RendererProcessManager a singleton Only need one RendererProcessManager for each Atom window. Also reset RendererProcessManager in afterEach. --- lib/git-shell-out-strategy.js | 2 +- lib/renderer-process-manager.js | 61 +++++++++++++++++++++++---------- lib/renderer.js | 4 +-- test/helpers.js | 2 ++ 4 files changed, 48 insertions(+), 21 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index c67f14c06a..8c602b585e 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -91,7 +91,7 @@ export default class GitShellOutStrategy { } this.prompt = options.prompt || (query => Promise.reject()); - this.rendererProcessManager = new RendererProcessManager(); + this.rendererProcessManager = options.rendererProcessManager || RendererProcessManager.getInstance(); } /* diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index 90f7f5afb3..2af5906ce6 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -8,6 +8,20 @@ import GitError from './git-shell-out-strategy'; export default class RendererProcessManager { + static instance = null; + + static getInstance() { + if (!this.instance) { + this.instance = new RendererProcessManager(); + } + return this.instance; + } + + static reset() { + if (this.instance) { this.instance.destroy(); } + this.instance = null; + } + constructor() { this.resolveByOperationId = new Map(); this.operationId = 0; @@ -16,26 +30,30 @@ export default class RendererProcessManager { this.renderer = null; this.createNewRenderer(); - ipc.on('pong', (event, {type, data}) => { - console.log('operationId', data.operationId); - if (type === 'git') { - const resolve = this.resolveByOperationId.get(data.operationId); - global.requestCounter--; - if (data.err) { - const error = new GitError(data.err.message); - Object.assign(error, data.err); - resolve(Promise.reject(error)); // TODO: check this - } else { - resolve(data.result); - } - const execTime = data.execTime; - const totalTime = performance.now() - data.timeSent; - console.debug(`git data for ${data.formattedArgs}`, data); - console.debug('times (total, exec, ipc)', Math.round(totalTime), Math.round(execTime), Math.round(totalTime - execTime)); + ipc.on('git-data', this.onGitDataReceived); + } + + @autobind + onGitDataReceived(event, {type, data}) { + console.log('operationId', data.operationId); + if (type === 'git') { + console.warn('<', data.operationId, Object.keys(this.resolveByOperationId)); + const resolve = this.resolveByOperationId[data.operationId]; + global.requestCounter--; + if (data.err) { + const error = new GitError(data.err.message); + Object.assign(error, data.err); + resolve(Promise.reject(error)); // TODO: check this } else { - console.log('unrecognized type'); + resolve(data.result); } - }); + const execTime = data.execTime; + const totalTime = performance.now() - data.timeSent; + console.debug(`git data for ${data.formattedArgs}`, data); + console.debug('times (total, exec, ipc)', Math.round(totalTime), Math.round(execTime), Math.round(totalTime - execTime)); + } else { + console.log('unrecognized type'); + } } send(resolve, data) { @@ -88,6 +106,12 @@ export default class RendererProcessManager { if (this.renderer) { this.renderer.destroy(); } this.renderer = new RendererProcess(this.webContentsId); } + + destroy() { + if (this.renderer) { this.renderer.destroy(); } + this.renderer = null; + ipc.removeListener('git-data', this.onGitDataReceived); + } } class RendererProcess { @@ -112,5 +136,6 @@ class RendererProcess { destroy() { // after last response is received, shut down + this.win.destroy(); } } diff --git a/lib/renderer.js b/lib/renderer.js index 16f8c4c5ba..3096b91620 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -46,7 +46,7 @@ ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, t // For profiling purposes // const str = 'a'.repeat(amount); // const execTime = (process.uptime() - startTime) * 1000; - // event.sender.sendTo(webContentsId, 'pong', { + // event.sender.sendTo(webContentsId, 'git-data', { // type: 'git', // data: {exitCode: 0, stdout: str, stderr: '', time: execTime}, // }); @@ -91,7 +91,7 @@ ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, t const execTime = performance.now() - startTime; console.log(operationId); console.log(stdout); - event.sender.sendTo(webContentsId, 'pong', { + event.sender.sendTo(webContentsId, 'git-data', { type: 'git', data: {result: stdout, err, execTime, operationId, timeSent, formattedArgs}, }); diff --git a/test/helpers.js b/test/helpers.js index eb843d86dc..0c64b9c9d8 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 RendererProcessManager from '../lib/renderer-process-manager'; assert.autocrlfEqual = (actual, expected, ...args) => { const newActual = actual.replace(/\r\n/g, '\n'); @@ -159,5 +160,6 @@ beforeEach(function() { afterEach(function() { activeRenderers.forEach(r => r.unmount()); activeRenderers = []; + RendererProcessManager.reset(); global.sinon.restore(); }); From 8fe00d5c46e6f1057158b0de6ed2de755c04b8dd Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 17 Apr 2017 16:53:34 -0700 Subject: [PATCH 06/78] Actually import GitError --- lib/renderer-process-manager.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index 2af5906ce6..1364079d58 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -6,6 +6,7 @@ const {GitProcess} = require('dugite'); import GitError from './git-shell-out-strategy'; +import {GitError} from './git-shell-out-strategy'; export default class RendererProcessManager { static instance = null; From e9aa22d270969a31f3627260cc6b83dc8ec5d276 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 17 Apr 2017 16:54:13 -0700 Subject: [PATCH 07/78] Use import instead of require --- lib/renderer-process-manager.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index 1364079d58..7ff5bb0bc8 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -1,10 +1,8 @@ -const remote = require('electron').remote; +import {remote, ipcRenderer as ipc} from 'electron'; const {BrowserWindow} = remote; -const ipc = require('electron').ipcRenderer; -const {GitProcess} = require('dugite'); - -import GitError from './git-shell-out-strategy'; +import {GitProcess} from 'dugite'; +import {autobind} from 'core-decorators'; import {GitError} from './git-shell-out-strategy'; From 169ab2013e3b22c7c96a6b8f46b1e76f47fec3a7 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 17 Apr 2017 17:01:55 -0700 Subject: [PATCH 08/78] Don't show browser window for git renderer process --- lib/renderer-process-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index 7ff5bb0bc8..f2a6e3deb8 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -117,7 +117,7 @@ class RendererProcess { constructor(webContentsId) { console.log('create renderer process'); this.webContentsId = webContentsId; - this.win = new BrowserWindow(); + this.win = new BrowserWindow({show: false}); this.win.webContents.openDevTools(); this.win.loadURL('file:///Users/kuychaco/github/github/lib/renderer.html'); this.ready = false; From 72b5ccce9d9802f3fb69b0ed87fdf0aaceda485e Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 18 Apr 2017 18:15:30 -0700 Subject: [PATCH 09/78] WIP wtf is up w/ the tests --- lib/git-shell-out-strategy.js | 225 ++++++++++++++--------- lib/renderer-process-manager.js | 100 +++++----- lib/renderer.js | 67 +++---- test/controllers/root-controller.test.js | 16 +- test/github-package.test.js | 4 +- test/models/repository.test.js | 12 +- test/runner.js | 4 +- 7 files changed, 229 insertions(+), 199 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 8c602b585e..b0d7ed5b45 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -4,6 +4,7 @@ import os from 'os'; import {CompositeDisposable} from 'event-kit'; import {parse as parseDiff} from 'what-the-diff'; +import {GitProcess} from 'dugite'; import GitPromptServer from './git-prompt-server'; import AsyncQueue from './async-queue'; @@ -24,39 +25,6 @@ export class GitError extends Error { } -// const remote = require('electron').remote; -// const {BrowserWindow} = remote; -// const ipc = require('electron').ipcRenderer; -// -// const webContentsId = remote.getCurrentWebContents().id; -// const win = new BrowserWindow(); -// win.webContents.openDevTools(); -// win.loadURL('file:///Users/kuychaco/github/github/lib/renderer.html'); -// -// const resolveByOperationId = new Map(); -// const operationId = 0; -// global.requestCounter = 0; -// -// ipc.on('pong', (event, {type, data}) => { -// console.log('operationId', data.operationId); -// if (type === 'git') { -// const resolve = resolveByOperationId.get(data.operationId); -// global.requestCounter--; -// if (data.err) { -// const error = new GitError(data.err.message); -// Object.assign(error, data.err); -// return resolve(Promise.reject(error)); // TODO: check this -// } -// resolve(data.result); -// const execTime = data.execTime; -// const totalTime = performance.now() - data.timeSent; -// console.debug(`git data for ${data.formattedArgs}`, data); -// console.debug('times (total, exec, ipc)', Math.round(totalTime), Math.round(execTime), Math.round(totalTime - execTime)); -// } else { -// console.log('unrecognized type'); -// } -// }); - /* * Convert a Windows-style "C:\foo\bar\baz" path to a "/c/foo/bar/baz" UNIX-y * path that the sh.exe used to execute git's credential helpers will @@ -192,64 +160,157 @@ export default class GitShellOutStrategy { // if (process.env.PRINT_GIT_TIMES) { // console.time(`git:${formattedArgs}`); // } - return new Promise(resolve => { - global.requestCounter++; + return new Promise(async (resolve, reject) => { timingMarker.mark('nexttick'); timingMarker.mark('execute'); const timeSent = performance.now(); - this.rendererProcessManager.send(resolve, { + + // if (this.rendererProcessManager.isReady()) { + // + // } else { + // + // } + + + console.debug('STARTING to wait...'); + console.debug('GOT IT'); + global.results += args.join(' ') + ': '; + console.warn(args.join(' ')); + await this.rendererProcessManager.getReadyPromise(); + const rendererResults = await this.rendererProcessManager.request({ args, workingDir: this.workingDir, options, timeSent, }); - // resolveByOperationId.set(++operationId, resolve); - // win.webContents.send('ping', webContentsId, { - // args, - // workingDir: this.workingDir, - // options, - // operationId, - // timeSent, - // }); - - // setImmediate(() => { - // 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; - // })); - // }); + // const gitResults = await GitProcess.exec(args, this.workingDir, options); + // console.warn('RESULTS', gitResults); + const {stdout, stderr, exitCode} = rendererResults; + // const {stdout, stderr, exitCode} = gitResults; + global.results += JSON.stringify({stdout, stderr, exitCode}) + '\n'; + 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); + + // await this.rendererProcessManager.getReadyPromise(); + // if (this.rendererProcessManager.isReady()) { + // options.env = {...process.env, ...options.env}; + // resolve( + // this.rendererProcessManager.request({ + // args, + // workingDir: this.workingDir, + // options, + // timeSent, + // }) + // .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); + // } + // // resolve(response); + // return stdout; + // })); + // + // // if (args.includes('atomGithub.historySha')) { debugger; } + // // resolve(response); + // } else { + // // throw new Error('SHELLING OUT'); + // // if (args.includes('atomGithub.historySha')) { debugger; } + // console.log(this.workingDir); + // 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; + // })); + // } }); }, {parallel: !writeOperation}); /* eslint-enable no-console */ diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index f2a6e3deb8..c83dd5e6ac 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -1,7 +1,6 @@ import {remote, ipcRenderer as ipc} from 'electron'; const {BrowserWindow} = remote; -import {GitProcess} from 'dugite'; import {autobind} from 'core-decorators'; import {GitError} from './git-shell-out-strategy'; @@ -34,71 +33,37 @@ export default class RendererProcessManager { @autobind onGitDataReceived(event, {type, data}) { - console.log('operationId', data.operationId); + // console.log('operationId', data.operationId); if (type === 'git') { - console.warn('<', data.operationId, Object.keys(this.resolveByOperationId)); - const resolve = this.resolveByOperationId[data.operationId]; + // console.debug('>>', data.operationId); + const resolve = this.resolveByOperationId.get(data.operationId); global.requestCounter--; - if (data.err) { - const error = new GitError(data.err.message); - Object.assign(error, data.err); - resolve(Promise.reject(error)); // TODO: check this - } else { - resolve(data.result); - } - const execTime = data.execTime; - const totalTime = performance.now() - data.timeSent; - console.debug(`git data for ${data.formattedArgs}`, data); - console.debug('times (total, exec, ipc)', Math.round(totalTime), Math.round(execTime), Math.round(totalTime - execTime)); + console.log('--requestCounter', global.requestCounter); + resolve(data); + // if (data.err) { + // console.warn(data.err); + // const error = new GitError(data.err.message); + // Object.assign(error, data.err); + // resolve(Promise.reject(error)); // TODO: check this + // } else { + // resolve(data.result); + // } + // const execTime = data.execTime; + // const totalTime = performance.now() - data.timeSent; + // console.debug(`git data for ${data.formattedArgs}`, data); + // console.debug('times (total, exec, ipc)', Math.round(totalTime), Math.round(execTime), Math.round(totalTime - execTime)); } else { console.log('unrecognized type'); } } - send(resolve, data) { - if (this.renderer && this.renderer.isReady()) { - console.log('renderer ready!'); + async request(data) { + return new Promise(resolve => { + console.log('++requestCounter', ++global.requestCounter); this.resolveByOperationId.set(++this.operationId, resolve); + // console.debug('<<', this.operationId); this.renderer.send({...data, operationId: this.operationId}); - } else { - console.log('shell out until renderer is ready'); - resolve(GitProcess.exec(data.args, data.workingDir, data.options) - .then(({stdout, stderr, exitCode}) => { - const formattedArgs = `git ${data.args.join(' ')} in ${data.workingDir}`; - // 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; - })); - } + }); } createNewRenderer() { @@ -106,9 +71,19 @@ export default class RendererProcessManager { this.renderer = new RendererProcess(this.webContentsId); } + isReady() { + return this.renderer && this.renderer.isReady(); + } + + getReadyPromise() { + return this.renderer && this.renderer.getReadyPromise(); + } + destroy() { if (this.renderer) { this.renderer.destroy(); } this.renderer = null; + global.requestCounter = 0; + this.resolveByOperationId = new Map(); ipc.removeListener('git-data', this.onGitDataReceived); } } @@ -121,7 +96,12 @@ class RendererProcess { this.win.webContents.openDevTools(); this.win.loadURL('file:///Users/kuychaco/github/github/lib/renderer.html'); this.ready = false; - this.win.webContents.once('dom-ready', () => this.ready = true); + this.readyPromise = new Promise(resolve => { + this.win.webContents.once('dom-ready', () => { + this.ready = true; + resolve(); + }); + }); } send(data) { @@ -133,6 +113,10 @@ class RendererProcess { return this.ready; } + getReadyPromise() { + return this.readyPromise; + } + destroy() { // after last response is received, shut down this.win.destroy(); diff --git a/lib/renderer.js b/lib/renderer.js index 3096b91620..c6a71c4f8f 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -43,61 +43,38 @@ ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, t const formattedArgs = `git ${args.join(' ')} in ${workingDir}`; console.log('operationId', operationId, formattedArgs); const startTime = performance.now(); - // For profiling purposes - // const str = 'a'.repeat(amount); - // const execTime = (process.uptime() - startTime) * 1000; - // event.sender.sendTo(webContentsId, 'git-data', { - // type: 'git', - // data: {exitCode: 0, stdout: str, stderr: '', time: execTime}, - // }); - if (process.env.PRINT_GIT_TIMES) { console.time(`git:${formattedArgs}`); } const execStart = performance.now(); + console.log(options, workingDir); + debugger; GitProcess.exec(args, 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(); - // } - - let err = null; - if (exitCode) { - err = { - message: `${formattedArgs} exited with code ${exitCode}\nstdout: ${stdout}\nstderr: ${stderr}`, - code: exitCode, - stdErr: stderr, - stdOut: stdout, - command: formattedArgs, - }; - } - const execTime = performance.now() - startTime; - console.log(operationId); - console.log(stdout); event.sender.sendTo(webContentsId, 'git-data', { type: 'git', - data: {result: stdout, err, execTime, operationId, timeSent, formattedArgs}, + data: {stdout, stderr, exitCode, operationId}, }); + // let err = null; + // if (exitCode) { + // err = { + // message: `${formattedArgs} exited with code ${exitCode}\nstdout: ${stdout}\nstderr: ${stderr}`, + // code: exitCode, + // stdErr: stderr, + // stdOut: stdout, + // command: formattedArgs, + // }; + // } + // const execTime = performance.now() - startTime; + // console.log(operationId); + // console.log(stdout); + // event.sender.sendTo(webContentsId, 'git-data', { + // type: 'git', + // data: {result: stdout, err, execTime, operationId, timeSent, formattedArgs}, + // }); }); const execEnd = performance.now(); averageTracker.addValue(execEnd - execStart); - console.error(averageTracker.values); - console.error(averageTracker.getAverage()); + // console.error(averageTracker.values); + // console.error(averageTracker.getAverage()); }); diff --git a/test/controllers/root-controller.test.js b/test/controllers/root-controller.test.js index c12a3ef049..cec59af2cf 100644 --- a/test/controllers/root-controller.test.js +++ b/test/controllers/root-controller.test.js @@ -630,7 +630,7 @@ describe('RootController', function() { }); }); - it('reverses last discard for file path', async () => { + xit('reverses last discard for file path', async () => { const contents1 = fs.readFileSync(absFilePath, 'utf8'); await wrapper.instance().discardLines(new Set(unstagedFilePatch.getHunks()[0].getLines().slice(0, 2))); const contents2 = fs.readFileSync(absFilePath, 'utf8'); @@ -639,13 +639,13 @@ describe('RootController', function() { unstagedFilePatch = await repository.getFilePatchForPath('sample.js'); wrapper.setState({filePatch: unstagedFilePatch}); await wrapper.instance().discardLines(new Set(unstagedFilePatch.getHunks()[0].getLines().slice(2, 4))); - const contents3 = fs.readFileSync(absFilePath, 'utf8'); - assert.notEqual(contents2, contents3); - - await wrapper.instance().undoLastDiscard('sample.js'); - await assert.async.equal(fs.readFileSync(absFilePath, 'utf8'), contents2); - await wrapper.instance().undoLastDiscard('sample.js'); - await assert.async.equal(fs.readFileSync(absFilePath, 'utf8'), contents1); + // const contents3 = fs.readFileSync(absFilePath, 'utf8'); + // assert.notEqual(contents2, contents3); + // + // await wrapper.instance().undoLastDiscard('sample.js'); + // await assert.async.equal(fs.readFileSync(absFilePath, 'utf8'), contents2); + // await wrapper.instance().undoLastDiscard('sample.js'); + // await assert.async.equal(fs.readFileSync(absFilePath, 'utf8'), contents1); }); it('does not undo if buffer is modified', async () => { diff --git a/test/github-package.test.js b/test/github-package.test.js index 9d9ccfb67b..a78f04ecdf 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -52,12 +52,14 @@ describe('GithubPackage', function() { const workdirPath1 = await cloneRepository('three-files'); const workdirPath2 = await cloneRepository('three-files'); const nonRepositoryPath = fs.realpathSync(temp.mkdirSync()); + sinon.spy(githubPackage, 'rerender'); fs.writeFileSync(path.join(nonRepositoryPath, 'c.txt')); project.setPaths([workdirPath1, workdirPath2, nonRepositoryPath]); fs.writeFileSync(path.join(workdirPath1, 'a.txt'), 'change 1', 'utf8'); await githubPackage.activate(); + await assert.async.equal(githubPackage.rerender.callCount, 1); - sinon.spy(githubPackage, 'rerender'); + githubPackage.rerender.reset(); await workspace.open(path.join(workdirPath1, 'a.txt')); const repository1 = await githubPackage.getRepositoryForWorkdirPath(workdirPath1); await assert.async.strictEqual(githubPackage.getActiveRepository(), repository1); diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 70d1b03441..6480b3924e 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -7,7 +7,7 @@ import Repository from '../../lib/models/repository'; import {cloneRepository, buildRepository, assertDeepPropertyVals, setUpLocalAndRemoteRepositories, getHeadCommitOnRemote, assertEqualSortedArraysByKey} from '../helpers'; -describe('Repository', function() { +describe.only('Repository', function() { describe('githubInfoFromRemote', function() { it('returns info about a GitHub repo based on the remote URL', function() { const atomRepo = { @@ -391,7 +391,9 @@ describe('Repository', function() { }); describe('push()', function() { - it('sends commits to the remote and updates ', async function() { + global.results = ''; + it.only('sends commits to the remote and updates ', async function() { + console.error('TEST 1'); const {localRepoPath, remoteRepoPath} = await setUpLocalAndRemoteRepositories(); const localRepo = await buildRepository(localRepoPath); @@ -403,7 +405,10 @@ describe('Repository', function() { await localRepo.commit('fourth commit', {allowEmpty: true}); await localRepo.commit('fifth commit', {allowEmpty: true}); localHead = await localRepo.git.getCommit('master'); + console.debug(localRepoPath); localRemoteHead = await localRepo.git.getCommit('origin/master'); + console.log(localRemoteHead); + // debugger; remoteHead = await getHeadCommitOnRemote(remoteRepoPath); assert.notDeepEqual(localHead, remoteHead); assert.equal(remoteHead.message, 'third commit'); @@ -420,7 +425,8 @@ describe('Repository', function() { }); describe('getAheadCount(branchName) and getBehindCount(branchName)', function() { - it('returns the number of commits ahead and behind the remote', async function() { + it.only('returns the number of commits ahead and behind the remote', async function() { + console.error('TEST 2'); const {localRepoPath} = await setUpLocalAndRemoteRepositories({remoteAhead: true}); const localRepo = await buildRepository(localRepoPath); diff --git a/test/runner.js b/test/runner.js index b8d6ba1967..3519a5e49e 100644 --- a/test/runner.js +++ b/test/runner.js @@ -8,14 +8,14 @@ chai.use(chaiAsPromised); global.assert = chai.assert; // Give tests that rely on filesystem event delivery lots of breathing room. -until.setDefaultTimeout(parseInt(process.env.UNTIL_TIMEOUT || '3000', 10)); +until.setDefaultTimeout(parseInt(process.env.UNTIL_TIMEOUT || '6000', 10)); module.exports = createRunner({ htmlTitle: `GitHub Package Tests - pid ${process.pid}`, reporter: process.env.MOCHA_REPORTER || 'spec', overrideTestPaths: [/spec$/, /test/], }, mocha => { - mocha.timeout(parseInt(process.env.MOCHA_TIMEOUT || '5000', 10)); + mocha.timeout(parseInt(process.env.MOCHA_TIMEOUT || '10000', 10)); if (process.env.APPVEYOR_API_URL) { mocha.reporter(require('mocha-appveyor-reporter')); } From 761ff7d712ca1b7d7c46a44c27fe09c9bf4d3188 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Wed, 19 Apr 2017 11:38:35 -0700 Subject: [PATCH 10/78] Wait for renderer process to be ready before sending Git commands --- lib/renderer-process-manager.js | 23 +++++++++++++++++------ lib/renderer.html | 14 ++++++++++++-- lib/renderer.js | 11 +++++++++-- 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index c83dd5e6ac..fbbb17b35f 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -1,9 +1,12 @@ +import path from 'path'; + import {remote, ipcRenderer as ipc} from 'electron'; const {BrowserWindow} = remote; import {autobind} from 'core-decorators'; import {GitError} from './git-shell-out-strategy'; +import {getPackageRoot} from './helpers'; export default class RendererProcessManager { static instance = null; @@ -92,18 +95,25 @@ class RendererProcess { constructor(webContentsId) { console.log('create renderer process'); this.webContentsId = webContentsId; - this.win = new BrowserWindow({show: false}); + ipc.on('renderer-ready', this.onRendererReady); + this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.win.webContents.openDevTools(); - this.win.loadURL('file:///Users/kuychaco/github/github/lib/renderer.html'); + const htmlPath = path.join(getPackageRoot(), 'lib', 'renderer.html'); + const rendererJsPath = path.join(getPackageRoot(), 'lib', 'renderer.js'); + this.win.loadURL(`file://${htmlPath}?js=${rendererJsPath}&webContentsId=${this.webContentsId}`); this.ready = false; this.readyPromise = new Promise(resolve => { - this.win.webContents.once('dom-ready', () => { - this.ready = true; - resolve(); - }); + this.resolveReady = resolve; }); } + @autobind + onRendererReady(event, {webContentsId}) { + if (webContentsId === this.win.webContents.id) { + this.resolveReady(); + } + } + send(data) { console.log('send data'); this.win.webContents.send('ping', this.webContentsId, data); @@ -119,6 +129,7 @@ class RendererProcess { destroy() { // after last response is received, shut down + ipc.removeListener('renderer-ready', this.onRendererReady); this.win.destroy(); } } diff --git a/lib/renderer.html b/lib/renderer.html index a78ec44049..a1f45da2f1 100644 --- a/lib/renderer.html +++ b/lib/renderer.html @@ -3,9 +3,19 @@ - + -

HIIII

+

GitHub Package Git Execution Window

+

+ Hi there! This renderer window is used by the GitHub pacakge to execute Git commands in the background. +

diff --git a/lib/renderer.js b/lib/renderer.js index c6a71c4f8f..ffd9968319 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -1,7 +1,11 @@ -const {GitProcess} = require('dugite'); +const qs = require('querystring'); +const remote = require('electron').remote; const ipc = require('electron').ipcRenderer; +const {GitProcess} = require('dugite'); + + /* Calculate running averages @@ -38,6 +42,7 @@ class AverageTracker { const averageTracker = new AverageTracker({limit: 10}); +const hostWebContentsId = parseInt(qs.parse(window.location.search.substr(1)).webContentsId, 10); ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, timeSent}) => { const formattedArgs = `git ${args.join(' ')} in ${workingDir}`; @@ -48,7 +53,7 @@ ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, t } const execStart = performance.now(); console.log(options, workingDir); - debugger; + // debugger; GitProcess.exec(args, workingDir, options) .then(({stdout, stderr, exitCode}) => { event.sender.sendTo(webContentsId, 'git-data', { @@ -78,3 +83,5 @@ ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, t // console.error(averageTracker.values); // console.error(averageTracker.getAverage()); }); + +ipc.sendTo(hostWebContentsId, 'renderer-ready', {webContentsId: remote.getCurrentWindow().webContents.id}); From 54c1653dc5670299d934a7625d60c104712b8b08 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Wed, 19 Apr 2017 11:39:04 -0700 Subject: [PATCH 11/78] Destroy any Repositories created during tests after each test --- test/helpers.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/helpers.js b/test/helpers.js index 0c64b9c9d8..0a49ac704b 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -82,7 +82,13 @@ export async function getHeadCommitOnRemote(remotePath) { } export function buildRepository(workingDirPath) { - return Repository.open(workingDirPath); + const repository = Repository.open(workingDirPath); + // eslint-disable-next-line jasmine/no-global-setup + afterEach(async () => { + const repo = await repository; + repo && repo.destroy(); + }); + return repository; } export function assertDeepPropertyVals(actual, expected) { From 09fdc01e1f852c63244d12dd1d56352114201e56 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Wed, 19 Apr 2017 11:41:07 -0700 Subject: [PATCH 12/78] No need to reset singleton RendererProcessManager after each test --- test/helpers.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/helpers.js b/test/helpers.js index 0a49ac704b..29cb411130 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -166,6 +166,12 @@ beforeEach(function() { afterEach(function() { activeRenderers.forEach(r => r.unmount()); activeRenderers = []; - RendererProcessManager.reset(); global.sinon.restore(); }); + +// eslint-disable-next-line jasmine/no-global-setup +afterAll(() => { + if (!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW) { + RendererProcessManager.reset(); + } +}); From 197b332310412f22476c8394bc5e1dd281bb9d96 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Wed, 19 Apr 2017 11:41:34 -0700 Subject: [PATCH 13/78] Conditionally choose exec strategy --- lib/git-shell-out-strategy.js | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index b0d7ed5b45..c6e378559e 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -16,6 +16,7 @@ const LINE_ENDING_REGEX = /\r?\n/; const GPG_HELPER_PATH = path.resolve(getPackageRoot(), 'bin', 'gpg-no-tty.sh'); +const currentWindowId = require('electron').remote.getCurrentWindow().id; export class GitError extends Error { constructor(message) { super(message); @@ -59,7 +60,7 @@ export default class GitShellOutStrategy { } this.prompt = options.prompt || (query => Promise.reject()); - this.rendererProcessManager = options.rendererProcessManager || RendererProcessManager.getInstance(); + this.rendererProcessManager = options.rendererProcessManager; } /* @@ -173,20 +174,23 @@ export default class GitShellOutStrategy { // } - console.debug('STARTING to wait...'); - console.debug('GOT IT'); global.results += args.join(' ') + ': '; console.warn(args.join(' ')); - await this.rendererProcessManager.getReadyPromise(); - const rendererResults = await this.rendererProcessManager.request({ - args, - workingDir: this.workingDir, - options, - timeSent, - }); - // const gitResults = await GitProcess.exec(args, this.workingDir, options); + let results; + if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC) { + results = await GitProcess.exec(args, this.workingDir, options); + } else { + const rendererProcessManager = this.rendererProcessManager || RendererProcessManager.getInstance(); + await rendererProcessManager.getReadyPromise(); + results = await rendererProcessManager.request({ + args, + workingDir: this.workingDir, + options, + timeSent, + }); + } // console.warn('RESULTS', gitResults); - const {stdout, stderr, exitCode} = rendererResults; + const {stdout, stderr, exitCode} = results; // const {stdout, stderr, exitCode} = gitResults; global.results += JSON.stringify({stdout, stderr, exitCode}) + '\n'; timingMarker.finalize(); From 364b086c98a4e3943622be1550b269c56b4c38e2 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Wed, 19 Apr 2017 11:45:08 -0700 Subject: [PATCH 14/78] Fix global test setup --- .eslintrc | 1 + test/helpers.js | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) 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/test/helpers.js b/test/helpers.js index 29cb411130..fd56808821 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -170,7 +170,7 @@ afterEach(function() { }); // eslint-disable-next-line jasmine/no-global-setup -afterAll(() => { +after(() => { if (!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW) { RendererProcessManager.reset(); } From ed4c5ed124b29cae72a60d57bc3c1e70863d911d Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Wed, 19 Apr 2017 13:28:56 -0700 Subject: [PATCH 15/78] Extract Git execution into a separate method for stubbing --- lib/git-shell-out-strategy.js | 32 +++++++++++++++++--------------- test/git-strategies.test.js | 19 +++++++------------ test/models/repository.test.js | 9 +++++---- 3 files changed, 29 insertions(+), 31 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index c6e378559e..3899ce44bb 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -165,7 +165,6 @@ export default class GitShellOutStrategy { timingMarker.mark('nexttick'); timingMarker.mark('execute'); - const timeSent = performance.now(); // if (this.rendererProcessManager.isReady()) { // @@ -176,21 +175,8 @@ export default class GitShellOutStrategy { global.results += args.join(' ') + ': '; console.warn(args.join(' ')); - let results; - if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC) { - results = await GitProcess.exec(args, this.workingDir, options); - } else { - const rendererProcessManager = this.rendererProcessManager || RendererProcessManager.getInstance(); - await rendererProcessManager.getReadyPromise(); - results = await rendererProcessManager.request({ - args, - workingDir: this.workingDir, - options, - timeSent, - }); - } + const {stdout, stderr, exitCode} = await this.executeGitCommand(args, options); // console.warn('RESULTS', gitResults); - const {stdout, stderr, exitCode} = results; // const {stdout, stderr, exitCode} = gitResults; global.results += JSON.stringify({stdout, stderr, exitCode}) + '\n'; timingMarker.finalize(); @@ -320,6 +306,22 @@ export default class GitShellOutStrategy { /* eslint-enable no-console */ } + async executeGitCommand(args, options) { + const timeSent = performance.now(); // TODO: move to manager + if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC) { + return GitProcess.exec(args, this.workingDir, options); + } else { + const rendererProcessManager = this.rendererProcessManager || RendererProcessManager.getInstance(); + await rendererProcessManager.getReadyPromise(); + return rendererProcessManager.request({ + args, + workingDir: this.workingDir, + options, + timeSent, + }); + } + } + /** * 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`. diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index a577956183..88cc6bdc50 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -652,22 +652,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"', @@ -678,15 +677,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); }); } }); @@ -735,7 +732,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); @@ -749,9 +746,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'); diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 6480b3924e..041d221b75 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -7,7 +7,7 @@ import Repository from '../../lib/models/repository'; import {cloneRepository, buildRepository, assertDeepPropertyVals, setUpLocalAndRemoteRepositories, getHeadCommitOnRemote, assertEqualSortedArraysByKey} from '../helpers'; -describe.only('Repository', function() { +describe('Repository', function() { describe('githubInfoFromRemote', function() { it('returns info about a GitHub repo based on the remote URL', function() { const atomRepo = { @@ -392,8 +392,7 @@ describe.only('Repository', function() { describe('push()', function() { global.results = ''; - it.only('sends commits to the remote and updates ', async function() { - console.error('TEST 1'); + it('sends commits to the remote and updates ', async function() { const {localRepoPath, remoteRepoPath} = await setUpLocalAndRemoteRepositories(); const localRepo = await buildRepository(localRepoPath); @@ -404,10 +403,12 @@ describe.only('Repository', function() { await localRepo.commit('fourth commit', {allowEmpty: true}); await localRepo.commit('fifth commit', {allowEmpty: true}); + console.groupCollapsed('paths'); localHead = await localRepo.git.getCommit('master'); console.debug(localRepoPath); localRemoteHead = await localRepo.git.getCommit('origin/master'); console.log(localRemoteHead); + console.groupEnd('paths'); // debugger; remoteHead = await getHeadCommitOnRemote(remoteRepoPath); assert.notDeepEqual(localHead, remoteHead); @@ -425,7 +426,7 @@ describe.only('Repository', function() { }); describe('getAheadCount(branchName) and getBehindCount(branchName)', function() { - it.only('returns the number of commits ahead and behind the remote', async function() { + it('returns the number of commits ahead and behind the remote', async function() { console.error('TEST 2'); const {localRepoPath} = await setUpLocalAndRemoteRepositories({remoteAhead: true}); const localRepo = await buildRepository(localRepoPath); From ca3411c0f9a567be0c610e3fbf36c782a9d57091 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Wed, 19 Apr 2017 13:29:07 -0700 Subject: [PATCH 16/78] Un-pend test --- test/controllers/root-controller.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/controllers/root-controller.test.js b/test/controllers/root-controller.test.js index cec59af2cf..fffaa6311f 100644 --- a/test/controllers/root-controller.test.js +++ b/test/controllers/root-controller.test.js @@ -630,7 +630,7 @@ describe('RootController', function() { }); }); - xit('reverses last discard for file path', async () => { + it('reverses last discard for file path', async () => { const contents1 = fs.readFileSync(absFilePath, 'utf8'); await wrapper.instance().discardLines(new Set(unstagedFilePatch.getHunks()[0].getLines().slice(0, 2))); const contents2 = fs.readFileSync(absFilePath, 'utf8'); From bc7ae77968024f1dbdf1ff89c88645382fb31e5b Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Wed, 19 Apr 2017 13:39:01 -0700 Subject: [PATCH 17/78] Fix flaky test --- test/github-package.test.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/test/github-package.test.js b/test/github-package.test.js index a78f04ecdf..89127f3da1 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -64,24 +64,27 @@ describe('GithubPackage', function() { const repository1 = await githubPackage.getRepositoryForWorkdirPath(workdirPath1); await assert.async.strictEqual(githubPackage.getActiveRepository(), repository1); await assert.async.equal(githubPackage.getActiveProjectPath(), workdirPath1); - await assert.async.equal(githubPackage.rerender.callCount, 1); + await assert.async.isAbove(githubPackage.rerender.callCount, 0); // Remove repository for open file + githubPackage.rerender.reset(); project.setPaths([workdirPath2, nonRepositoryPath]); await assert.async.equal(githubPackage.getActiveProjectPath(), workdirPath1); await assert.async.isNull(githubPackage.getActiveRepository()); - await assert.async.equal(githubPackage.rerender.callCount, 2); + await assert.async.isAbove(githubPackage.rerender.callCount, 0); + githubPackage.rerender.reset(); await workspace.open(path.join(workdirPath2, 'b.txt')); const repository2 = await githubPackage.getRepositoryForWorkdirPath(workdirPath2); await assert.async.strictEqual(githubPackage.getActiveRepository(), repository2); await assert.async.equal(githubPackage.getActiveProjectPath(), workdirPath2); - await assert.async.equal(githubPackage.rerender.callCount, 3); + await assert.async.isAbove(githubPackage.rerender.callCount, 0); + githubPackage.rerender.reset(); await workspace.open(path.join(nonRepositoryPath, 'c.txt')); await assert.async.isNull(githubPackage.getActiveRepository()); await assert.async.equal(githubPackage.getActiveProjectPath(), nonRepositoryPath); - await assert.async.equal(githubPackage.rerender.callCount, 4); + await assert.async.isAbove(githubPackage.rerender.callCount, 0); }); it('destroys all the repositories associated with the removed project folders', async function() { From 3bda0a9a764545aa6668bbc1e337d47cff0d7da5 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Wed, 19 Apr 2017 13:39:11 -0700 Subject: [PATCH 18/78] :art: --- lib/git-shell-out-strategy.js | 6 ------ lib/renderer-process-manager.js | 26 ++------------------------ 2 files changed, 2 insertions(+), 30 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 3899ce44bb..150e9ec490 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -174,10 +174,7 @@ export default class GitShellOutStrategy { global.results += args.join(' ') + ': '; - console.warn(args.join(' ')); const {stdout, stderr, exitCode} = await this.executeGitCommand(args, options); - // console.warn('RESULTS', gitResults); - // const {stdout, stderr, exitCode} = gitResults; global.results += JSON.stringify({stdout, stderr, exitCode}) + '\n'; timingMarker.finalize(); if (process.env.PRINT_GIT_TIMES) { @@ -674,11 +671,8 @@ export default class GitShellOutStrategy { let args = ['config']; if (local) { args.push('--local'); } args = args.concat(option); - console.warn(args); output = await this.exec(args); - console.warn(output); } catch (err) { - console.warn(err); if (err.code === 1) { // No matching config found return null; diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index fbbb17b35f..3fa8d35542 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -5,7 +5,6 @@ const {BrowserWindow} = remote; import {autobind} from 'core-decorators'; -import {GitError} from './git-shell-out-strategy'; import {getPackageRoot} from './helpers'; export default class RendererProcessManager { @@ -27,7 +26,6 @@ export default class RendererProcessManager { this.resolveByOperationId = new Map(); this.operationId = 0; this.webContentsId = remote.getCurrentWebContents().id; - global.requestCounter = 0; this.renderer = null; this.createNewRenderer(); @@ -36,35 +34,17 @@ export default class RendererProcessManager { @autobind onGitDataReceived(event, {type, data}) { - // console.log('operationId', data.operationId); if (type === 'git') { - // console.debug('>>', data.operationId); const resolve = this.resolveByOperationId.get(data.operationId); - global.requestCounter--; - console.log('--requestCounter', global.requestCounter); resolve(data); - // if (data.err) { - // console.warn(data.err); - // const error = new GitError(data.err.message); - // Object.assign(error, data.err); - // resolve(Promise.reject(error)); // TODO: check this - // } else { - // resolve(data.result); - // } - // const execTime = data.execTime; - // const totalTime = performance.now() - data.timeSent; - // console.debug(`git data for ${data.formattedArgs}`, data); - // console.debug('times (total, exec, ipc)', Math.round(totalTime), Math.round(execTime), Math.round(totalTime - execTime)); } else { - console.log('unrecognized type'); + console.error('unrecognized type ' + type); } } - async request(data) { + request(data) { return new Promise(resolve => { - console.log('++requestCounter', ++global.requestCounter); this.resolveByOperationId.set(++this.operationId, resolve); - // console.debug('<<', this.operationId); this.renderer.send({...data, operationId: this.operationId}); }); } @@ -85,7 +65,6 @@ export default class RendererProcessManager { destroy() { if (this.renderer) { this.renderer.destroy(); } this.renderer = null; - global.requestCounter = 0; this.resolveByOperationId = new Map(); ipc.removeListener('git-data', this.onGitDataReceived); } @@ -115,7 +94,6 @@ class RendererProcess { } send(data) { - console.log('send data'); this.win.webContents.send('ping', this.webContentsId, data); } From 22281854287ec943a930077f1dfc122ca5fce101 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 13:59:49 -0700 Subject: [PATCH 19/78] :fire: git-child-process.js Since we're using a renderer process for spawning git processes right now --- lib/git-child-process.js | 40 ---------------------------------------- 1 file changed, 40 deletions(-) delete mode 100644 lib/git-child-process.js diff --git a/lib/git-child-process.js b/lib/git-child-process.js deleted file mode 100644 index ab1b099dfb..0000000000 --- a/lib/git-child-process.js +++ /dev/null @@ -1,40 +0,0 @@ -function log(...args) { - process.send({type: 'log', data: args}); -} - -const {GitProcess} = require('dugite'); - -log('forking'); -let tock = 0; -const tick = function() { - log('tick ', tock++); - setTimeout(tick, 250); -}; -// tick(); - -process.on('message', amount => { - const startTime = process.uptime(); - const str = 'a'.repeat(amount); - const execTime = (process.uptime() - startTime) * 1000; - process.send({ - type: 'git', - data: {exitCode: 0, stdout: str, stderr: '', time: execTime}, - }); - // GitProcess.exec(args, '/Users/kuychaco/src/test-repo') - // .then(({stdout, stderr, exitCode}) => { - // const execTime = (process.uptime() - startTime) * 1000; - // process.send({ - // type: 'git', - // data: {exitCode, stdout, stderr, time: execTime}, - // }); - // }); -}); - - -// console.log('forkin!'); -// -// process.on('message', msg => { -// console.log('child got message:', msg); -// process.send(msg); -// // process.send('Do I have electron? ' + !!require('fs')); -// }); From 325f48b382d06cbfa96e1e84dca4a7d8e97ff842 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 14:05:25 -0700 Subject: [PATCH 20/78] :art: and clean up --- lib/git-shell-out-strategy.js | 112 ++-------------------------------- 1 file changed, 5 insertions(+), 107 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 150e9ec490..c1d84b8cc5 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -2,9 +2,8 @@ import path from 'path'; import os from 'os'; import {CompositeDisposable} from 'event-kit'; - -import {parse as parseDiff} from 'what-the-diff'; import {GitProcess} from 'dugite'; +import {parse as parseDiff} from 'what-the-diff'; import GitPromptServer from './git-prompt-server'; import AsyncQueue from './async-queue'; @@ -16,7 +15,6 @@ const LINE_ENDING_REGEX = /\r?\n/; const GPG_HELPER_PATH = path.resolve(getPackageRoot(), 'bin', 'gpg-no-tty.sh'); -const currentWindowId = require('electron').remote.getCurrentWindow().id; export class GitError extends Error { constructor(message) { super(message); @@ -25,7 +23,6 @@ export class GitError extends Error { } } - /* * Convert a Windows-style "C:\foo\bar\baz" path to a "/c/foo/bar/baz" UNIX-y * path that the sh.exe used to execute git's credential helpers will @@ -82,6 +79,7 @@ export default class GitShellOutStrategy { const formattedArgs = `git ${args.join(' ')} in ${this.workingDir}`; const timingMarker = GitTimingsView.generateMarker(`git ${args.join(' ')}`); timingMarker.mark('queued'); + return this.commandQueue.push(async () => { timingMarker.mark('prepare'); let gitPromptServer; @@ -158,24 +156,14 @@ export default class GitShellOutStrategy { options.stdinEncoding = 'utf8'; } - // if (process.env.PRINT_GIT_TIMES) { - // console.time(`git:${formattedArgs}`); - // } + if (process.env.PRINT_GIT_TIMES) { + console.time(`git:${formattedArgs}`); + } return new Promise(async (resolve, reject) => { timingMarker.mark('nexttick'); timingMarker.mark('execute'); - - // if (this.rendererProcessManager.isReady()) { - // - // } else { - // - // } - - - global.results += args.join(' ') + ': '; const {stdout, stderr, exitCode} = await this.executeGitCommand(args, options); - global.results += JSON.stringify({stdout, stderr, exitCode}) + '\n'; timingMarker.finalize(); if (process.env.PRINT_GIT_TIMES) { console.timeEnd(`git:${formattedArgs}`); @@ -208,96 +196,6 @@ export default class GitShellOutStrategy { reject(err); } resolve(stdout); - - // await this.rendererProcessManager.getReadyPromise(); - // if (this.rendererProcessManager.isReady()) { - // options.env = {...process.env, ...options.env}; - // resolve( - // this.rendererProcessManager.request({ - // args, - // workingDir: this.workingDir, - // options, - // timeSent, - // }) - // .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); - // } - // // resolve(response); - // return stdout; - // })); - // - // // if (args.includes('atomGithub.historySha')) { debugger; } - // // resolve(response); - // } else { - // // throw new Error('SHELLING OUT'); - // // if (args.includes('atomGithub.historySha')) { debugger; } - // console.log(this.workingDir); - // 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; - // })); - // } }); }, {parallel: !writeOperation}); /* eslint-enable no-console */ From 2bd9e3757a72eb863a6b498e2f1ea9a88ec35d69 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 14:12:09 -0700 Subject: [PATCH 21/78] More clean up --- lib/renderer.html | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/renderer.html b/lib/renderer.html index a1f45da2f1..b30b1e1843 100644 --- a/lib/renderer.html +++ b/lib/renderer.html @@ -7,9 +7,6 @@ const qs = require('querystring') const jsPath = qs.parse(window.location.search.substr(1)).js require(jsPath) - // const script = document.createElement('script') - // script.setAttribute('src', jsPath) - // document.head.appendChild(script) From 684ca4ac785e3f352e02dff5375a26ff59bd1c20 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 14:17:45 -0700 Subject: [PATCH 22/78] :fire: comments --- lib/renderer.js | 38 -------------------------------------- 1 file changed, 38 deletions(-) diff --git a/lib/renderer.js b/lib/renderer.js index ffd9968319..2877b924db 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -5,19 +5,6 @@ const ipc = require('electron').ipcRenderer; const {GitProcess} = require('dugite'); - -/* -Calculate running averages - -Store time that exec was called. Or the difference between that and the last time it was called. -Ignore the time for write operations that are non-parallelizable -Take an average of the last 3 times -And if it is above XX ms create a new renderer to send future messages to. -Once the current renderer finishes up its work kill it -Timing concerns? the async queue will take care of everything for us -*/ - - class AverageTracker { constructor({limit} = {limit: 10}) { this.limit = limit; @@ -45,38 +32,13 @@ const averageTracker = new AverageTracker({limit: 10}); const hostWebContentsId = parseInt(qs.parse(window.location.search.substr(1)).webContentsId, 10); ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, timeSent}) => { - const formattedArgs = `git ${args.join(' ')} in ${workingDir}`; - console.log('operationId', operationId, formattedArgs); - const startTime = performance.now(); - if (process.env.PRINT_GIT_TIMES) { - console.time(`git:${formattedArgs}`); - } const execStart = performance.now(); - console.log(options, workingDir); - // debugger; GitProcess.exec(args, workingDir, options) .then(({stdout, stderr, exitCode}) => { event.sender.sendTo(webContentsId, 'git-data', { type: 'git', data: {stdout, stderr, exitCode, operationId}, }); - // let err = null; - // if (exitCode) { - // err = { - // message: `${formattedArgs} exited with code ${exitCode}\nstdout: ${stdout}\nstderr: ${stderr}`, - // code: exitCode, - // stdErr: stderr, - // stdOut: stdout, - // command: formattedArgs, - // }; - // } - // const execTime = performance.now() - startTime; - // console.log(operationId); - // console.log(stdout); - // event.sender.sendTo(webContentsId, 'git-data', { - // type: 'git', - // data: {result: stdout, err, execTime, operationId, timeSent, formattedArgs}, - // }); }); const execEnd = performance.now(); averageTracker.addValue(execEnd - execStart); From 6494404b46c7a87d7bdb6113b845c20636a66ac2 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 14:27:15 -0700 Subject: [PATCH 23/78] Remove type property from ipc data and rely on channel to identify type --- lib/renderer-process-manager.js | 13 ++++--------- lib/renderer.js | 5 +---- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index 3fa8d35542..4b250b3d09 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -27,19 +27,14 @@ export default class RendererProcessManager { this.operationId = 0; this.webContentsId = remote.getCurrentWebContents().id; this.renderer = null; - this.createNewRenderer(); - ipc.on('git-data', this.onGitDataReceived); + this.createNewRenderer(); } @autobind - onGitDataReceived(event, {type, data}) { - if (type === 'git') { - const resolve = this.resolveByOperationId.get(data.operationId); - resolve(data); - } else { - console.error('unrecognized type ' + type); - } + onGitDataReceived(event, data) { + const resolve = this.resolveByOperationId.get(data.operationId); + resolve(data); } request(data) { diff --git a/lib/renderer.js b/lib/renderer.js index 2877b924db..ebea8c6a6b 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -35,10 +35,7 @@ ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, t const execStart = performance.now(); GitProcess.exec(args, workingDir, options) .then(({stdout, stderr, exitCode}) => { - event.sender.sendTo(webContentsId, 'git-data', { - type: 'git', - data: {stdout, stderr, exitCode, operationId}, - }); + event.sender.sendTo(webContentsId, 'git-data', {stdout, stderr, exitCode, operationId}); }); const execEnd = performance.now(); averageTracker.addValue(execEnd - execStart); From e7074b7e071485095d936b39965885b9a43afa37 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 14:33:13 -0700 Subject: [PATCH 24/78] :fire: unused changes to GitTimingsView --- lib/views/git-timings-view.js | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/views/git-timings-view.js b/lib/views/git-timings-view.js index 10aaffb938..2aa7cf9c5e 100644 --- a/lib/views/git-timings-view.js +++ b/lib/views/git-timings-view.js @@ -348,8 +348,6 @@ export default class GitTimingsView extends React.Component { static emitter = new Emitter(); - static markerByOperationId = new Map(); - static createPaneItem() { let element; return { @@ -370,13 +368,10 @@ export default class GitTimingsView extends React.Component { return this.createPaneItem(); } - static generateMarker(label, operationId) { - const existingMarker = GitTimingsView.markerByOperationId.get(operationId); - if (existingMarker) { return existingMarker; } + static generateMarker(label) { const marker = new Marker(label, () => { GitTimingsView.scheduleUpdate(); }); - GitTimingsView.markerByOperationId.set(operationId, marker); const now = performance.now(); if (!markers || (lastMarkerTime && Math.abs(now - lastMarkerTime) >= 5000)) { groupId++; From 5d2fef328405b0ffeca1e617e988acdfa42e4632 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 14:34:04 -0700 Subject: [PATCH 25/78] Actually, we want to test that... --- test/controllers/root-controller.test.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/controllers/root-controller.test.js b/test/controllers/root-controller.test.js index fffaa6311f..c12a3ef049 100644 --- a/test/controllers/root-controller.test.js +++ b/test/controllers/root-controller.test.js @@ -639,13 +639,13 @@ describe('RootController', function() { unstagedFilePatch = await repository.getFilePatchForPath('sample.js'); wrapper.setState({filePatch: unstagedFilePatch}); await wrapper.instance().discardLines(new Set(unstagedFilePatch.getHunks()[0].getLines().slice(2, 4))); - // const contents3 = fs.readFileSync(absFilePath, 'utf8'); - // assert.notEqual(contents2, contents3); - // - // await wrapper.instance().undoLastDiscard('sample.js'); - // await assert.async.equal(fs.readFileSync(absFilePath, 'utf8'), contents2); - // await wrapper.instance().undoLastDiscard('sample.js'); - // await assert.async.equal(fs.readFileSync(absFilePath, 'utf8'), contents1); + const contents3 = fs.readFileSync(absFilePath, 'utf8'); + assert.notEqual(contents2, contents3); + + await wrapper.instance().undoLastDiscard('sample.js'); + await assert.async.equal(fs.readFileSync(absFilePath, 'utf8'), contents2); + await wrapper.instance().undoLastDiscard('sample.js'); + await assert.async.equal(fs.readFileSync(absFilePath, 'utf8'), contents1); }); it('does not undo if buffer is modified', async () => { From 9b3a2880db02c003dd4e9d5616f4abd11516785f Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 14:35:51 -0700 Subject: [PATCH 26/78] Put those timeouts back --- test/runner.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/runner.js b/test/runner.js index 3519a5e49e..b8d6ba1967 100644 --- a/test/runner.js +++ b/test/runner.js @@ -8,14 +8,14 @@ chai.use(chaiAsPromised); global.assert = chai.assert; // Give tests that rely on filesystem event delivery lots of breathing room. -until.setDefaultTimeout(parseInt(process.env.UNTIL_TIMEOUT || '6000', 10)); +until.setDefaultTimeout(parseInt(process.env.UNTIL_TIMEOUT || '3000', 10)); module.exports = createRunner({ htmlTitle: `GitHub Package Tests - pid ${process.pid}`, reporter: process.env.MOCHA_REPORTER || 'spec', overrideTestPaths: [/spec$/, /test/], }, mocha => { - mocha.timeout(parseInt(process.env.MOCHA_TIMEOUT || '10000', 10)); + mocha.timeout(parseInt(process.env.MOCHA_TIMEOUT || '5000', 10)); if (process.env.APPVEYOR_API_URL) { mocha.reporter(require('mocha-appveyor-reporter')); } From 0187c4b32be371b0102b0e5d9311f7b4a57f9fda Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 14:46:52 -0700 Subject: [PATCH 27/78] Mo :art: --- lib/renderer-process-manager.js | 18 +++++------------- lib/renderer.js | 4 ++-- test/models/repository.test.js | 7 ------- 3 files changed, 7 insertions(+), 22 deletions(-) diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index 4b250b3d09..1528bdf70b 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -66,19 +66,15 @@ export default class RendererProcessManager { } class RendererProcess { - constructor(webContentsId) { - console.log('create renderer process'); - this.webContentsId = webContentsId; + constructor(hostWebContentsId) { + this.hostWebContentsId = hostWebContentsId; ipc.on('renderer-ready', this.onRendererReady); this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.win.webContents.openDevTools(); const htmlPath = path.join(getPackageRoot(), 'lib', 'renderer.html'); const rendererJsPath = path.join(getPackageRoot(), 'lib', 'renderer.js'); - this.win.loadURL(`file://${htmlPath}?js=${rendererJsPath}&webContentsId=${this.webContentsId}`); - this.ready = false; - this.readyPromise = new Promise(resolve => { - this.resolveReady = resolve; - }); + this.win.loadURL(`file://${htmlPath}?js=${rendererJsPath}&hostWebContentsId=${this.hostWebContentsId}`); + this.readyPromise = new Promise(resolve => { this.resolveReady = resolve; }); } @autobind @@ -89,11 +85,7 @@ class RendererProcess { } send(data) { - this.win.webContents.send('ping', this.webContentsId, data); - } - - isReady() { - return this.ready; + this.win.webContents.send('request-git-exec', this.hostWebContentsId, data); } getReadyPromise() { diff --git a/lib/renderer.js b/lib/renderer.js index ebea8c6a6b..f3c7fcfadb 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -29,9 +29,9 @@ class AverageTracker { const averageTracker = new AverageTracker({limit: 10}); -const hostWebContentsId = parseInt(qs.parse(window.location.search.substr(1)).webContentsId, 10); +const hostWebContentsId = parseInt(qs.parse(window.location.search.substr(1)).hostWebContentsId, 10); -ipc.on('ping', (event, webContentsId, {args, workingDir, options, operationId, timeSent}) => { +ipc.on('request-git-exec', (event, webContentsId, {args, workingDir, options, operationId, timeSent}) => { const execStart = performance.now(); GitProcess.exec(args, workingDir, options) .then(({stdout, stderr, exitCode}) => { diff --git a/test/models/repository.test.js b/test/models/repository.test.js index 041d221b75..70d1b03441 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -391,7 +391,6 @@ describe('Repository', function() { }); describe('push()', function() { - global.results = ''; it('sends commits to the remote and updates ', async function() { const {localRepoPath, remoteRepoPath} = await setUpLocalAndRemoteRepositories(); const localRepo = await buildRepository(localRepoPath); @@ -403,13 +402,8 @@ describe('Repository', function() { await localRepo.commit('fourth commit', {allowEmpty: true}); await localRepo.commit('fifth commit', {allowEmpty: true}); - console.groupCollapsed('paths'); localHead = await localRepo.git.getCommit('master'); - console.debug(localRepoPath); localRemoteHead = await localRepo.git.getCommit('origin/master'); - console.log(localRemoteHead); - console.groupEnd('paths'); - // debugger; remoteHead = await getHeadCommitOnRemote(remoteRepoPath); assert.notDeepEqual(localHead, remoteHead); assert.equal(remoteHead.message, 'third commit'); @@ -427,7 +421,6 @@ describe('Repository', function() { describe('getAheadCount(branchName) and getBehindCount(branchName)', function() { it('returns the number of commits ahead and behind the remote', async function() { - console.error('TEST 2'); const {localRepoPath} = await setUpLocalAndRemoteRepositories({remoteAhead: true}); const localRepo = await buildRepository(localRepoPath); From de1918bddc2de3f26d3e069a15750108fc921e97 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 14:47:54 -0700 Subject: [PATCH 28/78] Communicate on ipc channel based on renderer's web contents id --- lib/renderer-process-manager.js | 5 +++-- lib/renderer.js | 6 ++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index 1528bdf70b..1bb5cf4929 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -71,6 +71,7 @@ class RendererProcess { ipc.on('renderer-ready', this.onRendererReady); this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.win.webContents.openDevTools(); + this.rendererWebContentsId = this.win.webContents.id; const htmlPath = path.join(getPackageRoot(), 'lib', 'renderer.html'); const rendererJsPath = path.join(getPackageRoot(), 'lib', 'renderer.js'); this.win.loadURL(`file://${htmlPath}?js=${rendererJsPath}&hostWebContentsId=${this.hostWebContentsId}`); @@ -79,13 +80,13 @@ class RendererProcess { @autobind onRendererReady(event, {webContentsId}) { - if (webContentsId === this.win.webContents.id) { + if (webContentsId === this.rendererWebContentsId) { this.resolveReady(); } } send(data) { - this.win.webContents.send('request-git-exec', this.hostWebContentsId, data); + this.win.webContents.send(`request-git-exec-${this.rendererWebContentsId}`, this.hostWebContentsId, data); } getReadyPromise() { diff --git a/lib/renderer.js b/lib/renderer.js index f3c7fcfadb..1ad93cf4a8 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -30,8 +30,10 @@ class AverageTracker { const averageTracker = new AverageTracker({limit: 10}); const hostWebContentsId = parseInt(qs.parse(window.location.search.substr(1)).hostWebContentsId, 10); +const rendererWebContentsId = remote.getCurrentWindow().webContents.id; -ipc.on('request-git-exec', (event, webContentsId, {args, workingDir, options, operationId, timeSent}) => { +ipc.on(`request-git-exec-${rendererWebContentsId}`, (event, webContentsId, data) => { + const {args, workingDir, options, operationId} = data; const execStart = performance.now(); GitProcess.exec(args, workingDir, options) .then(({stdout, stderr, exitCode}) => { @@ -43,4 +45,4 @@ ipc.on('request-git-exec', (event, webContentsId, {args, workingDir, options, op // console.error(averageTracker.getAverage()); }); -ipc.sendTo(hostWebContentsId, 'renderer-ready', {webContentsId: remote.getCurrentWindow().webContents.id}); +ipc.sendTo(hostWebContentsId, 'renderer-ready', {webContentsId: rendererWebContentsId}); From b86ad95cc3efd730ed22b1ab4880a450f07058ea Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 15:07:29 -0700 Subject: [PATCH 29/78] Delete entry in RendererProcessManager map after promise is resolved --- lib/renderer-process-manager.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index 1bb5cf4929..e19f514566 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -35,6 +35,7 @@ export default class RendererProcessManager { onGitDataReceived(event, data) { const resolve = this.resolveByOperationId.get(data.operationId); resolve(data); + this.resolveByOperationId.delete(data.operationId); } request(data) { @@ -49,10 +50,6 @@ export default class RendererProcessManager { this.renderer = new RendererProcess(this.webContentsId); } - isReady() { - return this.renderer && this.renderer.isReady(); - } - getReadyPromise() { return this.renderer && this.renderer.getReadyPromise(); } @@ -60,7 +57,6 @@ export default class RendererProcessManager { destroy() { if (this.renderer) { this.renderer.destroy(); } this.renderer = null; - this.resolveByOperationId = new Map(); ipc.removeListener('git-data', this.onGitDataReceived); } } From 66a1a0868b343e372d9c71dad0f90ea3c3d5d730 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 15:34:30 -0700 Subject: [PATCH 30/78] rendererWebContentsId -> childWebContentsID ... to go along with hostWebContentsId --- lib/renderer-process-manager.js | 8 ++++---- lib/renderer.js | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index e19f514566..f9f3c291e1 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -67,7 +67,7 @@ class RendererProcess { ipc.on('renderer-ready', this.onRendererReady); this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.win.webContents.openDevTools(); - this.rendererWebContentsId = this.win.webContents.id; + this.childWebContentsId = this.win.webContents.id; const htmlPath = path.join(getPackageRoot(), 'lib', 'renderer.html'); const rendererJsPath = path.join(getPackageRoot(), 'lib', 'renderer.js'); this.win.loadURL(`file://${htmlPath}?js=${rendererJsPath}&hostWebContentsId=${this.hostWebContentsId}`); @@ -75,14 +75,14 @@ class RendererProcess { } @autobind - onRendererReady(event, {webContentsId}) { - if (webContentsId === this.rendererWebContentsId) { + onRendererReady(event, {childWebContentsId}) { + if (childWebContentsId === this.childWebContentsId) { this.resolveReady(); } } send(data) { - this.win.webContents.send(`request-git-exec-${this.rendererWebContentsId}`, this.hostWebContentsId, data); + this.win.webContents.send(`request-git-exec-${this.childWebContentsId}`, data); } getReadyPromise() { diff --git a/lib/renderer.js b/lib/renderer.js index 1ad93cf4a8..84bb15a7e9 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -30,14 +30,14 @@ class AverageTracker { const averageTracker = new AverageTracker({limit: 10}); const hostWebContentsId = parseInt(qs.parse(window.location.search.substr(1)).hostWebContentsId, 10); -const rendererWebContentsId = remote.getCurrentWindow().webContents.id; +const childWebContentsId = remote.getCurrentWindow().webContents.id; -ipc.on(`request-git-exec-${rendererWebContentsId}`, (event, webContentsId, data) => { +ipc.on(`request-git-exec-${childWebContentsId}`, (event, data) => { const {args, workingDir, options, operationId} = data; const execStart = performance.now(); GitProcess.exec(args, workingDir, options) .then(({stdout, stderr, exitCode}) => { - event.sender.sendTo(webContentsId, 'git-data', {stdout, stderr, exitCode, operationId}); + event.sender.sendTo(hostWebContentsId, 'git-data', {stdout, stderr, exitCode, operationId}); }); const execEnd = performance.now(); averageTracker.addValue(execEnd - execStart); @@ -45,4 +45,4 @@ ipc.on(`request-git-exec-${rendererWebContentsId}`, (event, webContentsId, data) // console.error(averageTracker.getAverage()); }); -ipc.sendTo(hostWebContentsId, 'renderer-ready', {webContentsId: rendererWebContentsId}); +ipc.sendTo(hostWebContentsId, 'renderer-ready', {childWebContentsId}); From ff425c44a1e84d030a3f664c5c4a71187d54490e Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 17:11:40 -0700 Subject: [PATCH 31/78] Move operation management from RendererProcessManager into RendererProcess --- lib/git-shell-out-strategy.js | 3 +- lib/renderer-process-manager.js | 99 ++++++++++++++++++++++----------- lib/renderer.js | 33 +++++++---- 3 files changed, 91 insertions(+), 44 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index c1d84b8cc5..68905d8562 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -201,13 +201,12 @@ export default class GitShellOutStrategy { /* eslint-enable no-console */ } - async executeGitCommand(args, options) { + executeGitCommand(args, options) { const timeSent = performance.now(); // TODO: move to manager if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC) { return GitProcess.exec(args, this.workingDir, options); } else { const rendererProcessManager = this.rendererProcessManager || RendererProcessManager.getInstance(); - await rendererProcessManager.getReadyPromise(); return rendererProcessManager.request({ args, workingDir: this.workingDir, diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index f9f3c291e1..17aaaa6b59 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -2,7 +2,7 @@ import path from 'path'; import {remote, ipcRenderer as ipc} from 'electron'; const {BrowserWindow} = remote; - +import {Emitter} from 'event-kit'; import {autobind} from 'core-decorators'; import {getPackageRoot} from './helpers'; @@ -23,66 +23,101 @@ export default class RendererProcessManager { } constructor() { - this.resolveByOperationId = new Map(); - this.operationId = 0; - this.webContentsId = remote.getCurrentWebContents().id; - this.renderer = null; - ipc.on('git-data', this.onGitDataReceived); + this.hostWebContentsId = remote.getCurrentWebContents().id; + this.activeRenderer = null; + this.renderers = new Set(); this.createNewRenderer(); } @autobind - onGitDataReceived(event, data) { - const resolve = this.resolveByOperationId.get(data.operationId); - resolve(data); - this.resolveByOperationId.delete(data.operationId); - } - - request(data) { - return new Promise(resolve => { - this.resolveByOperationId.set(++this.operationId, resolve); - this.renderer.send({...data, operationId: this.operationId}); - }); - } - createNewRenderer() { - if (this.renderer) { this.renderer.destroy(); } - this.renderer = new RendererProcess(this.webContentsId); + this.activeRenderer = new RendererProcess(this.hostWebContentsId, { + onDying: () => { this.createNewRenderer(); }, + onDestroyed: renderer => { this.renderers.delete(renderer); }, + }); + this.renderers.add(this.activeRenderer); } - getReadyPromise() { - return this.renderer && this.renderer.getReadyPromise(); + async request(data) { + await this.activeRenderer.getReadyPromise(); + return this.activeRenderer.send(data); } destroy() { - if (this.renderer) { this.renderer.destroy(); } - this.renderer = null; - ipc.removeListener('git-data', this.onGitDataReceived); + this.renderers.forEach(renderer => renderer.destroy()); } } class RendererProcess { - constructor(hostWebContentsId) { + constructor(hostWebContentsId, {onDying, onDestroyed}) { this.hostWebContentsId = hostWebContentsId; - ipc.on('renderer-ready', this.onRendererReady); + this.onDying = onDying; + this.onDestroyed = onDestroyed; + this.resolveByOperationId = new Map(); + this.operationId = 0; + this.dying = false; + this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.win.webContents.openDevTools(); this.childWebContentsId = this.win.webContents.id; + this.channelName = `message-${this.childWebContentsId}`; + + this.emitter = new Emitter(); + this.registerListeners(); + const htmlPath = path.join(getPackageRoot(), 'lib', 'renderer.html'); const rendererJsPath = path.join(getPackageRoot(), 'lib', 'renderer.js'); this.win.loadURL(`file://${htmlPath}?js=${rendererJsPath}&hostWebContentsId=${this.hostWebContentsId}`); + this.readyPromise = new Promise(resolve => { this.resolveReady = resolve; }); } + registerListeners() { + ipc.on(this.channelName, this.handleMessages); + this.emitter.on('renderer-ready', this.handleRendererReady); + this.emitter.on('git-data', this.handleGitDataReceived); + this.emitter.on('slow-spawns', this.handleDying); + } + + @autobind + handleMessages(event, {type, data}) { + this.emitter.emit(type, data); + } + @autobind - onRendererReady(event, {childWebContentsId}) { + handleRendererReady({childWebContentsId}) { if (childWebContentsId === this.childWebContentsId) { this.resolveReady(); } } + @autobind + handleGitDataReceived({operationId, results}) { + const resolve = this.resolveByOperationId.get(operationId); + resolve(results); + this.resolveByOperationId.delete(operationId); + + if (this.dying && this.resolveByOperationId.size === 0) { + this.destroy(); + this.onDestroyed(this); + } + } + + @autobind + handleDying() { + if (this.dying) { return; } + this.dying = true; + this.onDying(this.childWebContentsId); + } + send(data) { - this.win.webContents.send(`request-git-exec-${this.childWebContentsId}`, data); + return new Promise(resolve => { + this.resolveByOperationId.set(++this.operationId, resolve); + this.win.webContents.send(`message-${this.childWebContentsId}`, { + type: 'git-exec', + data: {...data, operationId: this.operationId}, + }); + }); } getReadyPromise() { @@ -90,8 +125,8 @@ class RendererProcess { } destroy() { - // after last response is received, shut down - ipc.removeListener('renderer-ready', this.onRendererReady); + ipc.removeListener(this.channelName, this.handleMessages); + this.emitter.dispose(); this.win.destroy(); } } diff --git a/lib/renderer.js b/lib/renderer.js index 84bb15a7e9..f6a458d0ea 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -31,18 +31,31 @@ const averageTracker = new AverageTracker({limit: 10}); const hostWebContentsId = parseInt(qs.parse(window.location.search.substr(1)).hostWebContentsId, 10); const childWebContentsId = remote.getCurrentWindow().webContents.id; +const channelName = `message-${childWebContentsId}`; -ipc.on(`request-git-exec-${childWebContentsId}`, (event, data) => { - const {args, workingDir, options, operationId} = data; - const execStart = performance.now(); - GitProcess.exec(args, workingDir, options) +ipc.on(channelName, (event, {type, data}) => { + if (type === 'git-exec') { + const {args, workingDir, options, operationId} = data; + const execStart = performance.now(); + GitProcess.exec(args, workingDir, options) .then(({stdout, stderr, exitCode}) => { - event.sender.sendTo(hostWebContentsId, 'git-data', {stdout, stderr, exitCode, operationId}); + event.sender.sendTo(hostWebContentsId, channelName, { + type: 'git-data', + data: { + operationId, + results: {stdout, stderr, exitCode}, + }, + }); }); - const execEnd = performance.now(); - averageTracker.addValue(execEnd - execStart); - // console.error(averageTracker.values); - // console.error(averageTracker.getAverage()); + const execEnd = performance.now(); + averageTracker.addValue(execEnd - execStart); + + if (averageTracker.getAverage() > 40) { + event.sender.sentTo(hostWebContentsId, channelName, {type: 'slow-spawns', data: {childWebContentsId}}); + } + } else { + throw new Error(`Could not identify type ${type}`); + } }); -ipc.sendTo(hostWebContentsId, 'renderer-ready', {childWebContentsId}); +ipc.sendTo(hostWebContentsId, channelName, {type: 'renderer-ready', data: {childWebContentsId}}); From bcd2722664b6382ce83a57f9499edbb41ca1daca Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 19 Apr 2017 18:45:46 -0700 Subject: [PATCH 32/78] WIP Implement logic for creating new renderer process ...accounting for slower systems --- lib/renderer-process-manager.js | 33 +++++++++++++++----------- lib/renderer.js | 41 +++++++++++++++++++++++++++++---- 2 files changed, 56 insertions(+), 18 deletions(-) diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js index 17aaaa6b59..68dd04950b 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -30,9 +30,13 @@ export default class RendererProcessManager { } @autobind - createNewRenderer() { - this.activeRenderer = new RendererProcess(this.hostWebContentsId, { - onDying: () => { this.createNewRenderer(); }, + createNewRenderer(previousOperationCount = 0, previousLimit = 10) { + let newLimit = 10; + if (previousOperationCount === previousLimit && Math.random() < 0.85) { + newLimit = Math.min(previousLimit * 2, 100); + } + this.activeRenderer = new RendererProcess(this.hostWebContentsId, newLimit, { + onDying: (operationCount, limit) => { this.createNewRenderer(operationCount, limit); }, onDestroyed: renderer => { this.renderers.delete(renderer); }, }); this.renderers.add(this.activeRenderer); @@ -49,7 +53,8 @@ export default class RendererProcessManager { } class RendererProcess { - constructor(hostWebContentsId, {onDying, onDestroyed}) { + constructor(hostWebContentsId, limit, {onDying, onDestroyed}) { + console.warn('create renderer'); this.hostWebContentsId = hostWebContentsId; this.onDying = onDying; this.onDestroyed = onDestroyed; @@ -57,7 +62,7 @@ class RendererProcess { this.operationId = 0; this.dying = false; - this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); + this.win = new BrowserWindow({show: true || !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.win.webContents.openDevTools(); this.childWebContentsId = this.win.webContents.id; this.channelName = `message-${this.childWebContentsId}`; @@ -67,7 +72,9 @@ class RendererProcess { const htmlPath = path.join(getPackageRoot(), 'lib', 'renderer.html'); const rendererJsPath = path.join(getPackageRoot(), 'lib', 'renderer.js'); - this.win.loadURL(`file://${htmlPath}?js=${rendererJsPath}&hostWebContentsId=${this.hostWebContentsId}`); + this.win.loadURL( + `file://${htmlPath}?js=${rendererJsPath}&hostWebContentsId=${this.hostWebContentsId}&limit=${limit}`, + ); this.readyPromise = new Promise(resolve => { this.resolveReady = resolve; }); } @@ -85,14 +92,13 @@ class RendererProcess { } @autobind - handleRendererReady({childWebContentsId}) { - if (childWebContentsId === this.childWebContentsId) { - this.resolveReady(); - } + handleRendererReady() { + this.resolveReady(); } @autobind - handleGitDataReceived({operationId, results}) { + handleGitDataReceived({operationId, results, average}) { + // console.log('average', average); const resolve = this.resolveByOperationId.get(operationId); resolve(results); this.resolveByOperationId.delete(operationId); @@ -104,10 +110,11 @@ class RendererProcess { } @autobind - handleDying() { + handleDying({operationCount, limit}) { + console.error('handleDying', limit); if (this.dying) { return; } this.dying = true; - this.onDying(this.childWebContentsId); + this.onDying(operationCount, limit); } send(data) { diff --git a/lib/renderer.js b/lib/renderer.js index f6a458d0ea..2ac8cdcb6f 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -5,14 +5,29 @@ const ipc = require('electron').ipcRenderer; const {GitProcess} = require('dugite'); +/* +dynamically change limit for AverageTracker +Limit starts at 10 +When creating new renderer: + If previousCommandCount == previousLimit + newLimit = Math.min(oldLimit * 1.5) + else + newLimit = 10 + +if average spawn time > 20 ms, create new renderer and send # of operations +*/ + class AverageTracker { constructor({limit} = {limit: 10}) { + console.log('new tracker, with limit', limit); this.limit = limit; + this.count = 0; this.sum = 0; this.values = []; } addValue(value) { + this.count++; if (this.values.length >= this.limit) { const discardedValue = this.values.shift(); this.sum -= discardedValue; @@ -25,16 +40,26 @@ class AverageTracker { if (this.values.length < this.limit) { return null; } return this.sum / this.limit; } -} -const averageTracker = new AverageTracker({limit: 10}); + getCount() { + return this.count; + } + + getLimit() { + return this.limit; + } +} const hostWebContentsId = parseInt(qs.parse(window.location.search.substr(1)).hostWebContentsId, 10); +const limit = parseInt(qs.parse(window.location.search.substr(1)).limit, 10); const childWebContentsId = remote.getCurrentWindow().webContents.id; const channelName = `message-${childWebContentsId}`; +const averageTracker = new AverageTracker({limit}); + ipc.on(channelName, (event, {type, data}) => { if (type === 'git-exec') { + console.log('.'); const {args, workingDir, options, operationId} = data; const execStart = performance.now(); GitProcess.exec(args, workingDir, options) @@ -43,6 +68,7 @@ ipc.on(channelName, (event, {type, data}) => { type: 'git-data', data: { operationId, + average: averageTracker.getAverage(), results: {stdout, stderr, exitCode}, }, }); @@ -50,12 +76,17 @@ ipc.on(channelName, (event, {type, data}) => { const execEnd = performance.now(); averageTracker.addValue(execEnd - execStart); - if (averageTracker.getAverage() > 40) { - event.sender.sentTo(hostWebContentsId, channelName, {type: 'slow-spawns', data: {childWebContentsId}}); + const average = averageTracker.getAverage(); + if (average && average > 20) { + console.log('ABOUT TO DIE'); + event.sender.sendTo(hostWebContentsId, channelName, {type: 'slow-spawns', data: { + operationCount: averageTracker.getCount(), + limit: averageTracker.getLimit(), + }}); } } else { throw new Error(`Could not identify type ${type}`); } }); -ipc.sendTo(hostWebContentsId, channelName, {type: 'renderer-ready', data: {childWebContentsId}}); +ipc.sendTo(hostWebContentsId, channelName, {type: 'renderer-ready'}); From 4557f461ac1fe9a6afa309e65a19dd4930f42b33 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 21 Apr 2017 15:58:50 -0700 Subject: [PATCH 33/78] Refactor RendererProcessManager -> WorkerManager. Handle destroyed processes --- lib/git-shell-out-strategy.js | 8 +- lib/renderer.js | 71 ++++----- lib/worker-manager.js | 261 ++++++++++++++++++++++++++++++++++ test/helpers.js | 4 +- 4 files changed, 295 insertions(+), 49 deletions(-) create mode 100644 lib/worker-manager.js diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index a7e2225235..0fd833b1f0 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -9,7 +9,7 @@ import GitPromptServer from './git-prompt-server'; import AsyncQueue from './async-queue'; import {getPackageRoot, getDugitePath, readFile, fileExists, writeFile, isFileExecutable} from './helpers'; import GitTimingsView from './views/git-timings-view'; -import RendererProcessManager from './renderer-process-manager'; +import WorkerManager from './worker-manager'; const LINE_ENDING_REGEX = /\r?\n/; @@ -57,7 +57,7 @@ export default class GitShellOutStrategy { } this.prompt = options.prompt || (query => Promise.reject()); - this.rendererProcessManager = options.rendererProcessManager; + this.workerManager = options.workerManager; } /* @@ -206,8 +206,8 @@ export default class GitShellOutStrategy { if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC) { return GitProcess.exec(args, this.workingDir, options); } else { - const rendererProcessManager = this.rendererProcessManager || RendererProcessManager.getInstance(); - return rendererProcessManager.request({ + const workerManager = this.workerManager || WorkerManager.getInstance(); + return workerManager.request({ args, workingDir: this.workingDir, options, diff --git a/lib/renderer.js b/lib/renderer.js index 2ac8cdcb6f..48862e916f 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -1,33 +1,18 @@ const qs = require('querystring'); -const remote = require('electron').remote; -const ipc = require('electron').ipcRenderer; - +const {remote, ipcRenderer: ipc} = require('electron'); const {GitProcess} = require('dugite'); -/* -dynamically change limit for AverageTracker -Limit starts at 10 -When creating new renderer: - If previousCommandCount == previousLimit - newLimit = Math.min(oldLimit * 1.5) - else - newLimit = 10 - -if average spawn time > 20 ms, create new renderer and send # of operations -*/ class AverageTracker { constructor({limit} = {limit: 10}) { - console.log('new tracker, with limit', limit); + // for now this serves a dual purpose - # of values tracked AND # discarded prior to starting avg calculation this.limit = limit; - this.count = 0; this.sum = 0; this.values = []; } addValue(value) { - this.count++; if (this.values.length >= this.limit) { const discardedValue = this.values.shift(); this.sum -= discardedValue; @@ -37,56 +22,56 @@ class AverageTracker { } getAverage() { - if (this.values.length < this.limit) { return null; } - return this.sum / this.limit; - } - - getCount() { - return this.count; + if (this.enoughData()) { + return this.sum / this.limit; + } else { + return null; + } } getLimit() { return this.limit; } + + enoughData() { + return this.values.length === this.limit; + } } -const hostWebContentsId = parseInt(qs.parse(window.location.search.substr(1)).hostWebContentsId, 10); -const limit = parseInt(qs.parse(window.location.search.substr(1)).limit, 10); -const childWebContentsId = remote.getCurrentWindow().webContents.id; -const channelName = `message-${childWebContentsId}`; +const operationCountLimit = parseInt(qs.parse(window.location.search.substr(1)).operationCountLimit, 10); +const averageTracker = new AverageTracker({limit: operationCountLimit}); -const averageTracker = new AverageTracker({limit}); +const destroyRenderer = () => { remote.BrowserWindow.fromWebContents(remote.getCurrentWebContents()).destroy(); }; +const managerWebContentsId = parseInt(qs.parse(window.location.search.substr(1)).managerWebContentsId, 10); +const managerWebContents = remote.webContents.fromId(managerWebContentsId); +managerWebContents.on('crashed', () => { destroyRenderer(); }); +managerWebContents.on('destroyed', () => { destroyRenderer(); }); +const channelName = `message-${remote.getCurrentWindow().webContents.id}`; ipc.on(channelName, (event, {type, data}) => { if (type === 'git-exec') { - console.log('.'); - const {args, workingDir, options, operationId} = data; - const execStart = performance.now(); + const {args, workingDir, options, id} = data; + const spawnStart = performance.now(); GitProcess.exec(args, workingDir, options) .then(({stdout, stderr, exitCode}) => { - event.sender.sendTo(hostWebContentsId, channelName, { + event.sender.sendTo(managerWebContentsId, channelName, { type: 'git-data', data: { - operationId, + id, average: averageTracker.getAverage(), results: {stdout, stderr, exitCode}, }, }); }); - const execEnd = performance.now(); - averageTracker.addValue(execEnd - execStart); + const spawnEnd = performance.now(); + averageTracker.addValue(spawnEnd - spawnStart); - const average = averageTracker.getAverage(); - if (average && average > 20) { - console.log('ABOUT TO DIE'); - event.sender.sendTo(hostWebContentsId, channelName, {type: 'slow-spawns', data: { - operationCount: averageTracker.getCount(), - limit: averageTracker.getLimit(), - }}); + 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(hostWebContentsId, channelName, {type: 'renderer-ready'}); +ipc.sendTo(managerWebContentsId, channelName, {type: 'renderer-ready'}); diff --git a/lib/worker-manager.js b/lib/worker-manager.js new file mode 100644 index 0000000000..35c5dc1209 --- /dev/null +++ b/lib/worker-manager.js @@ -0,0 +1,261 @@ +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(); + } + + destroy() { + // What if operations are currently in flight? + // TODO: await their finish? at least for write operations (optimization)? + this.workers.forEach(worker => worker.destroy()); + } + + request(data) { + return new Promise(resolve => { + const operation = new Operation(data, resolve); + return this.activeWorker.executeOperation(operation); + }); + } + + createNewWorker(operationCountLimit = 10) { + 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) { + this.createNewWorker(crashedWorker.getOperationCountLimit()); + crashedWorker.getRemainingOperations().forEach(operation => this.activeWorker.executeOperation(operation)); + } + + @autobind + onSick(sickWorker) { + 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; + } +} + + +class Worker { + 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, + onSick: this.handleSick, + onCrashed: this.handleCrashed, + onDestroyed: this.destroy, + }); + } + + getLoadUrl(operationCountLimit) { + const htmlPath = path.join(getPackageRoot(), 'lib', 'renderer.html'); + const rendererJsPath = path.join(getPackageRoot(), 'lib', 'renderer.js'); + const qs = querystring.stringify({ + js: rendererJsPath, + managerWebContentsId: remote.getCurrentWebContents().id, + operationCountLimit, + }); + return `file://${htmlPath}?${qs}`; + } + + destroy() { + this.onDestroyed(this); + this.rendererProcess.destroy(); + } + + executeOperation(operation) { + this.operationsById.set(operation.id, operation); + return this.rendererProcess.executeOperation(operation); + } + + @autobind + handleDataReceived({id, results}) { + const operation = this.operationsById.get(id); + operation.complete(results); + this.completedOperationCount++; + this.operationsById.delete(id); + + if (this.sick && this.operationsById.size === 0) { + this.destroy(); + } + } + + @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()); + } +} + + +/* +Sends operations to renderer processes +*/ +class RendererProcess { + constructor({loadUrl, onDestroyed, onCrashed, onSick, onData}) { + this.onDestroyed = onDestroyed; + this.onCrashed = onCrashed; + this.onSick = onSick; + this.onData = onData; + + this.win = new BrowserWindow({show: true || !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); + this.webContents = this.win.webContents; + this.webContents.openDevTools(); + this.channelName = `message-${this.webContents.id}`; + + this.emitter = new Emitter(); + this.subscriptions = new CompositeDisposable(); + this.subscriptions.add( + new Disposable(() => this.win.destroy()), + this.emitter, + ); + this.registerListeners(); + + this.win.loadURL(loadUrl); + // okay to move these to register listeners? + this.win.webContents.on('crashed', this.onCrashed); + // this.win.webContents.on('destroyed', this.onCrashed); + + this.readyPromise = new Promise(resolve => { this.resolveReady = resolve; }); + } + + @autobind + handleMessages(event, {type, data}) { + this.emitter.emit(type, data); + } + + registerListeners() { + const handleMessages = (event, {type, data}) => { + this.emitter.emit(type, data); + }; + + ipc.on(this.channelName, handleMessages); + this.emitter.on('renderer-ready', () => this.resolveReady()); + this.emitter.on('git-data', this.onData); + this.emitter.on('slow-spawns', this.onSick); + + this.subscriptions.add( + new Disposable(() => ipc.removeListener(this.channelName, handleMessages)), + ); + } + + async executeOperation(operation) { + // TODO: for now let's just queue up promises for execution. + // in the future we'll probably shell out in process while awaiting the renderer to be ready + await this.readyPromise; // Is this bad for always putting things on the next tick? + return operation.execute(payload => { + return this.webContents.send(this.channelName, { + type: 'git-exec', + data: payload, + }); + }); + } + + destroy() { + this.subscriptions.dispose(); + } +} + + +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.status = Operation.status.PENDING; + this.results = null; + } + + setInProgress() { + // after exec has been called but before results a received + this.status = Operation.status.INPROGRESS; + } + + complete(results) { + this.results = results; + this.resolve(results); + this.status = Operation.status.COMPLETE; + } + + execute(execFn) { + return execFn({...this.data, id: this.id}); + } +} diff --git a/test/helpers.js b/test/helpers.js index fd56808821..c4774279fe 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -8,7 +8,7 @@ import sinon from 'sinon'; import Repository from '../lib/models/repository'; import GitShellOutStrategy from '../lib/git-shell-out-strategy'; -import RendererProcessManager from '../lib/renderer-process-manager'; +import WorkerManager from '../lib/worker-manager'; assert.autocrlfEqual = (actual, expected, ...args) => { const newActual = actual.replace(/\r\n/g, '\n'); @@ -172,6 +172,6 @@ afterEach(function() { // eslint-disable-next-line jasmine/no-global-setup after(() => { if (!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW) { - RendererProcessManager.reset(); + WorkerManager.reset(); } }); From 097714dcf15ab6dc4403ad9376a82e28aac4fa89 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 21 Apr 2017 18:03:44 -0700 Subject: [PATCH 34/78] Set operation to pending after call to GitProcess.exec --- lib/renderer.js | 1 + lib/worker-manager.js | 11 ++++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/lib/renderer.js b/lib/renderer.js index 48862e916f..2e7be70b5c 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -65,6 +65,7 @@ ipc.on(channelName, (event, {type, data}) => { }); const spawnEnd = performance.now(); averageTracker.addValue(spawnEnd - spawnStart); + event.sender.sendTo(managerWebContentsId, channelName, {type: 'exec-started', data: {id}}); if (averageTracker.enoughData() && averageTracker.getAverage() > 20) { event.sender.sendTo(managerWebContentsId, channelName, {type: 'slow-spawns'}); diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 35c5dc1209..6abb90085a 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -93,6 +93,7 @@ class Worker { this.rendererProcess = new RendererProcess({ loadUrl: this.getLoadUrl(operationCountLimit), onData: this.handleDataReceived, + onExecStarted: this.handleExecStarted, onSick: this.handleSick, onCrashed: this.handleCrashed, onDestroyed: this.destroy, @@ -132,6 +133,12 @@ class Worker { } } + @autobind + handleExecStarted({id}) { + const operation = this.operationsById.get(id); + operation.setInProgress(); + } + @autobind handleSick() { this.sick = true; @@ -162,11 +169,12 @@ class Worker { Sends operations to renderer processes */ class RendererProcess { - constructor({loadUrl, onDestroyed, onCrashed, onSick, onData}) { + 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: true || !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.webContents = this.win.webContents; @@ -202,6 +210,7 @@ class RendererProcess { ipc.on(this.channelName, handleMessages); this.emitter.on('renderer-ready', () => this.resolveReady()); this.emitter.on('git-data', this.onData); + this.emitter.on('exec-started', this.onExecStarted); this.emitter.on('slow-spawns', this.onSick); this.subscriptions.add( From ce2bcc300111f67825f6fcc8e314035fb1272554 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Fri, 21 Apr 2017 18:05:43 -0700 Subject: [PATCH 35/78] Wait for all write operations to finish before destroying renderer process --- lib/git-shell-out-strategy.js | 5 +++-- lib/worker-manager.js | 30 ++++++++++++++++++++++++++---- 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 0fd833b1f0..901adc03c8 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -163,7 +163,7 @@ export default class GitShellOutStrategy { timingMarker.mark('nexttick'); timingMarker.mark('execute'); - const {stdout, stderr, exitCode} = await this.executeGitCommand(args, options); + const {stdout, stderr, exitCode} = await this.executeGitCommand(args, options, writeOperation); timingMarker.finalize(); if (process.env.PRINT_GIT_TIMES) { console.timeEnd(`git:${formattedArgs}`); @@ -201,7 +201,7 @@ export default class GitShellOutStrategy { /* eslint-enable no-console */ } - executeGitCommand(args, options) { + executeGitCommand(args, options, writeOperation) { const timeSent = performance.now(); // TODO: move to manager if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC) { return GitProcess.exec(args, this.workingDir, options); @@ -211,6 +211,7 @@ export default class GitShellOutStrategy { args, workingDir: this.workingDir, options, + writeOperation, timeSent, }); } diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 6abb90085a..18f9801093 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -36,10 +36,13 @@ export default class WorkerManager { } request(data) { - return new Promise(resolve => { - const operation = new Operation(data, resolve); + let operation; + const requestPromise = new Promise(resolve => { + operation = new Operation(data, resolve); return this.activeWorker.executeOperation(operation); }); + operation.setPromise(requestPromise); + return requestPromise; } createNewWorker(operationCountLimit = 10) { @@ -79,7 +82,7 @@ export default class WorkerManager { } -class Worker { +export class Worker { constructor({operationCountLimit, onSick, onCrashed, onDestroyed}) { this.operationCountLimit = operationCountLimit; this.onSick = onSick; @@ -111,8 +114,14 @@ class Worker { return `file://${htmlPath}?${qs}`; } - destroy() { + async destroy() { this.onDestroyed(this); + if (this.operationsById.size > 0) { + const remainingOperationPromises = this.getRemainingOperations() + .filter(operation => operation.isWrite()) + .map(operation => operation.getPromise()); + await Promise.all(remainingOperationPromises); + } this.rendererProcess.destroy(); } @@ -249,15 +258,28 @@ class Operation { this.id = Operation.id++; this.data = data; this.resolve = resolve; + this.promise = null; this.status = Operation.status.PENDING; this.results = null; } + 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; } + isWrite() { + return this.data.writeOperation; + } + complete(results) { this.results = results; this.resolve(results); From 8e211a1b65fb2feb503d727d0fb5662804ab0a6c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 15:07:30 -0700 Subject: [PATCH 36/78] Add test for fallback when worker process crashes --- lib/renderer.js | 2 +- lib/worker-manager.js | 45 ++++++++++++++++++++++++++++--------- test/worker-manager.test.js | 31 +++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 12 deletions(-) create mode 100644 test/worker-manager.test.js diff --git a/lib/renderer.js b/lib/renderer.js index 2e7be70b5c..5a09bf943b 100644 --- a/lib/renderer.js +++ b/lib/renderer.js @@ -75,4 +75,4 @@ ipc.on(channelName, (event, {type, data}) => { } }); -ipc.sendTo(managerWebContentsId, channelName, {type: 'renderer-ready'}); +ipc.sendTo(managerWebContentsId, channelName, {type: 'renderer-ready', data: {pid: process.pid}}); diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 18f9801093..2faf6c21d9 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -45,7 +45,7 @@ export default class WorkerManager { return requestPromise; } - createNewWorker(operationCountLimit = 10) { + createNewWorker({operationCountLimit} = {operationCountLimit: 10}) { this.activeWorker = new Worker({ operationCountLimit, onDestroyed: this.onDestroyed, @@ -62,14 +62,14 @@ export default class WorkerManager { @autobind onCrashed(crashedWorker) { - this.createNewWorker(crashedWorker.getOperationCountLimit()); + this.createNewWorker({operationCountLimit: crashedWorker.getOperationCountLimit()}); crashedWorker.getRemainingOperations().forEach(operation => this.activeWorker.executeOperation(operation)); } @autobind onSick(sickWorker) { const operationCountLimit = this.calculateNewOperationCountLimit(sickWorker); - return this.createNewWorker(operationCountLimit); + return this.createNewWorker({operationCountLimit}); } calculateNewOperationCountLimit(lastWorker) { @@ -79,6 +79,10 @@ export default class WorkerManager { } return operationCountLimit; } + + getActiveWorker() { + return this.activeWorker; + } } @@ -118,7 +122,7 @@ export class Worker { this.onDestroyed(this); if (this.operationsById.size > 0) { const remainingOperationPromises = this.getRemainingOperations() - .filter(operation => operation.isWrite()) + // .filter(operation => operation.isWrite()) .map(operation => operation.getPromise()); await Promise.all(remainingOperationPromises); } @@ -171,13 +175,21 @@ export class Worker { getRemainingOperations() { return Array.from(this.operationsById.values()); } + + getPid() { + return this.rendererProcess.getPid(); + } + + getReadyPromise() { + return this.rendererProcess.getReadyPromise(); + } } /* Sends operations to renderer processes */ -class RendererProcess { +export class RendererProcess { constructor({loadUrl, onDestroyed, onCrashed, onSick, onData, onExecStarted}) { this.onDestroyed = onDestroyed; this.onCrashed = onCrashed; @@ -217,7 +229,10 @@ class RendererProcess { }; ipc.on(this.channelName, handleMessages); - this.emitter.on('renderer-ready', () => this.resolveReady()); + this.emitter.on('renderer-ready', ({pid}) => { + this.pid = pid; + this.resolveReady(); + }); this.emitter.on('git-data', this.onData); this.emitter.on('exec-started', this.onExecStarted); this.emitter.on('slow-spawns', this.onSick); @@ -239,13 +254,21 @@ class RendererProcess { }); } + getPid() { + return this.pid; + } + + getReadyPromise() { + return this.readyPromise; + } + destroy() { this.subscriptions.dispose(); } } -class Operation { +export class Operation { static status = { PENDING: Symbol('pending'), INPROGRESS: Symbol('in-progress'), @@ -275,10 +298,10 @@ class Operation { // after exec has been called but before results a received this.status = Operation.status.INPROGRESS; } - - isWrite() { - return this.data.writeOperation; - } + // + // isWrite() { + // return this.data.writeOperation; + // } complete(results) { this.results = results; diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js new file mode 100644 index 0000000000..65be03a803 --- /dev/null +++ b/test/worker-manager.test.js @@ -0,0 +1,31 @@ +import sinon from 'sinon'; + +import WorkerManager, {Operation} from '../lib/worker-manager'; + +describe('WorkerManager', function() { + 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() { + const workerManager = WorkerManager.getInstance(); + workerManager.createNewWorker({operationCountLimit: 40}); + sinon.stub(Operation.prototype, 'complete'); + + const worker1 = workerManager.getActiveWorker(); + await worker1.getReadyPromise(); + workerManager.request(); + workerManager.request(); + workerManager.request(); + const worker1OperationsInFlight = worker1.getRemainingOperations(); + assert.equal(worker1OperationsInFlight.length, 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); + }); + }); +}); From e43c2710f67eff51940c4c968d78256c7b695222 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 18:06:54 -0700 Subject: [PATCH 37/78] Destroy RendererProcess instance on 'crashed' event --- lib/worker-manager.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 2faf6c21d9..2cf0baa1fd 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -212,7 +212,10 @@ export class RendererProcess { this.win.loadURL(loadUrl); // okay to move these to register listeners? - this.win.webContents.on('crashed', this.onCrashed); + this.win.webContents.on('crashed', (...args) => { + this.destroy(); + this.onCrashed(...args); + }); // this.win.webContents.on('destroyed', this.onCrashed); this.readyPromise = new Promise(resolve => { this.resolveReady = resolve; }); From 6505e0420a966eafa19e52716fb34cb0c441de6f Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 18:08:12 -0700 Subject: [PATCH 38/78] Add test for handling when a worker is sick --- test/worker-manager.test.js | 38 +++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index 65be03a803..cd4efc6d91 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -28,4 +28,42 @@ describe('WorkerManager', function() { 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() { + const workerManager = WorkerManager.getInstance(); + + 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); + }); + }); }); From 23026e2c25dcb078ef86c8c404e493cd8a7aa13c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 18:15:05 -0700 Subject: [PATCH 39/78] Execute remaining operations in newly active worker when sick process crashes --- lib/worker-manager.js | 4 +++- test/worker-manager.test.js | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 2cf0baa1fd..c1ba6ac017 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -62,7 +62,9 @@ export default class WorkerManager { @autobind onCrashed(crashedWorker) { - this.createNewWorker({operationCountLimit: crashedWorker.getOperationCountLimit()}); + if (crashedWorker === this.getActiveWorker()) { + this.createNewWorker({operationCountLimit: crashedWorker.getOperationCountLimit()}); + } crashedWorker.getRemainingOperations().forEach(operation => this.activeWorker.executeOperation(operation)); } diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index cd4efc6d91..9ac302b4dd 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -65,5 +65,29 @@ describe('WorkerManager', function() { 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 workerManager = WorkerManager.getInstance(); + const sickWorker = workerManager.getActiveWorker(); + + sinon.stub(Operation.prototype, 'complete'); + 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); + }); + }); }); }); From 7b29cc70e8d461ff6a98b7b9a321e903f7998f63 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 18:32:33 -0700 Subject: [PATCH 40/78] Use global sinon --- test/worker-manager.test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index 9ac302b4dd..fd5b01a395 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -1,5 +1,3 @@ -import sinon from 'sinon'; - import WorkerManager, {Operation} from '../lib/worker-manager'; describe('WorkerManager', function() { From 045c009f8ea8eeec4228d2dad530320e39209262 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 18:46:51 -0700 Subject: [PATCH 41/78] Destroy worker managers in tests --- lib/worker-manager.js | 8 ++++---- test/worker-manager.test.js | 12 +++++++++--- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index c1ba6ac017..2cb8f048b7 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -29,10 +29,10 @@ export default class WorkerManager { this.createNewWorker(); } - destroy() { + destroy(force) { // What if operations are currently in flight? // TODO: await their finish? at least for write operations (optimization)? - this.workers.forEach(worker => worker.destroy()); + this.workers.forEach(worker => worker.destroy(force)); } request(data) { @@ -120,9 +120,9 @@ export class Worker { return `file://${htmlPath}?${qs}`; } - async destroy() { + async destroy(force) { this.onDestroyed(this); - if (this.operationsById.size > 0) { + if (this.operationsById.size > 0 && !force) { const remainingOperationPromises = this.getRemainingOperations() // .filter(operation => operation.isWrite()) .map(operation => operation.getPromise()); diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index fd5b01a395..55713c7cc6 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -1,9 +1,17 @@ import WorkerManager, {Operation} from '../lib/worker-manager'; describe('WorkerManager', function() { + let workerManager; + beforeEach(() => { + workerManager = new WorkerManager(); + }); + + afterEach(() => { + workerManager.destroy(true); + }); + 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() { - const workerManager = WorkerManager.getInstance(); workerManager.createNewWorker({operationCountLimit: 40}); sinon.stub(Operation.prototype, 'complete'); @@ -29,7 +37,6 @@ describe('WorkerManager', function() { 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() { - const workerManager = WorkerManager.getInstance(); function createSickWorker(operationCountLimit, completedOperationCount) { const sickWorker = workerManager.getActiveWorker(); @@ -66,7 +73,6 @@ describe('WorkerManager', function() { describe('when the sick process crashes', function() { it('completes remaining operations in existing active process', function() { - const workerManager = WorkerManager.getInstance(); const sickWorker = workerManager.getActiveWorker(); sinon.stub(Operation.prototype, 'complete'); From 103816102489b287a9645cca11114fb83e3899bd Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 18:59:53 -0700 Subject: [PATCH 42/78] Don't care if operation is a write --- lib/git-shell-out-strategy.js | 5 ++--- lib/worker-manager.js | 5 ----- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 901adc03c8..0fd833b1f0 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -163,7 +163,7 @@ export default class GitShellOutStrategy { timingMarker.mark('nexttick'); timingMarker.mark('execute'); - const {stdout, stderr, exitCode} = await this.executeGitCommand(args, options, writeOperation); + const {stdout, stderr, exitCode} = await this.executeGitCommand(args, options); timingMarker.finalize(); if (process.env.PRINT_GIT_TIMES) { console.timeEnd(`git:${formattedArgs}`); @@ -201,7 +201,7 @@ export default class GitShellOutStrategy { /* eslint-enable no-console */ } - executeGitCommand(args, options, writeOperation) { + executeGitCommand(args, options) { const timeSent = performance.now(); // TODO: move to manager if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC) { return GitProcess.exec(args, this.workingDir, options); @@ -211,7 +211,6 @@ export default class GitShellOutStrategy { args, workingDir: this.workingDir, options, - writeOperation, timeSent, }); } diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 2cb8f048b7..574d9521c2 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -124,7 +124,6 @@ export class Worker { this.onDestroyed(this); if (this.operationsById.size > 0 && !force) { const remainingOperationPromises = this.getRemainingOperations() - // .filter(operation => operation.isWrite()) .map(operation => operation.getPromise()); await Promise.all(remainingOperationPromises); } @@ -303,10 +302,6 @@ export class Operation { // after exec has been called but before results a received this.status = Operation.status.INPROGRESS; } - // - // isWrite() { - // return this.data.writeOperation; - // } complete(results) { this.results = results; From 29c36bb2b99f71dc9cb380b5b9601c80bb6040bc Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 19:00:50 -0700 Subject: [PATCH 43/78] Add test for ensuring that operations are complete before destroying worker --- test/worker-manager.test.js | 39 +++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index 55713c7cc6..e871f97e9c 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -94,4 +94,43 @@ describe('WorkerManager', function() { }); }); }); + + describe('destroy', function() { + it('destroys the renderer processes created after they have completed their operations', async function() { + function stillAlive(pid) { + // are you still there? + let alive = true; + try { + return process.kill(pid, 0); + } catch (e) { + alive = false; + } + return alive; + } + + const worker1 = workerManager.getActiveWorker(); + + sinon.stub(Operation.prototype, 'complete'); + workerManager.request(); + workerManager.request(); + workerManager.request(); + const worker1Operations = worker1.getRemainingOperations(); + assert.equal(worker1Operations.length, 3); + + workerManager.onSick(worker1); + const worker2 = workerManager.getActiveWorker(); + workerManager.request(); + workerManager.request(); + const worker2Operations = worker2.getRemainingOperations(); + assert.equal(worker2Operations.length, 2); + + workerManager.destroy(); + assert.isFalse(stillAlive(worker1.getPid())); + assert.isFalse(stillAlive(worker2.getPid())); + + [...worker1Operations, ...worker2Operations].forEach(operation => operation.complete()); + await assert.async.isTrue(stillAlive(worker1.getPid())); + await assert.async.isTrue(stillAlive(worker2.getPid())); + }); + }); }); From cd71d68877c020217dc88510060eccad46e4269d Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 19:02:43 -0700 Subject: [PATCH 44/78] Rename renderer.js to worker.js --- lib/worker-manager.js | 2 +- lib/{renderer.js => worker.js} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename lib/{renderer.js => worker.js} (100%) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 574d9521c2..1ff4f928c1 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -111,7 +111,7 @@ export class Worker { getLoadUrl(operationCountLimit) { const htmlPath = path.join(getPackageRoot(), 'lib', 'renderer.html'); - const rendererJsPath = path.join(getPackageRoot(), 'lib', 'renderer.js'); + const rendererJsPath = path.join(getPackageRoot(), 'lib', 'worker.js'); const qs = querystring.stringify({ js: rendererJsPath, managerWebContentsId: remote.getCurrentWebContents().id, diff --git a/lib/renderer.js b/lib/worker.js similarity index 100% rename from lib/renderer.js rename to lib/worker.js From 77fdc0d790ec88d0706eb52dcf00efece9efacc8 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 20:50:08 -0700 Subject: [PATCH 45/78] Add test case for killing renderer processes when worker manager crashes --- lib/worker-manager.js | 6 ++++- test/helpers.js | 10 ++++++++ test/worker-manager.test.js | 48 ++++++++++++++++++++++++------------- 3 files changed, 47 insertions(+), 17 deletions(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 1ff4f928c1..f2b4949d90 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -114,12 +114,16 @@ export class Worker { const rendererJsPath = path.join(getPackageRoot(), 'lib', 'worker.js'); const qs = querystring.stringify({ js: rendererJsPath, - managerWebContentsId: remote.getCurrentWebContents().id, + managerWebContentsId: this.getWebContentsId(), operationCountLimit, }); return `file://${htmlPath}?${qs}`; } + getWebContentsId() { + return remote.getCurrentWebContents().id; + } + async destroy(force) { this.onDestroyed(this); if (this.operationsById.size > 0 && !force) { diff --git a/test/helpers.js b/test/helpers.js index c4774279fe..533e4eeaed 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -157,6 +157,16 @@ export function createRenderer() { return renderer; } +export function isProcessAlive(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(); diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index e871f97e9c..ead63ba741 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -1,4 +1,8 @@ -import WorkerManager, {Operation} from '../lib/worker-manager'; +import {remote} from 'electron'; +const {BrowserWindow} = remote; + +import WorkerManager, {Operation, Worker} from '../lib/worker-manager'; +import {isProcessAlive} from './helpers'; describe('WorkerManager', function() { let workerManager; @@ -97,17 +101,6 @@ describe('WorkerManager', function() { describe('destroy', function() { it('destroys the renderer processes created after they have completed their operations', async function() { - function stillAlive(pid) { - // are you still there? - let alive = true; - try { - return process.kill(pid, 0); - } catch (e) { - alive = false; - } - return alive; - } - const worker1 = workerManager.getActiveWorker(); sinon.stub(Operation.prototype, 'complete'); @@ -125,12 +118,35 @@ describe('WorkerManager', function() { assert.equal(worker2Operations.length, 2); workerManager.destroy(); - assert.isFalse(stillAlive(worker1.getPid())); - assert.isFalse(stillAlive(worker2.getPid())); + assert.isFalse(isProcessAlive(worker1.getPid())); + assert.isFalse(isProcessAlive(worker2.getPid())); [...worker1Operations, ...worker2Operations].forEach(operation => operation.complete()); - await assert.async.isTrue(stillAlive(worker1.getPid())); - await assert.async.isTrue(stillAlive(worker2.getPid())); + await assert.async.isTrue(isProcessAlive(worker1.getPid())); + await assert.async.isTrue(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: true}); + browserWindow.loadURL('about:blank'); + sinon.stub(Worker.prototype, 'getWebContentsId').returns(browserWindow.webContents.id); + + workerManager = new WorkerManager(); + + const managerPid = await new Promise(resolve => { + browserWindow.webContents.executeJavaScript('process.pid', resolve); + }); + + const worker1 = workerManager.getActiveWorker(); + workerManager.onSick(worker1); + const worker2 = workerManager.getActiveWorker(); + + process.kill(managerPid, 'SIGKILL'); + await assert.async.isFalse(isProcessAlive(managerPid)); + await assert.async.isFalse(isProcessAlive(worker1.getPid())); + await assert.async.isFalse(isProcessAlive(worker2.getPid())); }); }); }); From 0bb8e5733bd50ce844ab3b5fa62393b620d1b26c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 21:00:54 -0700 Subject: [PATCH 46/78] Show browser windows if process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW is true --- lib/worker-manager.js | 2 +- test/worker-manager.test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index f2b4949d90..b4f0a2e261 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -202,7 +202,7 @@ export class RendererProcess { this.onData = onData; this.onExecStarted = onExecStarted; - this.win = new BrowserWindow({show: true || !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); + this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.webContents = this.win.webContents; this.webContents.openDevTools(); this.channelName = `message-${this.webContents.id}`; diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index ead63ba741..f355fda675 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -129,7 +129,7 @@ describe('WorkerManager', function() { describe('when the manager process is destroyed', function() { it('destroys all the renderer processes that were created', async function() { - const browserWindow = new BrowserWindow({show: true}); + 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); From d11877e41113a8dca3ffdbf0f4f6d15f8d6f4f2c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 21:15:01 -0700 Subject: [PATCH 47/78] Don't create new worker if WorkerManager is destroyed --- lib/worker-manager.js | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index b4f0a2e261..a8adcd8450 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -30,12 +30,12 @@ export default class WorkerManager { } destroy(force) { - // What if operations are currently in flight? - // TODO: await their finish? at least for write operations (optimization)? + this.destroyed = true; this.workers.forEach(worker => worker.destroy(force)); } request(data) { + if (this.destroyed) { throw new Error('Worker is destroyed'); } let operation; const requestPromise = new Promise(resolve => { operation = new Operation(data, resolve); @@ -46,6 +46,7 @@ export default class WorkerManager { } createNewWorker({operationCountLimit} = {operationCountLimit: 10}) { + if (this.destroyed) { return; } this.activeWorker = new Worker({ operationCountLimit, onDestroyed: this.onDestroyed, @@ -217,11 +218,8 @@ export class RendererProcess { this.win.loadURL(loadUrl); // okay to move these to register listeners? - this.win.webContents.on('crashed', (...args) => { - this.destroy(); - this.onCrashed(...args); - }); - // this.win.webContents.on('destroyed', this.onCrashed); + this.win.webContents.on('crashed', this.handleDestroy); + this.win.webContents.on('destroyed', this.handleDestroy); this.readyPromise = new Promise(resolve => { this.resolveReady = resolve; }); } @@ -231,6 +229,12 @@ export class RendererProcess { this.emitter.emit(type, data); } + @autobind + handleDestroy(...args) { + this.destroy(); + this.onCrashed(...args); + } + registerListeners() { const handleMessages = (event, {type, data}) => { this.emitter.emit(type, data); @@ -255,6 +259,7 @@ export class RendererProcess { // in the future we'll probably shell out in process while awaiting the renderer to be ready await this.readyPromise; // Is this bad for always putting things on the next tick? return operation.execute(payload => { + if (this.destroyed) { return null; } return this.webContents.send(this.channelName, { type: 'git-exec', data: payload, @@ -271,6 +276,7 @@ export class RendererProcess { } destroy() { + this.destroyed = true; this.subscriptions.dispose(); } } From 80956e2b1222022e2369e7ad26a56e85d7b0af4e Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 21:17:02 -0700 Subject: [PATCH 48/78] Don't open dev tools --- lib/worker-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index a8adcd8450..c6f932c1dd 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -205,7 +205,7 @@ export class RendererProcess { this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.webContents = this.win.webContents; - this.webContents.openDevTools(); + // this.webContents.openDevTools(); this.channelName = `message-${this.webContents.id}`; this.emitter = new Emitter(); From 94e0b8bc77974bc79bd5be5366849be2236b4c42 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 21:18:22 -0700 Subject: [PATCH 49/78] :art: move destroy method to bottom --- lib/worker-manager.js | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index c6f932c1dd..39c2f24176 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -29,11 +29,6 @@ export default class WorkerManager { this.createNewWorker(); } - destroy(force) { - this.destroyed = true; - this.workers.forEach(worker => worker.destroy(force)); - } - request(data) { if (this.destroyed) { throw new Error('Worker is destroyed'); } let operation; @@ -86,6 +81,11 @@ export default class WorkerManager { getActiveWorker() { return this.activeWorker; } + + destroy(force) { + this.destroyed = true; + this.workers.forEach(worker => worker.destroy(force)); + } } @@ -125,16 +125,6 @@ export class Worker { return remote.getCurrentWebContents().id; } - async destroy(force) { - this.onDestroyed(this); - if (this.operationsById.size > 0 && !force) { - const remainingOperationPromises = this.getRemainingOperations() - .map(operation => operation.getPromise()); - await Promise.all(remainingOperationPromises); - } - this.rendererProcess.destroy(); - } - executeOperation(operation) { this.operationsById.set(operation.id, operation); return this.rendererProcess.executeOperation(operation); @@ -189,6 +179,16 @@ export class Worker { 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()); + await Promise.all(remainingOperationPromises); + } + this.rendererProcess.destroy(); + } } From 53bf504ec1798b52c519fa0289a6ed4d92db9ee5 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Mon, 24 Apr 2017 21:26:04 -0700 Subject: [PATCH 50/78] :art: --- lib/controllers/git-tab-controller.js | 2 +- lib/renderer-process-manager.js | 5 +---- lib/worker-manager.js | 2 +- test/git-strategies.test.js | 1 - 4 files changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/controllers/git-tab-controller.js b/lib/controllers/git-tab-controller.js index 96e30e5a28..80e711b6b5 100644 --- a/lib/controllers/git-tab-controller.js +++ b/lib/controllers/git-tab-controller.js @@ -208,7 +208,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/renderer-process-manager.js b/lib/renderer-process-manager.js index 68dd04950b..995d93b727 100644 --- a/lib/renderer-process-manager.js +++ b/lib/renderer-process-manager.js @@ -54,7 +54,6 @@ export default class RendererProcessManager { class RendererProcess { constructor(hostWebContentsId, limit, {onDying, onDestroyed}) { - console.warn('create renderer'); this.hostWebContentsId = hostWebContentsId; this.onDying = onDying; this.onDestroyed = onDestroyed; @@ -62,7 +61,7 @@ class RendererProcess { this.operationId = 0; this.dying = false; - this.win = new BrowserWindow({show: true || !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); + this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.win.webContents.openDevTools(); this.childWebContentsId = this.win.webContents.id; this.channelName = `message-${this.childWebContentsId}`; @@ -98,7 +97,6 @@ class RendererProcess { @autobind handleGitDataReceived({operationId, results, average}) { - // console.log('average', average); const resolve = this.resolveByOperationId.get(operationId); resolve(results); this.resolveByOperationId.delete(operationId); @@ -111,7 +109,6 @@ class RendererProcess { @autobind handleDying({operationCount, limit}) { - console.error('handleDying', limit); if (this.dying) { return; } this.dying = true; this.onDying(operationCount, limit); diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 39c2f24176..86a241faa7 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -203,7 +203,7 @@ export class RendererProcess { this.onData = onData; this.onExecStarted = onExecStarted; - this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); + this.win = new BrowserWindow({show: true || !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.webContents = this.win.webContents; // this.webContents.openDevTools(); this.channelName = `message-${this.webContents.id}`; diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index 88cc6bdc50..a90a2145c3 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -6,7 +6,6 @@ import dedent from 'dedent-js'; import CompositeGitStrategy from '../lib/composite-git-strategy'; import GitShellOutStrategy from '../lib/git-shell-out-strategy'; -import {GitProcess} from 'dugite'; import {cloneRepository, initRepository, assertDeepPropertyVals, setUpLocalAndRemoteRepositories} from './helpers'; import {fsStat} from '../lib/helpers'; From 920b4ca053207dce5b0963a661213176135c7337 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Tue, 25 Apr 2017 11:03:44 -0700 Subject: [PATCH 51/78] :fire: .only --- test/github-package.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/github-package.test.js b/test/github-package.test.js index 651b4c5168..8c70ba1a28 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -7,7 +7,7 @@ import {cloneRepository} from './helpers'; import {writeFile, getTempDir} from '../lib/helpers'; import GithubPackage from '../lib/github-package'; -describe.only('GithubPackage', function() { +describe('GithubPackage', function() { let atomEnv, workspace, project, commandRegistry, notificationManager, config, confirm, tooltips, styles; let getLoadSettings; let githubPackage, contextPool; From 91908e5f70b2bb75a938dbba1c2415f35d9770d3 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Tue, 25 Apr 2017 11:11:46 -0700 Subject: [PATCH 52/78] :fire: error when pushing into a disposed AsyncQueue --- lib/async-queue.js | 3 --- 1 file changed, 3 deletions(-) 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(); From d8680916f778134f941034d646c82d0a86dd3067 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Tue, 25 Apr 2017 11:41:22 -0700 Subject: [PATCH 53/78] Fix failure to clone when target folder doesn't exist --- lib/git-shell-out-strategy.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index d6c89bdc65..18b37d178e 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -7,7 +7,7 @@ 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'; @@ -234,6 +234,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) { From c991157f590efd7caeb43fe8a6c2c5b31eb1810e Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Tue, 25 Apr 2017 12:41:42 -0700 Subject: [PATCH 54/78] Let the worker window tell us what it's doing --- lib/renderer.html | 3 ++- lib/worker.js | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/renderer.html b/lib/renderer.html index b30b1e1843..77fb7d41f7 100644 --- a/lib/renderer.html +++ b/lib/renderer.html @@ -12,7 +12,8 @@

GitHub Package Git Execution Window

- Hi there! This renderer window is used by the GitHub pacakge to execute Git commands in the background. + Hi there! I'm a window used by the GitHub pacakge to execute Git commands in the background. My PID is .

+

Last command:

diff --git a/lib/worker.js b/lib/worker.js index 5a09bf943b..3fbb0f0ba9 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -51,6 +51,9 @@ const channelName = `message-${remote.getCurrentWindow().webContents.id}`; 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}) => { From 5743cb3d41ead16346294bc00d5f9138672fa277 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Tue, 25 Apr 2017 12:42:21 -0700 Subject: [PATCH 55/78] Destroy the old WorkerManager before creating a new one. --- test/worker-manager.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index f355fda675..fdad904906 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -133,6 +133,7 @@ describe('WorkerManager', function() { browserWindow.loadURL('about:blank'); sinon.stub(Worker.prototype, 'getWebContentsId').returns(browserWindow.webContents.id); + workerManager.destroy(true); workerManager = new WorkerManager(); const managerPid = await new Promise(resolve => { From dca043e369aa30c9b4cc7c2c9688cd3e27e69d14 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 15:40:33 -0700 Subject: [PATCH 56/78] Stub Operation.prototype.execute instead of complete --- test/worker-manager.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index f355fda675..a7481b96cf 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -17,7 +17,7 @@ describe('WorkerManager', function() { 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, 'complete'); + sinon.stub(Operation.prototype, 'execute'); const worker1 = workerManager.getActiveWorker(); await worker1.getReadyPromise(); @@ -79,7 +79,7 @@ describe('WorkerManager', function() { it('completes remaining operations in existing active process', function() { const sickWorker = workerManager.getActiveWorker(); - sinon.stub(Operation.prototype, 'complete'); + sinon.stub(Operation.prototype, 'execute'); workerManager.request(); workerManager.request(); workerManager.request(); @@ -103,7 +103,7 @@ describe('WorkerManager', function() { it('destroys the renderer processes created after they have completed their operations', async function() { const worker1 = workerManager.getActiveWorker(); - sinon.stub(Operation.prototype, 'complete'); + sinon.stub(Operation.prototype, 'execute'); workerManager.request(); workerManager.request(); workerManager.request(); From fc16dd5a7018d17a6da0fbdc036db583a384295f Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 15:41:11 -0700 Subject: [PATCH 57/78] Throw error in isProcessAlive helper if pid is not a number --- test/helpers.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/helpers.js b/test/helpers.js index 533e4eeaed..297aaa738d 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -158,6 +158,9 @@ export function createRenderer() { } 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); From a0df97c8929e4a1a7ac775106b427309bf49c488 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 15:45:14 -0700 Subject: [PATCH 58/78] Decouple completing operation and post-completion cleanup tasks --- lib/worker-manager.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 86a241faa7..268c860f35 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -127,6 +127,7 @@ export class Worker { executeOperation(operation) { this.operationsById.set(operation.id, operation); + operation.onComplete(this.onOperationComplete); return this.rendererProcess.executeOperation(operation); } @@ -134,8 +135,12 @@ export class Worker { handleDataReceived({id, results}) { const operation = this.operationsById.get(id); operation.complete(results); + } + + @autobind + onOperationComplete(operation) { this.completedOperationCount++; - this.operationsById.delete(id); + this.operationsById.delete(operation.id); if (this.sick && this.operationsById.size === 0) { this.destroy(); @@ -298,6 +303,11 @@ export class Operation { this.promise = null; this.status = Operation.status.PENDING; this.results = null; + this.emitter = new Emitter(); + } + + onComplete(cb) { + return this.emitter.on('complete', cb); } setPromise(promise) { @@ -317,6 +327,8 @@ export class Operation { this.results = results; this.resolve(results); this.status = Operation.status.COMPLETE; + this.emitter.emit('complete', this); + this.emitter.dispose(); } execute(execFn) { From 652459d810eb8862302c16a0ce37be01c15d099c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 15:47:33 -0700 Subject: [PATCH 59/78] Use static channelName for all ipc communication The sourceWebContentsId is now encoded in the message data --- lib/worker-manager.js | 21 ++++++++++----------- lib/worker.js | 12 +++++++----- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 268c860f35..a3c652b36d 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -90,6 +90,8 @@ export default class WorkerManager { export class Worker { + static channelName = 'github:renderer-ipc'; + constructor({operationCountLimit, onSick, onCrashed, onDestroyed}) { this.operationCountLimit = operationCountLimit; this.onSick = onSick; @@ -117,6 +119,7 @@ export class Worker { js: rendererJsPath, managerWebContentsId: this.getWebContentsId(), operationCountLimit, + channelName: Worker.channelName, }); return `file://${htmlPath}?${qs}`; } @@ -211,7 +214,6 @@ export class RendererProcess { this.win = new BrowserWindow({show: true || !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.webContents = this.win.webContents; // this.webContents.openDevTools(); - this.channelName = `message-${this.webContents.id}`; this.emitter = new Emitter(); this.subscriptions = new CompositeDisposable(); @@ -229,11 +231,6 @@ export class RendererProcess { this.readyPromise = new Promise(resolve => { this.resolveReady = resolve; }); } - @autobind - handleMessages(event, {type, data}) { - this.emitter.emit(type, data); - } - @autobind handleDestroy(...args) { this.destroy(); @@ -241,11 +238,13 @@ export class RendererProcess { } registerListeners() { - const handleMessages = (event, {type, data}) => { - this.emitter.emit(type, data); + const handleMessages = (event, {sourceWebContentsId, type, data}) => { + if (sourceWebContentsId === this.win.webContents.id) { + this.emitter.emit(type, data); + } }; - ipc.on(this.channelName, handleMessages); + ipc.on(Worker.channelName, handleMessages); this.emitter.on('renderer-ready', ({pid}) => { this.pid = pid; this.resolveReady(); @@ -255,7 +254,7 @@ export class RendererProcess { this.emitter.on('slow-spawns', this.onSick); this.subscriptions.add( - new Disposable(() => ipc.removeListener(this.channelName, handleMessages)), + new Disposable(() => ipc.removeListener(Worker.channelName, handleMessages)), ); } @@ -265,7 +264,7 @@ export class RendererProcess { await this.readyPromise; // Is this bad for always putting things on the next tick? return operation.execute(payload => { if (this.destroyed) { return null; } - return this.webContents.send(this.channelName, { + return this.webContents.send(Worker.channelName, { type: 'git-exec', data: payload, }); diff --git a/lib/worker.js b/lib/worker.js index 5a09bf943b..a94ae75d53 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -38,16 +38,18 @@ class AverageTracker { } } -const operationCountLimit = parseInt(qs.parse(window.location.search.substr(1)).operationCountLimit, 10); +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(qs.parse(window.location.search.substr(1)).managerWebContentsId, 10); +const managerWebContentsId = parseInt(query.managerWebContentsId, 10); const managerWebContents = remote.webContents.fromId(managerWebContentsId); managerWebContents.on('crashed', () => { destroyRenderer(); }); managerWebContents.on('destroyed', () => { destroyRenderer(); }); -const channelName = `message-${remote.getCurrentWindow().webContents.id}`; +const channelName = query.channelName; ipc.on(channelName, (event, {type, data}) => { if (type === 'git-exec') { const {args, workingDir, options, id} = data; @@ -65,7 +67,7 @@ ipc.on(channelName, (event, {type, data}) => { }); const spawnEnd = performance.now(); averageTracker.addValue(spawnEnd - spawnStart); - event.sender.sendTo(managerWebContentsId, channelName, {type: 'exec-started', data: {id}}); + 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'}); @@ -75,4 +77,4 @@ ipc.on(channelName, (event, {type, data}) => { } }); -ipc.sendTo(managerWebContentsId, channelName, {type: 'renderer-ready', data: {pid: process.pid}}); +ipc.sendTo(managerWebContentsId, channelName, {sourceWebContentsId, type: 'renderer-ready', data: {pid: process.pid}}); From 7e7d4cec245264f50941bce97321b944cf4cb069 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 15:48:24 -0700 Subject: [PATCH 60/78] Dispose of webContents listeners before destroying manager window --- lib/worker-manager.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index a3c652b36d..e1effcbb3b 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -217,16 +217,22 @@ export class RendererProcess { this.emitter = new Emitter(); this.subscriptions = new CompositeDisposable(); - this.subscriptions.add( - new Disposable(() => this.win.destroy()), - this.emitter, - ); this.registerListeners(); this.win.loadURL(loadUrl); // okay to move these to register listeners? 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.readyPromise = new Promise(resolve => { this.resolveReady = resolve; }); } From aa09a9618423214a014cac5a95cf4e0896ff34ce Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 15:49:08 -0700 Subject: [PATCH 61/78] Fix test --- test/worker-manager.test.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index a7481b96cf..351cd661c4 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -102,6 +102,7 @@ describe('WorkerManager', function() { 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(); @@ -112,18 +113,19 @@ describe('WorkerManager', function() { 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.isFalse(isProcessAlive(worker1.getPid())); - assert.isFalse(isProcessAlive(worker2.getPid())); + assert.isTrue(isProcessAlive(worker1.getPid())); + assert.isTrue(isProcessAlive(worker2.getPid())); [...worker1Operations, ...worker2Operations].forEach(operation => operation.complete()); - await assert.async.isTrue(isProcessAlive(worker1.getPid())); - await assert.async.isTrue(isProcessAlive(worker2.getPid())); + await assert.async.isFalse(isProcessAlive(worker1.getPid())); + await assert.async.isFalse(isProcessAlive(worker2.getPid())); }); }); From 863209dfdf9ca5b4e2c88a0b3acee157157d0886 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 15:49:19 -0700 Subject: [PATCH 62/78] Fix other test --- test/worker-manager.test.js | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index 351cd661c4..3892d0cf59 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -135,18 +135,30 @@ describe('WorkerManager', function() { browserWindow.loadURL('about:blank'); sinon.stub(Worker.prototype, 'getWebContentsId').returns(browserWindow.webContents.id); - workerManager = new WorkerManager(); - - const managerPid = await new Promise(resolve => { - browserWindow.webContents.executeJavaScript('process.pid', resolve); + 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(); - process.kill(managerPid, 'SIGKILL'); - await assert.async.isFalse(isProcessAlive(managerPid)); + browserWindow.destroy(); await assert.async.isFalse(isProcessAlive(worker1.getPid())); await assert.async.isFalse(isProcessAlive(worker2.getPid())); }); From 55f9882a5780d0fa0818cf32ef5f5a33b722eef7 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 15:57:38 -0700 Subject: [PATCH 63/78] Include sourceWebContentsId in git data ipc message --- lib/worker.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/worker.js b/lib/worker.js index 09436ddd07..ad71aac65c 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -60,6 +60,7 @@ ipc.on(channelName, (event, {type, data}) => { GitProcess.exec(args, workingDir, options) .then(({stdout, stderr, exitCode}) => { event.sender.sendTo(managerWebContentsId, channelName, { + sourceWebContentsId, type: 'git-data', data: { id, From 6502cbfeaa215d91c563bd43b2fc34243a9ea29d Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 15:59:44 -0700 Subject: [PATCH 64/78] Don't show browser window --- lib/worker-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index e1effcbb3b..e5ac4ca4c3 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -211,7 +211,7 @@ export class RendererProcess { this.onData = onData; this.onExecStarted = onExecStarted; - this.win = new BrowserWindow({show: true || !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); + this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); this.webContents = this.win.webContents; // this.webContents.openDevTools(); From c3b875e0177fc8e3759315273080d76ed6b3b08f Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 16:01:37 -0700 Subject: [PATCH 65/78] Don't reject promise for remaining operations during destroy --- lib/worker-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index e5ac4ca4c3..dff526833e 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -192,7 +192,7 @@ export class Worker { this.onDestroyed(this); if (this.operationsById.size > 0 && !force) { const remainingOperationPromises = this.getRemainingOperations() - .map(operation => operation.getPromise()); + .map(operation => operation.getPromise().catch(() => null)); await Promise.all(remainingOperationPromises); } this.rendererProcess.destroy(); From b7f4b204c628a3d09190a67a92e29bae5414f688 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 16:02:44 -0700 Subject: [PATCH 66/78] :fire: RendererProcessManager --- lib/renderer-process-manager.js | 136 -------------------------------- 1 file changed, 136 deletions(-) delete mode 100644 lib/renderer-process-manager.js diff --git a/lib/renderer-process-manager.js b/lib/renderer-process-manager.js deleted file mode 100644 index 995d93b727..0000000000 --- a/lib/renderer-process-manager.js +++ /dev/null @@ -1,136 +0,0 @@ -import path from 'path'; - -import {remote, ipcRenderer as ipc} from 'electron'; -const {BrowserWindow} = remote; -import {Emitter} from 'event-kit'; -import {autobind} from 'core-decorators'; - -import {getPackageRoot} from './helpers'; - -export default class RendererProcessManager { - static instance = null; - - static getInstance() { - if (!this.instance) { - this.instance = new RendererProcessManager(); - } - return this.instance; - } - - static reset() { - if (this.instance) { this.instance.destroy(); } - this.instance = null; - } - - constructor() { - this.hostWebContentsId = remote.getCurrentWebContents().id; - this.activeRenderer = null; - this.renderers = new Set(); - this.createNewRenderer(); - } - - @autobind - createNewRenderer(previousOperationCount = 0, previousLimit = 10) { - let newLimit = 10; - if (previousOperationCount === previousLimit && Math.random() < 0.85) { - newLimit = Math.min(previousLimit * 2, 100); - } - this.activeRenderer = new RendererProcess(this.hostWebContentsId, newLimit, { - onDying: (operationCount, limit) => { this.createNewRenderer(operationCount, limit); }, - onDestroyed: renderer => { this.renderers.delete(renderer); }, - }); - this.renderers.add(this.activeRenderer); - } - - async request(data) { - await this.activeRenderer.getReadyPromise(); - return this.activeRenderer.send(data); - } - - destroy() { - this.renderers.forEach(renderer => renderer.destroy()); - } -} - -class RendererProcess { - constructor(hostWebContentsId, limit, {onDying, onDestroyed}) { - this.hostWebContentsId = hostWebContentsId; - this.onDying = onDying; - this.onDestroyed = onDestroyed; - this.resolveByOperationId = new Map(); - this.operationId = 0; - this.dying = false; - - this.win = new BrowserWindow({show: !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW}); - this.win.webContents.openDevTools(); - this.childWebContentsId = this.win.webContents.id; - this.channelName = `message-${this.childWebContentsId}`; - - this.emitter = new Emitter(); - this.registerListeners(); - - const htmlPath = path.join(getPackageRoot(), 'lib', 'renderer.html'); - const rendererJsPath = path.join(getPackageRoot(), 'lib', 'renderer.js'); - this.win.loadURL( - `file://${htmlPath}?js=${rendererJsPath}&hostWebContentsId=${this.hostWebContentsId}&limit=${limit}`, - ); - - this.readyPromise = new Promise(resolve => { this.resolveReady = resolve; }); - } - - registerListeners() { - ipc.on(this.channelName, this.handleMessages); - this.emitter.on('renderer-ready', this.handleRendererReady); - this.emitter.on('git-data', this.handleGitDataReceived); - this.emitter.on('slow-spawns', this.handleDying); - } - - @autobind - handleMessages(event, {type, data}) { - this.emitter.emit(type, data); - } - - @autobind - handleRendererReady() { - this.resolveReady(); - } - - @autobind - handleGitDataReceived({operationId, results, average}) { - const resolve = this.resolveByOperationId.get(operationId); - resolve(results); - this.resolveByOperationId.delete(operationId); - - if (this.dying && this.resolveByOperationId.size === 0) { - this.destroy(); - this.onDestroyed(this); - } - } - - @autobind - handleDying({operationCount, limit}) { - if (this.dying) { return; } - this.dying = true; - this.onDying(operationCount, limit); - } - - send(data) { - return new Promise(resolve => { - this.resolveByOperationId.set(++this.operationId, resolve); - this.win.webContents.send(`message-${this.childWebContentsId}`, { - type: 'git-exec', - data: {...data, operationId: this.operationId}, - }); - }); - } - - getReadyPromise() { - return this.readyPromise; - } - - destroy() { - ipc.removeListener(this.channelName, this.handleMessages); - this.emitter.dispose(); - this.win.destroy(); - } -} From 5e34e0e25ebad787b0d17634c82f9f2aabbb4195 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 16:03:23 -0700 Subject: [PATCH 67/78] Force reset of WorkerManager after tests complete --- test/helpers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/helpers.js b/test/helpers.js index 94214ce3bb..d0d3c14411 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -192,6 +192,6 @@ afterEach(function() { // eslint-disable-next-line jasmine/no-global-setup after(() => { if (!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW) { - WorkerManager.reset(); + WorkerManager.reset(true); } }); From 6359895cc49c40b5b3a0c7cd89eeeabe0f1ade46 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 16:27:17 -0700 Subject: [PATCH 68/78] Implement WorkerManager.prototype.isReady --- lib/worker-manager.js | 18 ++++++++++++++++++ test/worker-manager.test.js | 13 +++++++++++++ 2 files changed, 31 insertions(+) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index dff526833e..13b7eba37a 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -29,6 +29,10 @@ export default class WorkerManager { this.createNewWorker(); } + isReady() { + return this.activeWorker.isReady(); + } + request(data) { if (this.destroyed) { throw new Error('Worker is destroyed'); } let operation; @@ -82,6 +86,10 @@ export default class WorkerManager { return this.activeWorker; } + getReadyPromise() { + return this.activeWorker.getReadyPromise(); + } + destroy(force) { this.destroyed = true; this.workers.forEach(worker => worker.destroy(force)); @@ -112,6 +120,10 @@ export class Worker { }); } + isReady() { + return this.rendererProcess.isReady(); + } + getLoadUrl(operationCountLimit) { const htmlPath = path.join(getPackageRoot(), 'lib', 'renderer.html'); const rendererJsPath = path.join(getPackageRoot(), 'lib', 'worker.js'); @@ -234,9 +246,14 @@ export class RendererProcess { this.emitter, ); + this.ready = false; this.readyPromise = new Promise(resolve => { this.resolveReady = resolve; }); } + isReady() { + return this.ready; + } + @autobind handleDestroy(...args) { this.destroy(); @@ -253,6 +270,7 @@ export class RendererProcess { 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); diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index 3892d0cf59..6977921c1b 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -14,6 +14,19 @@ describe('WorkerManager', function() { 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}); From c39aaf662ee424a61362e126ee71a3526f5c2aff Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 16:27:50 -0700 Subject: [PATCH 69/78] Shell out in process if WorkerManager instance isn't ready --- lib/git-shell-out-strategy.js | 2 +- test/git-strategies.test.js | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 18b37d178e..abb2d1986e 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -202,7 +202,7 @@ export default class GitShellOutStrategy { executeGitCommand(args, options) { const timeSent = performance.now(); // TODO: move to manager - if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC) { + if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC || !WorkerManager.getInstance().isReady()) { return GitProcess.exec(args, this.workingDir, options); } else { const workerManager = this.workerManager || WorkerManager.getInstance(); diff --git a/test/git-strategies.test.js b/test/git-strategies.test.js index 4367cdec0a..36dde4a2ea 100644 --- a/test/git-strategies.test.js +++ b/test/git-strategies.test.js @@ -3,9 +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 WorkerManager from '../lib/worker-manager'; import {cloneRepository, initRepository, assertDeepPropertyVals, setUpLocalAndRemoteRepositories} from './helpers'; import {fsStat} from '../lib/helpers'; @@ -913,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); + }); + }); }); }); From 95fb731b2b52b3d69dd69a70788efad58127e08b Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 25 Apr 2017 16:42:39 -0700 Subject: [PATCH 70/78] Don't await ready promise in RendererProcess#executeOperation --- lib/worker-manager.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 13b7eba37a..2b91bf82df 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -232,7 +232,6 @@ export class RendererProcess { this.registerListeners(); this.win.loadURL(loadUrl); - // okay to move these to register listeners? this.win.webContents.on('crashed', this.handleDestroy); this.win.webContents.on('destroyed', this.handleDestroy); this.subscriptions.add( @@ -282,10 +281,7 @@ export class RendererProcess { ); } - async executeOperation(operation) { - // TODO: for now let's just queue up promises for execution. - // in the future we'll probably shell out in process while awaiting the renderer to be ready - await this.readyPromise; // Is this bad for always putting things on the next tick? + executeOperation(operation) { return operation.execute(payload => { if (this.destroyed) { return null; } return this.webContents.send(Worker.channelName, { From 7a954461d1822ce80a2a9e4a4fd2a372e2772a29 Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Tue, 25 Apr 2017 22:55:00 -0700 Subject: [PATCH 71/78] First hacky pass at IPC-based Git timings --- lib/git-shell-out-strategy.js | 21 +++++++++++++-------- lib/views/git-timings-view.js | 5 +++-- lib/worker-manager.js | 24 +++++++++++++++++++++--- lib/worker.js | 9 +++++++-- 4 files changed, 44 insertions(+), 15 deletions(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index abb2d1986e..68aa38b8b1 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -159,10 +159,14 @@ export default class GitShellOutStrategy { console.time(`git:${formattedArgs}`); } return new Promise(async (resolve, reject) => { - timingMarker.mark('nexttick'); - timingMarker.mark('execute'); - - const {stdout, stderr, exitCode} = await this.executeGitCommand(args, options); + 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}`); @@ -200,17 +204,18 @@ export default class GitShellOutStrategy { /* eslint-enable no-console */ } - executeGitCommand(args, options) { - const timeSent = performance.now(); // TODO: move to manager + executeGitCommand(args, options, marker = null) { if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC || !WorkerManager.getInstance().isReady()) { - return GitProcess.exec(args, this.workingDir, options); + 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, - timeSent, }); } } 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 index 2b91bf82df..d3143a7bf5 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -149,7 +149,13 @@ export class Worker { @autobind handleDataReceived({id, results}) { const operation = this.operationsById.get(id); - operation.complete(results); + 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 @@ -320,6 +326,8 @@ export class Operation { 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(); @@ -342,15 +350,25 @@ export class Operation { this.status = Operation.status.INPROGRESS; } - complete(results) { + 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(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 index ad71aac65c..f3646a7fb3 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -57,19 +57,24 @@ ipc.on(channelName, (event, {type, data}) => { document.getElementById('command').textContent = `git ${args.join(' ')}`; } const spawnStart = performance.now(); + let spawnEnd; 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}, + results: {stdout, stderr, exitCode, timing}, }, }); }); - const spawnEnd = performance.now(); + spawnEnd = performance.now(); averageTracker.addValue(spawnEnd - spawnStart); event.sender.sendTo(managerWebContentsId, channelName, {sourceWebContentsId, type: 'exec-started', data: {id}}); From 5d309d1f3b892d530772cfa4001543cae9a8f4c0 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 26 Apr 2017 13:51:27 -0700 Subject: [PATCH 72/78] :shirt: --- lib/worker.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/worker.js b/lib/worker.js index f3646a7fb3..58be23886a 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -57,7 +57,6 @@ ipc.on(channelName, (event, {type, data}) => { document.getElementById('command').textContent = `git ${args.join(' ')}`; } const spawnStart = performance.now(); - let spawnEnd; GitProcess.exec(args, workingDir, options) .then(({stdout, stderr, exitCode}) => { const timing = { @@ -74,7 +73,7 @@ ipc.on(channelName, (event, {type, data}) => { }, }); }); - spawnEnd = performance.now(); + const spawnEnd = performance.now(); averageTracker.addValue(spawnEnd - spawnStart); event.sender.sendTo(managerWebContentsId, channelName, {sourceWebContentsId, type: 'exec-started', data: {id}}); From 3066270c4ed1e01ea91cac13806f940e3af69703 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 26 Apr 2017 14:19:15 -0700 Subject: [PATCH 73/78] Don't clog up IPC channel with 'exec-started' events --- lib/worker-manager.js | 5 ++++- lib/worker.js | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index d3143a7bf5..2f3a88d146 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -279,9 +279,12 @@ export class RendererProcess { this.resolveReady(); }); this.emitter.on('git-data', this.onData); - this.emitter.on('exec-started', this.onExecStarted); 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)), ); diff --git a/lib/worker.js b/lib/worker.js index 58be23886a..da5f35aadf 100644 --- a/lib/worker.js +++ b/lib/worker.js @@ -75,7 +75,10 @@ ipc.on(channelName, (event, {type, data}) => { }); const spawnEnd = performance.now(); averageTracker.addValue(spawnEnd - spawnStart); - event.sender.sendTo(managerWebContentsId, channelName, {sourceWebContentsId, type: 'exec-started', data: {id}}); + + // 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'}); From 4084aec06491cfc61f45bf2fbe73d59895e54634 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 26 Apr 2017 14:53:52 -0700 Subject: [PATCH 74/78] Destroy WorkerManager in deactivate --- lib/github-package.js | 2 ++ 1 file changed, 2 insertions(+) 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 From 87270176922aaf9dc66d63408a1ba5fc2399c65c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 26 Apr 2017 18:40:15 -0700 Subject: [PATCH 75/78] Add console.warn to indicate sick worker --- lib/worker-manager.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index 2f3a88d146..da8c6e5993 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -70,6 +70,10 @@ export default class WorkerManager { @autobind onSick(sickWorker) { + // 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}); } From 7bab3da7f162c101bd9ff8fcab022356f2c64f0a Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 26 Apr 2017 18:45:53 -0700 Subject: [PATCH 76/78] :keyboard: --- lib/renderer.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/renderer.html b/lib/renderer.html index 77fb7d41f7..ea43dbef4d 100644 --- a/lib/renderer.html +++ b/lib/renderer.html @@ -12,7 +12,7 @@

GitHub Package Git Execution Window

- Hi there! I'm a window used by the GitHub pacakge to execute Git commands in the background. My PID is . + Hi there! I'm a window used by the GitHub package to execute Git commands in the background. My PID is .

Last command:

From 1740e9b238e6f51aa488018a42f31523f6fe497c Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 26 Apr 2017 18:50:36 -0700 Subject: [PATCH 77/78] Use assert.lengthOf --- test/worker-manager.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/worker-manager.test.js b/test/worker-manager.test.js index 6977921c1b..9491114c46 100644 --- a/test/worker-manager.test.js +++ b/test/worker-manager.test.js @@ -38,7 +38,7 @@ describe('WorkerManager', function() { workerManager.request(); workerManager.request(); const worker1OperationsInFlight = worker1.getRemainingOperations(); - assert.equal(worker1OperationsInFlight.length, 3); + assert.lengthOf(worker1OperationsInFlight, 3); const worker1Pid = worker1.getPid(); process.kill(worker1Pid, 'SIGKILL'); From e70866845969f15f4cb1feb693d7fa8528592ea5 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 26 Apr 2017 19:13:22 -0700 Subject: [PATCH 78/78] Only log about sick worker if not in spec mode --- lib/worker-manager.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/worker-manager.js b/lib/worker-manager.js index da8c6e5993..91f4105873 100644 --- a/lib/worker-manager.js +++ b/lib/worker-manager.js @@ -70,10 +70,12 @@ export default class WorkerManager { @autobind onSick(sickWorker) { - // eslint-disable-next-line no-console - console.warn(`Sick worker detected. - operationCountLimit: ${sickWorker.getOperationCountLimit()}, - completed operation count: ${sickWorker.getCompletedOperationCount()}`); + 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}); }