Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

Run git commands in dedicated renderer process #688

Merged
merged 82 commits into from
Apr 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
82 commits
Select commit Hold shift + click to select a range
c07d65b
WIP compare renderer and child process IPC times
kuychaco Apr 13, 2017
53384de
WIP shell out to git in renderer process
kuychaco Apr 13, 2017
519ebfc
WIP Create RendererProcessManager
kuychaco Apr 14, 2017
c9b2978
:arrow_up: atom-mocha-test-runner@1.0.1
kuychaco Apr 17, 2017
f91e8cd
Make RendererProcessManager a singleton
kuychaco Apr 17, 2017
8fe00d5
Actually import GitError
kuychaco Apr 17, 2017
e9aa22d
Use import instead of require
kuychaco Apr 17, 2017
169ab20
Don't show browser window for git renderer process
kuychaco Apr 18, 2017
72b5ccc
WIP wtf is up w/ the tests
kuychaco Apr 19, 2017
761ff7d
Wait for renderer process to be ready before sending Git commands
Apr 19, 2017
54c1653
Destroy any Repositories created during tests after each test
Apr 19, 2017
09fdc01
No need to reset singleton RendererProcessManager after each test
Apr 19, 2017
197b332
Conditionally choose exec strategy
Apr 19, 2017
364b086
Fix global test setup
Apr 19, 2017
ed4c5ed
Extract Git execution into a separate method for stubbing
Apr 19, 2017
ca3411c
Un-pend test
Apr 19, 2017
bc7ae77
Fix flaky test
BinaryMuse Apr 19, 2017
3bda0a9
:art:
BinaryMuse Apr 19, 2017
2228185
:fire: git-child-process.js
kuychaco Apr 19, 2017
325f48b
:art: and clean up
kuychaco Apr 19, 2017
2bd9e37
More clean up
kuychaco Apr 19, 2017
684ca4a
:fire: comments
kuychaco Apr 19, 2017
6494404
Remove type property from ipc data and rely on channel to identify type
kuychaco Apr 19, 2017
e7074b7
:fire: unused changes to GitTimingsView
kuychaco Apr 19, 2017
5d2fef3
Actually, we want to test that...
kuychaco Apr 19, 2017
9b3a288
Put those timeouts back
kuychaco Apr 19, 2017
0187c4b
Mo :art:
kuychaco Apr 19, 2017
de1918b
Communicate on ipc channel based on renderer's web contents id
kuychaco Apr 19, 2017
b86ad95
Delete entry in RendererProcessManager map after promise is resolved
kuychaco Apr 19, 2017
66a1a08
rendererWebContentsId -> childWebContentsID
kuychaco Apr 19, 2017
ff425c4
Move operation management from RendererProcessManager into RendererPr…
kuychaco Apr 20, 2017
bcd2722
WIP Implement logic for creating new renderer process
kuychaco Apr 20, 2017
8bfa4ae
Merge branch 'master' into ku-renderer-shell-out
kuychaco Apr 20, 2017
4557f46
Refactor RendererProcessManager -> WorkerManager. Handle destroyed pr…
kuychaco Apr 21, 2017
097714d
Set operation to pending after call to GitProcess.exec
kuychaco Apr 22, 2017
ce2bcc3
Wait for all write operations to finish before destroying renderer pr…
kuychaco Apr 22, 2017
8e211a1
Add test for fallback when worker process crashes
kuychaco Apr 24, 2017
e43c271
Destroy RendererProcess instance on 'crashed' event
kuychaco Apr 25, 2017
6505e04
Add test for handling when a worker is sick
kuychaco Apr 25, 2017
23026e2
Execute remaining operations in newly active worker when sick process…
kuychaco Apr 25, 2017
7b29cc7
Use global sinon
kuychaco Apr 25, 2017
045c009
Destroy worker managers in tests
kuychaco Apr 25, 2017
1038161
Don't care if operation is a write
kuychaco Apr 25, 2017
29c36bb
Add test for ensuring that operations are complete before destroying …
kuychaco Apr 25, 2017
cd71d68
Rename renderer.js to worker.js
kuychaco Apr 25, 2017
77fdc0d
Add test case for killing renderer processes when worker manager crashes
kuychaco Apr 25, 2017
0bb8e57
Show browser windows if process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW …
kuychaco Apr 25, 2017
d11877e
Don't create new worker if WorkerManager is destroyed
kuychaco Apr 25, 2017
80956e2
Don't open dev tools
kuychaco Apr 25, 2017
94e0b8b
:art: move destroy method to bottom
kuychaco Apr 25, 2017
53bf504
:art:
kuychaco Apr 25, 2017
5db1010
Merge remote-tracking branch 'origin/master' into ku-renderer-shell-out
Apr 25, 2017
920b4ca
:fire: .only
Apr 25, 2017
91908e5
:fire: error when pushing into a disposed AsyncQueue
Apr 25, 2017
d868091
Fix failure to clone when target folder doesn't exist
Apr 25, 2017
c991157
Let the worker window tell us what it's doing
Apr 25, 2017
5743cb3
Destroy the old WorkerManager before creating a new one.
Apr 25, 2017
dca043e
Stub Operation.prototype.execute instead of complete
kuychaco Apr 25, 2017
fc16dd5
Throw error in isProcessAlive helper if pid is not a number
kuychaco Apr 25, 2017
a0df97c
Decouple completing operation and post-completion cleanup tasks
kuychaco Apr 25, 2017
652459d
Use static channelName for all ipc communication
kuychaco Apr 25, 2017
7e7d4ce
Dispose of webContents listeners before destroying manager window
kuychaco Apr 25, 2017
aa09a96
Fix test
kuychaco Apr 25, 2017
863209d
Fix other test
kuychaco Apr 25, 2017
bc3073c
Merge branch 'ku-renderer-shell-out' of github.com:atom/github into k…
kuychaco Apr 25, 2017
55f9882
Include sourceWebContentsId in git data ipc message
kuychaco Apr 25, 2017
6502cbf
Don't show browser window
kuychaco Apr 25, 2017
c3b875e
Don't reject promise for remaining operations during destroy
kuychaco Apr 25, 2017
b7f4b20
:fire: RendererProcessManager
kuychaco Apr 25, 2017
5e34e0e
Force reset of WorkerManager after tests complete
kuychaco Apr 25, 2017
6359895
Implement WorkerManager.prototype.isReady
kuychaco Apr 25, 2017
c39aaf6
Shell out in process if WorkerManager instance isn't ready
kuychaco Apr 25, 2017
95fb731
Don't await ready promise in RendererProcess#executeOperation
kuychaco Apr 25, 2017
7a95446
First hacky pass at IPC-based Git timings
Apr 26, 2017
5d309d1
:shirt:
kuychaco Apr 26, 2017
3066270
Don't clog up IPC channel with 'exec-started' events
kuychaco Apr 26, 2017
4084aec
Destroy WorkerManager in deactivate
kuychaco Apr 26, 2017
8727017
Add console.warn to indicate sick worker
kuychaco Apr 27, 2017
7bab3da
:keyboard:
kuychaco Apr 27, 2017
1740e9b
Use assert.lengthOf
kuychaco Apr 27, 2017
c3b4b23
Merge branch 'master' into ku-renderer-shell-out
kuychaco Apr 27, 2017
e708668
Only log about sick worker if not in spec mode
kuychaco Apr 27, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"MouseEvent": true,
"IDBKeyRange": true,
"beforeEach": true,
"after": true,
"afterEach": true,
"describe": true,
"fdescribe": true,
Expand Down
3 changes: 0 additions & 3 deletions lib/async-queue.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion lib/controllers/git-tab-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ export default class GitTabController {
const pathsToIgnore = [];
const repository = this.getActiveRepository();
for (const filePath of filePaths) {
if (await repository.pathHasMergeMarkers(filePath)) { // eslint-disable-line babel/no-await-in-loop
if (await repository.pathHasMergeMarkers(filePath)) { // eslint-disable-line no-await-in-loop
const choice = atom.confirm({
message: 'File contains merge markers: ',
detailedMessage: `Do you still want to stage this file?\n${filePath}`,
Expand Down
104 changes: 62 additions & 42 deletions lib/git-shell-out-strategy.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import path from 'path';
import os from 'os';

import {CompositeDisposable} from 'event-kit';

import {GitProcess} from 'dugite';
import {parse as parseDiff} from 'what-the-diff';

import GitPromptServer from './git-prompt-server';
import AsyncQueue from './async-queue';
import {getPackageRoot, getDugitePath, readFile, fileExists, writeFile, isFileExecutable} from './helpers';
import {getPackageRoot, getDugitePath, readFile, fileExists, fsStat, writeFile, isFileExecutable} from './helpers';
import GitTimingsView from './views/git-timings-view';
import WorkerManager from './worker-manager';

const LINE_ENDING_REGEX = /\r?\n/;

Expand Down Expand Up @@ -57,6 +57,7 @@ export default class GitShellOutStrategy {
}

this.prompt = options.prompt || (query => Promise.reject());
this.workerManager = options.workerManager;
}

/*
Expand Down Expand Up @@ -131,6 +132,7 @@ export default class GitShellOutStrategy {
const options = {
env,
processCallback: child => {
// TODO: move callback to renderer process. send child.pid back to add cancel listener
child.on('error', err => {
console.warn('Error executing: ' + formattedArgs + ':');
console.warn(err.stack);
Expand All @@ -156,51 +158,68 @@ export default class GitShellOutStrategy {
if (process.env.PRINT_GIT_TIMES) {
console.time(`git:${formattedArgs}`);
}
return new Promise(resolve => {
timingMarker.mark('nexttick');
setImmediate(() => {
timingMarker.mark('execute');
resolve(GitProcess.exec(args, this.workingDir, options)
.then(({stdout, stderr, exitCode}) => {
timingMarker.finalize();
if (process.env.PRINT_GIT_TIMES) {
console.timeEnd(`git:${formattedArgs}`);
}
if (gitPromptServer) {
gitPromptServer.terminate();
}
subscriptions.dispose();

if (diagnosticsEnabled) {
const headerStyle = 'font-weight: bold; color: blue;';

console.groupCollapsed(`git:${formattedArgs}`);
console.log('%cexit status%c %d', headerStyle, 'font-weight: normal; color: black;', exitCode);
console.log('%cstdout', headerStyle);
console.log(stdout);
console.log('%cstderr', headerStyle);
console.log(stderr);
console.groupEnd();
}

if (exitCode) {
const err = new GitError(
`${formattedArgs} exited with code ${exitCode}\nstdout: ${stdout}\nstderr: ${stderr}`,
);
err.code = exitCode;
err.stdErr = stderr;
err.stdOut = stdout;
err.command = formattedArgs;
return Promise.reject(err);
}
return stdout;
}));
});
return new Promise(async (resolve, reject) => {
const {stdout, stderr, exitCode, timing} = await this.executeGitCommand(args, options, timingMarker);
if (timing) {
const {execTime, spawnTime, ipcTime} = timing;
const now = performance.now();
timingMarker.mark('nexttick', now - execTime - spawnTime - ipcTime);
timingMarker.mark('execute', now - execTime - ipcTime);
timingMarker.mark('ipc', now - ipcTime);
}
timingMarker.finalize();
if (process.env.PRINT_GIT_TIMES) {
console.timeEnd(`git:${formattedArgs}`);
}
if (gitPromptServer) {
gitPromptServer.terminate();
}
subscriptions.dispose();

if (diagnosticsEnabled) {
const headerStyle = 'font-weight: bold; color: blue;';

console.groupCollapsed(`git:${formattedArgs}`);
console.log('%cexit status%c %d', headerStyle, 'font-weight: normal; color: black;', exitCode);
Copy link
Contributor

Choose a reason for hiding this comment

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

The day I discovered how %c works in console.log() was a dangerous day.

console.log('%cstdout', headerStyle);
console.log(stdout);
console.log('%cstderr', headerStyle);
console.log(stderr);
console.groupEnd();
}

if (exitCode) {
const err = new GitError(
`${formattedArgs} exited with code ${exitCode}\nstdout: ${stdout}\nstderr: ${stderr}`,
);
err.code = exitCode;
err.stdErr = stderr;
err.stdOut = stdout;
err.command = formattedArgs;
reject(err);
}
resolve(stdout);
});
}, {parallel: !writeOperation});
/* eslint-enable no-console */
}

executeGitCommand(args, options, marker = null) {
if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC || !WorkerManager.getInstance().isReady()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for offering an env var to opt out of the workers for when someone runs this on their Raspberry Pi 😉

If I'm reading it right it'll prevent the first worker from spawning too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm reading it right it'll prevent the first worker from spawning too?

Yup 👍 . Second expression after || doesn't get evaluated if first expression evaluates to true

marker && marker.mark('nexttick');
const promise = GitProcess.exec(args, this.workingDir, options);
marker && marker.mark('execute');
return promise;
} else {
const workerManager = this.workerManager || WorkerManager.getInstance();
return workerManager.request({
args,
workingDir: this.workingDir,
options,
});
}
}

/**
* Execute a git command that may create a commit. If the command fails because the GPG binary was invoked and unable
* to acquire a passphrase (because the pinentry program attempted to use a tty), retry with a `GitPromptServer`.
Expand All @@ -220,6 +239,7 @@ export default class GitShellOutStrategy {

async isGitRepository() {
try {
await fsStat(this.workingDir); // fails if folder doesn't exist
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, cool. This fixes that uncaught-exception-in-Promise from the test suite I presume.

await this.exec(['rev-parse', '--resolve-git-dir', path.join(this.workingDir, '.git')]);
return true;
} catch (e) {
Expand Down
2 changes: 2 additions & 0 deletions lib/github-package.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -216,6 +217,7 @@ export default class GithubPackage {
this.subscriptions.dispose();
this.contextPool.clear();
await yardstick.flush();
WorkerManager.reset();
}

@autobind
Expand Down
19 changes: 19 additions & 0 deletions lib/renderer.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title></title>
<script type="text/javascript">
const qs = require('querystring')
const jsPath = qs.parse(window.location.search.substr(1)).js
require(jsPath)
</script>
</head>
<body>
<h1>GitHub Package Git Execution Window</h1>
<p>
Hi there! I'm a window used by the GitHub package to execute Git commands in the background. My PID is <script>document.write(process.pid)</script>.
</p>
<p>Last command: <span id='command'></span></p>
</body>
</html>
5 changes: 3 additions & 2 deletions lib/views/git-timings-view.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -98,6 +98,7 @@ const COLORS = {
prepare: 'cyan',
nexttick: 'yellow',
execute: 'green',
ipc: 'pink',
};
class MarkerSpan extends React.Component {
static propTypes = {
Expand Down
Loading