This repository was archived by the owner on Dec 15, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 408
Run git commands in dedicated renderer process #688
Merged
Merged
Changes from 53 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 53384de
WIP shell out to git in renderer process
kuychaco 519ebfc
WIP Create RendererProcessManager
kuychaco c9b2978
:arrow_up: [email protected]
kuychaco f91e8cd
Make RendererProcessManager a singleton
kuychaco 8fe00d5
Actually import GitError
kuychaco e9aa22d
Use import instead of require
kuychaco 169ab20
Don't show browser window for git renderer process
kuychaco 72b5ccc
WIP wtf is up w/ the tests
kuychaco 761ff7d
Wait for renderer process to be ready before sending Git commands
54c1653
Destroy any Repositories created during tests after each test
09fdc01
No need to reset singleton RendererProcessManager after each test
197b332
Conditionally choose exec strategy
364b086
Fix global test setup
ed4c5ed
Extract Git execution into a separate method for stubbing
ca3411c
Un-pend test
bc7ae77
Fix flaky test
BinaryMuse 3bda0a9
:art:
BinaryMuse 2228185
:fire: git-child-process.js
kuychaco 325f48b
:art: and clean up
kuychaco 2bd9e37
More clean up
kuychaco 684ca4a
:fire: comments
kuychaco 6494404
Remove type property from ipc data and rely on channel to identify type
kuychaco e7074b7
:fire: unused changes to GitTimingsView
kuychaco 5d2fef3
Actually, we want to test that...
kuychaco 9b3a288
Put those timeouts back
kuychaco 0187c4b
Mo :art:
kuychaco de1918b
Communicate on ipc channel based on renderer's web contents id
kuychaco b86ad95
Delete entry in RendererProcessManager map after promise is resolved
kuychaco 66a1a08
rendererWebContentsId -> childWebContentsID
kuychaco ff425c4
Move operation management from RendererProcessManager into RendererPr…
kuychaco bcd2722
WIP Implement logic for creating new renderer process
kuychaco 8bfa4ae
Merge branch 'master' into ku-renderer-shell-out
kuychaco 4557f46
Refactor RendererProcessManager -> WorkerManager. Handle destroyed pr…
kuychaco 097714d
Set operation to pending after call to GitProcess.exec
kuychaco ce2bcc3
Wait for all write operations to finish before destroying renderer pr…
kuychaco 8e211a1
Add test for fallback when worker process crashes
kuychaco e43c271
Destroy RendererProcess instance on 'crashed' event
kuychaco 6505e04
Add test for handling when a worker is sick
kuychaco 23026e2
Execute remaining operations in newly active worker when sick process…
kuychaco 7b29cc7
Use global sinon
kuychaco 045c009
Destroy worker managers in tests
kuychaco 1038161
Don't care if operation is a write
kuychaco 29c36bb
Add test for ensuring that operations are complete before destroying …
kuychaco cd71d68
Rename renderer.js to worker.js
kuychaco 77fdc0d
Add test case for killing renderer processes when worker manager crashes
kuychaco 0bb8e57
Show browser windows if process.env.ATOM_GITHUB_SHOW_RENDERER_WINDOW …
kuychaco d11877e
Don't create new worker if WorkerManager is destroyed
kuychaco 80956e2
Don't open dev tools
kuychaco 94e0b8b
:art: move destroy method to bottom
kuychaco 53bf504
:art:
kuychaco 5db1010
Merge remote-tracking branch 'origin/master' into ku-renderer-shell-out
920b4ca
:fire: .only
91908e5
:fire: error when pushing into a disposed AsyncQueue
d868091
Fix failure to clone when target folder doesn't exist
c991157
Let the worker window tell us what it's doing
5743cb3
Destroy the old WorkerManager before creating a new one.
dca043e
Stub Operation.prototype.execute instead of complete
kuychaco fc16dd5
Throw error in isProcessAlive helper if pid is not a number
kuychaco a0df97c
Decouple completing operation and post-completion cleanup tasks
kuychaco 652459d
Use static channelName for all ipc communication
kuychaco 7e7d4ce
Dispose of webContents listeners before destroying manager window
kuychaco aa09a96
Fix test
kuychaco 863209d
Fix other test
kuychaco bc3073c
Merge branch 'ku-renderer-shell-out' of github.com:atom/github into k…
kuychaco 55f9882
Include sourceWebContentsId in git data ipc message
kuychaco 6502cbf
Don't show browser window
kuychaco c3b875e
Don't reject promise for remaining operations during destroy
kuychaco b7f4b20
:fire: RendererProcessManager
kuychaco 5e34e0e
Force reset of WorkerManager after tests complete
kuychaco 6359895
Implement WorkerManager.prototype.isReady
kuychaco c39aaf6
Shell out in process if WorkerManager instance isn't ready
kuychaco 95fb731
Don't await ready promise in RendererProcess#executeOperation
kuychaco 7a95446
First hacky pass at IPC-based Git timings
5d309d1
:shirt:
kuychaco 3066270
Don't clog up IPC channel with 'exec-started' events
kuychaco 4084aec
Destroy WorkerManager in deactivate
kuychaco 8727017
Add console.warn to indicate sick worker
kuychaco 7bab3da
:keyboard:
kuychaco 1740e9b
Use assert.lengthOf
kuychaco c3b4b23
Merge branch 'master' into ku-renderer-shell-out
kuychaco e708668
Only log about sick worker if not in spec mode
kuychaco File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| 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(); | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| <!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! This renderer window is used by the GitHub pacakge to execute Git commands in the background. | ||
| </p> | ||
| </body> | ||
| </html> |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
%cworks inconsole.log()was a dangerous day.