Skip to content
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

Use async randomBytes API to speedup Windows startup #19242

Merged
merged 2 commits into from May 1, 2019

Conversation

Projects
None yet
4 participants
@rafeca
Copy link
Contributor

commented Apr 30, 2019

This PR changes a bit the logic of atom-application to make the authentication through sockets async, which fixes #19239.

In order to make the code async and keep it as robust as possible a few changes needed to be done (basically to delay the execution of some logic while avoiding as much as possible potential race conditions).

@rafeca rafeca force-pushed the use-async-randombytes branch from b14fab2 to 6829557 Apr 30, 2019

@rafeca rafeca force-pushed the use-async-randombytes branch from 6829557 to 3fcdcde Apr 30, 2019

Avoid using randomBytes() when encrypting options
This way we avoid delaying the opening of a project when reusing an
existing Atom window.
@smashwilson
Copy link
Member

left a comment

👍 LGTM

@@ -15,6 +15,7 @@ const path = require('path')
const os = require('os')
const net = require('net')
const url = require('url')
const {promisify} = require('util')

This comment has been minimized.

Copy link
@smashwilson

smashwilson May 1, 2019

Member

Oh hey! I didn't realize we had access to promisify now. This will come in handy.

@@ -227,11 +230,16 @@ class AtomApplication extends EventEmitter {
this.applicationMenu = new ApplicationMenu(this.version, this.autoUpdateManager)
this.atomProtocolHandler = new AtomProtocolHandler(this.resourcePath, this.safeMode)

this.listenForArgumentsFromNewProcess()
// Don't await for the following method to avoid delaying the opening of a new window.
// (we await it just after opening it).

This comment has been minimized.

Copy link
@smashwilson

smashwilson May 1, 2019

Member

👍 Excellent.

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 1, 2019

Contributor

Again, worth mentioning explicitly that this method is slow on Windows?

listenForArgumentsFromNewProcess () {
if (!this.socketPath) return
async listenForArgumentsFromNewProcess (options) {
if (!options.test && !options.benchmark && !options.benchmarkTest) {

This comment has been minimized.

Copy link
@smashwilson

smashwilson May 1, 2019

Member

I still wish we had a more succinct way to express "a regular editor window" in these options. (Out of scope for this PR though.)

@rafeca rafeca merged commit 0132e1e into master May 1, 2019

2 checks passed

Atom Pull Requests #20190430.5 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@rafeca rafeca deleted the use-async-randombytes branch May 1, 2019

@nathansobo
Copy link
Contributor

left a comment

I think it might be helpful to clarify the motivations behind not awaiting in comments. So just checking my understanding... this increases the window of time in which a race condition might exist, correct? I suppose there was always a small window anyway if you ran atom twice quickly before the first Electron could start listening on the socket. This just makes it more likely.

Another question: Is there some way we can remember to revisit this when we upgrade Electron and see if crypto is still so slow?


return socketSecret
}

const encryptOptions = (options, secret) => {
const message = JSON.stringify(options)

const initVector = crypto.randomBytes(16)
// Even if the following IV is not cryptographically secure, there's a really good chance
// it's going to be unique between executions which is the requirement for GCM.

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 1, 2019

Contributor

I think it might be helpful to future readers if you explain the motivation for avoiding cryptographic randomness on this code path (that it's too slow).

This comment has been minimized.

Copy link
@rafeca

rafeca May 2, 2019

Author Contributor

I've added more explicit comments for both things in #19246

@@ -227,11 +230,16 @@ class AtomApplication extends EventEmitter {
this.applicationMenu = new ApplicationMenu(this.version, this.autoUpdateManager)
this.atomProtocolHandler = new AtomProtocolHandler(this.resourcePath, this.safeMode)

this.listenForArgumentsFromNewProcess()
// Don't await for the following method to avoid delaying the opening of a new window.
// (we await it just after opening it).

This comment has been minimized.

Copy link
@nathansobo

nathansobo May 1, 2019

Contributor

Again, worth mentioning explicitly that this method is slow on Windows?

@rafeca

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

I think it might be helpful to clarify the motivations behind not awaiting in comments. So just checking my understanding... this increases the window of time in which a race condition might exist, correct? I suppose there was always a small window anyway if you ran atom twice quickly before the first Electron could start listening on the socket. This just makes it more likely.

Exactly, it'll make this window (which is already a few hundred ms) unnoticeable longer in OSX/Linux (a few ticks) and quite much longer (up to a second) in Windows (at least until we upgrade to electron v3).

I think that's a fair tradeoff, since the only side effect that this will cause is that two Atom windows will get opened under that situation.

Another question: Is there some way we can remember to revisit this when we upgrade Electron and see if crypto is still so slow?

Maybe we can use a specially-crafted comment that's easy to search on GitHub for the whole Atom org, and then we add a point to the electron upgrade manual doc (if it exists) to revisit all these messages whenever we upgrade the electron version?

Something like:

// TodoElectronIssue: `randomBytes` is slow on electron v2. This issue is fixed in v3.

(I'm not using spaces/special characters since GitHub search is particularly bad when searching for non-words).

@50Wliu

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Another question: Is there some way we can remember to revisit this when we upgrade Electron and see if crypto is still so slow?

In addition to what @rafeca suggested, we could also create a tracking issue for it and apply the electron label, as our Electron upgrade checklist specifies to check each one and see if they have been fixed/can be resolved with the new Electron version.

rafeca added a commit that referenced this pull request May 7, 2019

Merge pull request #19242 from atom/use-async-randombytes
Use async randomBytes API to speedup Windows startup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.