core(driver): add timeout to getRequestContent #4718
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
patrickhulce
reviewed
Mar 9, 2018
lighthouse-core/gather/driver.js
Outdated
| return new Promise((resolve, reject) => { | ||
| // If this takes more than 2s, reject the Promise. | ||
| const err = new Error('Fetching resource content has exceeded the allotted time of 2s'); | ||
| const asyncTimeout = setTimeout((_ => reject(err)), 2000); |
There was a problem hiding this comment.
I'd be cool going down to 1s too to keep things moving
patrickhulce
reviewed
Mar 9, 2018
lighthouse-core/gather/driver.js
Outdated
| }).then(result => result.body); | ||
| return new Promise((resolve, reject) => { | ||
| // If this takes more than 2s, reject the Promise. | ||
| const err = new Error('Fetching resource content has exceeded the allotted time of 2s'); |
There was a problem hiding this comment.
should we try to give this thing an error code and be consistent about timeout stuff moving forward?
brendankenny
approved these changes
Mar 13, 2018
lighthouse-core/gather/driver.js
Outdated
| // Ignoring result.base64Encoded, which indicates if body is already encoded | ||
| }).then(result => result.body); | ||
| return new Promise((resolve, reject) => { | ||
| // If this takes more than 2s, reject the Promise. |
There was a problem hiding this comment.
maybe add a note with link back to your investigation for why this method in particular can be problematic?
paulirish
force-pushed
the
scripts-retriving-timeout
branch
from
ae6ef54
to
85ee3a7
Compare
patrickhulce
approved these changes
Mar 13, 2018
brendankenny
approved these changes
Mar 13, 2018
| return driverStub.getRequestContent(0, MAX_WAIT_FOR_PROTOCOL).then(_ => { | ||
| assert.ok(false, 'long-running getRequestContent supposed to reject'); | ||
| }).catch(e => { | ||
| assert.equal(e.code, 'REQUEST_CONTENT_TIMEOUT'); |
There was a problem hiding this comment.
will the above assert message get eaten by this comparison? could also do the two function then()
return driverStub.getRequestContent(0).then(value => {
assert.ok(false, 'long-running getRequestContent supposed to reject');
}, e => {
assert.equal(e.code, 'REQUEST_CONTENT_TIMEOUT');
});
lighthouse-core/gather/driver.js
Outdated
| @@ -714,13 +715,25 @@ class Driver { | |||
| /** | |||
| * Return the body of the response with the given ID. | |||
| * @param {string} requestId | |||
| * @param {?number} timeout | |||
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.
fixes #4258
I was able to repro on all the URLs mentioned in the ticket:
Network.getResponseBodyis taking >5s to respond when provided particular requestIds. But it's quite odd. If i have devtools open at the same time, the devtools frontend can issue the exact same protocol method with the same requestId and it has no problem. (and apparently devtools and CLI also have no problem)So this is particular to the extension.. sort of.
Update: the root bug appears to be an encoding issue in how the extension handles
chrome.debuggerpayloads. Below is a bunch of details that supports that argument:here's the 4 scripts that will reliably cause a problem:
I believe these files all have some characters in common that are mishandled somewhere in encoding.
I ran this across these 4 files:
And they all have in common some fairly high codepoints:
They all have in common 65534 or 65535.
And... check this out. Run this in any DevTools console:
Now take that resulting string, copy/paste it back and try to evaluate it. DevTools won't. :)
So, since this appears to be rather an involved bug. My easy workaround is to add a 2s timeout.
I did observe a 300ms successful response time for this method on a fairly big file so 2000ms seems an OK maximum.