-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Guard against launching multiple Chrome processes #1436
Guard against launching multiple Chrome processes #1436
Conversation
LGTM 😄 just a question when did you have this issue? ^^ |
@wardpeet Not in lighthouse. Subclassing this ChromeLauncher in my own code. Got this issue because I assumed |
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.
LGTM3. One small suggestion; feel free to weigh in either way on it and we can merge
@@ -136,6 +136,11 @@ class ChromeLauncher { | |||
|
|||
spawn(execPath: string): Promise<any[]> { | |||
return new Promise((resolve, reject) => { | |||
if (this.chrome) { | |||
log.verbose('ChromeLauncher', `Chrome already running with pid ${this.chrome.pid}.`); |
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.
maybe use log.warn
? Seems like an unusual situation that in most cases you'd want to know your use of launcher is trying to launch chrome twice
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 tradeoff being that it adds warning noise to a use case like your own. A compromise might be just log.log
)
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.
It's all the same for me, I just took whatever was underneath. Just let me know what you prefer
@brendankenny Changed to |
* Guard against launching multiple Chrome processes * Change verbosity
No description provided.