Launcher: add timeout to chrome launching #434
Merged
Conversation
This file contains 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
lib/Launcher.js
Outdated
|
|
||
| function onTimeout() { | ||
| cleanup(); | ||
| reject(new Error('Failed to connect to chrome! The only chrome revision guaranteed to work is r' + ChromiumRevision)); |
There was a problem hiding this comment.
Can we mention that it failed because of the timeout?
lib/Launcher.js
Outdated
| helper.addEventListener(rl, 'close', onClose), | ||
| helper.addEventListener(chromeProcess, 'exit', onClose) | ||
| ]; | ||
| let timeout = setTimeout(onTimeout, 30 * 1000); |
There was a problem hiding this comment.
I think we should make this optional. I can imagine some crazy cloud setup where 30 seconds to connect might not be unreasonable.
There was a problem hiding this comment.
Done. Introduced "timeout" option to launching
This patch: - adds 30s timeout for chromium to report the browser target url. - adds cleanup logic for main process SIGINT signal. Fixes puppeteer#363.
aslushnikov
force-pushed
the
timeout-launch
branch
from
bf9f523 to
33513c6
Compare
|
@JoelEinbinder addressed comments, please take another look! |
docs/api.md
Outdated
| @@ -158,6 +158,8 @@ This methods attaches Puppeteer to an existing Chromium instance. | |||
| - `executablePath` <[string]> Path to a Chromium executable to run instead of bundled Chromium. If `executablePath` is a relative path, then it is resolved relative to [current working directory](https://nodejs.org/api/process.html#process_process_cwd). | |||
| - `slowMo` <[number]> Slows down Puppeteer operations by the specified amount of milliseconds. Useful so that you can see what is going on. | |||
| - `args` <[Array]<[string]>> Additional arguments to pass to the Chromium instance. List of Chromium flags can be found [here](http://peter.sh/experiments/chromium-command-line-switches/). | |||
| - `handleSIGINT` <[boolean]> Close chrome process on Ctrl-C. Defaults to `true`. | |||
| - `timeout` <[number]> Maximum time in milliseconds to wait for chrome instance to start. Defaults to `30000` (30 seconds). Pass `0` to disable timeout. | |||
lib/Launcher.js
Outdated
|
|
||
| function onTimeout() { | ||
| cleanup(); | ||
| reject(new Error(`Timed out after ${timeout} ms while trying to connect to chrome! The only chrome revision guaranteed to work is r${ChromiumRevision}`)); |
JoelEinbinder
approved these changes
Aug 21, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This patch:
Fixes #363.