New issue

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

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

Already on GitHub? Sign in to your account

Run git commands in dedicated renderer process #688

Merged
merged 82 commits into from Apr 27, 2017

Conversation

Projects
None yet
3 participants
@kuychaco
Member

kuychaco commented Apr 20, 2017

Fixes #386.
Fixes #643.

This PR addresses the issue of Atom freezing due to multiple blocking spawn calls when shelling out to Git in process (#386).

The Options

The solution is to make Git calls in a separate process, options being a Node child process, Electron renderer process, or Chromium web worker.

This PR implements a dedicated renderer process for shelling out to Git.

Basic testing with a Node child process revealed quadratic growth in IPC time relative to message size so this approach was dismissed due to the negative performance implications. The fix for this won't be released until Node v7.5.0.

Web workers are the ideal solution, but will have to wait until Atom is upgraded to Electron v1.6.4 which adds Node integration to web workers.

Current Approach

This PR introduces a WorkerManager which creates a Worker that wraps a RendererProcess which shells out to Git and sends results back over IPC. If the renderer process is not yet ready, we fall back to shelling out in process.

The renderer process keeps a running average of the time it takes to make a spawn call and if this exceeds 20ms (and the minimum number of operations specified have been completed), the WorkerManager creates a new RendererProcess and routes all new Git data requests to it. Once the "sick" renderer process finishes its operations it will be destroyed. The new renderer process may have a higher operationCountLimit, so that if the spawn calls continue to take longer than 20ms, we'll wait longer before creating a new RendererProcess in order to prevent process thrashing on slow machines.

If a renderer process crashes, the WorkerManager will create a new RendererProcess with the same operationCountLimit and send all incomplete operations to the new process.

If the WorkerManager process is destroyed, all RendererProcess instances will immediately destroy themselves.

If a Worker has it's destroy method called and there are remaining operations to be completed, we wait until completion before destroying the associated RendererProcess. This is in the event that there is a write operation that is still in progress and we want to allow it to reach disk.

Things to consider for future PRs:

  • Avoid duplicate write operations when a renderer process crashes mid-write and a new one is created that picks up the unfinished operation. This can be done by marking an Operation as "in-progress" by sending an ipc message after GitProcess.exec has been called.

kuychaco and others added some commits Apr 13, 2017

Make RendererProcessManager a singleton
Only need one RendererProcessManager for each Atom window. Also reset
RendererProcessManager in afterEach.
🔥 git-child-process.js
Since we're using a renderer process for spawning git processes right
now
rendererWebContentsId -> childWebContentsID
... to go along with hostWebContentsId
@smashwilson

This comment has been minimized.

Show comment
Hide comment
Member

smashwilson commented Apr 20, 2017

salute

BinaryMuse added some commits Apr 25, 2017

@BinaryMuse

A few things to clean up and take care of, but overall this PR is ! Great job!

Show outdated Hide outdated lib/worker-manager.js
if (this.operationsById.size > 0 && !force) {
const remainingOperationPromises = this.getRemainingOperations()
.map(operation => operation.getPromise());
await Promise.all(remainingOperationPromises);

This comment has been minimized.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

If any of remainingOperationPromises are rejected, then the Promise.all will be immediately rejected. You probably want to convert each of these into a fulfilled promise using catch or similar.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

If any of remainingOperationPromises are rejected, then the Promise.all will be immediately rejected. You probably want to convert each of these into a fulfilled promise using catch or similar.

Show outdated Hide outdated lib/worker-manager.js
this.onData = onData;
this.onExecStarted = onExecStarted;
this.win = new BrowserWindow({show: true || !!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW});

This comment has been minimized.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

🔥 true ||

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

🔥 true ||

Show outdated Hide outdated lib/renderer-process-manager.js
import {getPackageRoot} from './helpers';
export default class RendererProcessManager {

This comment has been minimized.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

Should this file still exist?

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

Should this file still exist?

Show outdated Hide outdated lib/worker-manager.js
this.registerListeners();
this.win.loadURL(loadUrl);
// okay to move these to register listeners?

This comment has been minimized.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

Unsure what this comment means

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

Unsure what this comment means

Show outdated Hide outdated lib/worker-manager.js
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

This comment has been minimized.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

Can we do this at the GSOS level? If the manager is ready, send it the request, but otherwise use the other branch of execution that shells out in process?

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

Can we do this at the GSOS level? If the manager is ready, send it the request, but otherwise use the other branch of execution that shells out in process?

Show outdated Hide outdated lib/worker.js
const operationCountLimit = parseInt(qs.parse(window.location.search.substr(1)).operationCountLimit, 10);
const averageTracker = new AverageTracker({limit: operationCountLimit});
const destroyRenderer = () => { remote.BrowserWindow.fromWebContents(remote.getCurrentWebContents()).destroy(); };

This comment has been minimized.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

Minor nit: we're actually destroying the browser window here

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

Minor nit: we're actually destroying the browser window here

Show outdated Hide outdated lib/worker.js
if (type === 'git-exec') {
const {args, workingDir, options, id} = data;
const spawnStart = performance.now();
GitProcess.exec(args, workingDir, options)

This comment has been minimized.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

If GitProcess.exec throws an exception for any reason, or returns a rejected promise, then we won't catch it and send any data back to the main renderer process.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

If GitProcess.exec throws an exception for any reason, or returns a rejected promise, then we won't catch it and send any data back to the main renderer process.

@@ -158,6 +164,16 @@ export function createRenderer() {
return renderer;
}
export function isProcessAlive(pid) {

This comment has been minimized.

@smashwilson

smashwilson Apr 27, 2017

Member

I'll be honest: before I clicked that link, I really expected "Stayin' Alive." Should have known better 😆

@smashwilson

smashwilson Apr 27, 2017

Member

I'll be honest: before I clicked that link, I really expected "Stayin' Alive." Should have known better 😆

Show outdated Hide outdated test/helpers.js
// eslint-disable-next-line jasmine/no-global-setup
after(() => {
if (!process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW) {
WorkerManager.reset();

This comment has been minimized.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

Do we want to pass true to the force parameter here?

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

Do we want to pass true to the force parameter here?

Show outdated Hide outdated test/worker-manager.test.js
assert.equal(worker2Operations.length, 2);
workerManager.destroy();
assert.isFalse(isProcessAlive(worker1.getPid()));

This comment has been minimized.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

worker1.getPid() and worker2.getPid() both return undefined because we're not waiting for the ready promise here.

@BinaryMuse

BinaryMuse Apr 25, 2017

Member

worker1.getPid() and worker2.getPid() both return undefined because we're not waiting for the ready promise here.

BinaryMuse and others added some commits Apr 25, 2017

Use static channelName for all ipc communication
The sourceWebContentsId is now encoded in the message data

@kuychaco kuychaco requested a review from smashwilson Apr 26, 2017

kuychaco added some commits Apr 26, 2017

@smashwilson

This is really, really cool. I'll install this branch and give it a shot over the next few days but I'd ❤️ to get this merged and out to our staff users as soon as we can 😁

});
}, {parallel: !writeOperation});
/* eslint-enable no-console */
}
executeGitCommand(args, options, marker = null) {
if (process.env.ATOM_GITHUB_INLINE_GIT_EXEC || !WorkerManager.getInstance().isReady()) {

This comment has been minimized.

@smashwilson

smashwilson Apr 27, 2017

Member

👍 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?

@smashwilson

smashwilson Apr 27, 2017

Member

👍 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?

This comment has been minimized.

@kuychaco

kuychaco Apr 27, 2017

Member

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

@kuychaco

kuychaco Apr 27, 2017

Member

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

@@ -220,6 +239,7 @@ export default class GitShellOutStrategy {
async isGitRepository() {
try {
await fsStat(this.workingDir); // fails if folder doesn't exist

This comment has been minimized.

@smashwilson

smashwilson Apr 27, 2017

Member

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

@smashwilson

smashwilson Apr 27, 2017

Member

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

Show outdated Hide outdated lib/renderer.html
<body>
<h1>GitHub Package Git Execution Window</h1>
<p>
Hi there! I'm a window used by the GitHub pacakge to execute Git commands in the background. My PID is <script>document.write(process.pid)</script>.

This comment has been minimized.

@smashwilson

smashwilson Apr 27, 2017

Member

✍️ "GitHub pacakge"

(... Nobody will ever see this 😛 )

@smashwilson

smashwilson Apr 27, 2017

Member

✍️ "GitHub pacakge"

(... Nobody will ever see this 😛 )

static status = {
PENDING: Symbol('pending'),
INPROGRESS: Symbol('in-progress'),
COMPLETE: Symbol('complete'),

This comment has been minimized.

@smashwilson

smashwilson Apr 27, 2017

Member

Hooray for Symbols 🎉

@smashwilson

smashwilson Apr 27, 2017

Member

Hooray for Symbols 🎉

this.startTime = performance.now();
return execFn({...this.data, id: this.id});
}
}

This comment has been minimized.

@smashwilson

smashwilson Apr 27, 2017

Member

This WorkerManager/Operation setup is downright elegant 😄

@smashwilson

smashwilson Apr 27, 2017

Member

This WorkerManager/Operation setup is downright elegant 😄

This comment has been minimized.

@kuychaco

kuychaco Apr 27, 2017

Member

🙇‍♀️

@kuychaco

kuychaco Apr 27, 2017

Member

🙇‍♀️

// 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) {

This comment has been minimized.

@smashwilson

smashwilson Apr 27, 2017

Member

I'll be sure to give this a shot on the Windows box to see what the spawn latency is like here. If 20ms is enough here it should be enough for anywhere.

smashwilson's computer irl

@smashwilson

smashwilson Apr 27, 2017

Member

I'll be sure to give this a shot on the Windows box to see what the spawn latency is like here. If 20ms is enough here it should be enough for anywhere.

smashwilson's computer irl

This comment has been minimized.

@kuychaco

kuychaco Apr 27, 2017

Member

Thanks yo

@kuychaco

kuychaco Apr 27, 2017

Member

Thanks yo

@@ -158,6 +164,16 @@ export function createRenderer() {
return renderer;
}
export function isProcessAlive(pid) {

This comment has been minimized.

@smashwilson

smashwilson Apr 27, 2017

Member

I'll be honest: before I clicked that link, I really expected "Stayin' Alive." Should have known better 😆

@smashwilson

smashwilson Apr 27, 2017

Member

I'll be honest: before I clicked that link, I really expected "Stayin' Alive." Should have known better 😆

Show outdated Hide outdated test/worker-manager.test.js
workerManager.request();
workerManager.request();
const worker1OperationsInFlight = worker1.getRemainingOperations();
assert.equal(worker1OperationsInFlight.length, 3);

This comment has been minimized.

@smashwilson

smashwilson Apr 27, 2017

Member

There's an assert.lengthOf(...) too... I think it shows you the array contents if the length is wrong but it isn't too long?

@smashwilson

smashwilson Apr 27, 2017

Member

There's an assert.lengthOf(...) too... I think it shows you the array contents if the length is wrong but it isn't too long?

Show outdated Hide outdated test/worker-manager.test.js
assert.equal(worker1OperationsInFlight.length, 3);
const worker1Pid = worker1.getPid();
process.kill(worker1Pid, 'SIGKILL');

This comment has been minimized.

This comment has been minimized.

@smashwilson

smashwilson Apr 27, 2017

Member

Haha, to clarify, this was my attempt at a SIGKILL emoji, not an indication that this line shouldn't be here or anything.

@smashwilson

smashwilson Apr 27, 2017

Member

Haha, to clarify, this was my attempt at a SIGKILL emoji, not an indication that this line shouldn't be here or anything.

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

This comment has been minimized.

@smashwilson

smashwilson Apr 27, 2017

Member

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

@smashwilson

smashwilson Apr 27, 2017

Member

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

@kuychaco kuychaco merged commit 9b8ed5d into master Apr 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kuychaco kuychaco deleted the ku-renderer-shell-out branch Apr 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment