core: remove protocol timeout for Page.navigate #6413

connorjclark · 2018-10-27T00:17:53Z

~~i think 60s will be better~~

patrickhulce · 2018-10-27T01:24:48Z

lighthouse-core/gather/driver.js

@@ -840,7 +840,7 @@ class Driver {
    // happen _after_ onload: https://crbug.com/768961
    this.sendCommand('Page.enable');
    this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS});
-    this.setNextProtocolTimeout(30 * 1000);
+    this.setNextProtocolTimeout(60 * 1000); // see https://github.com/GoogleChrome/lighthouse/pull/6413


WDYT about @brendankenny's suggestion of ignoring the timeout, we don't wait or care about the result of this command at all so seems a bit silly to need to control a timeout for it

sg. i turned it all the way to 90s on lightrider.

might as well use that here, too.

paulirish

lgtm % tweak

paulirish · 2018-10-29T21:28:22Z

lighthouse-core/gather/driver.js

@@ -840,7 +840,7 @@ class Driver {
    // happen _after_ onload: https://crbug.com/768961
    this.sendCommand('Page.enable');
    this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS});
-    this.setNextProtocolTimeout(30 * 1000);
+    this.setNextProtocolTimeout(60 * 1000); // see https://github.com/GoogleChrome/lighthouse/pull/6413


sg. i turned it all the way to 90s on lightrider.

might as well use that here, too.

lighthouse-core/gather/driver.js

brendankenny

LGTM if we need to land this, but agree with @patrickhulce's comment that if we're purposefully disabling the timeout we should just do so explicitly

connorjclark · 2018-10-29T22:12:22Z

OK, I've disabled the timeout completely.

exterkamp

LGTM.

brendankenny · 2018-10-30T00:09:58Z

lighthouse-core/gather/driver.js

@@ -286,18 +286,19 @@ class Driver {
      }
    }
    return new Promise(async (resolve, reject) => {
-      const asyncTimeout = setTimeout((_ => {
+      const asyncTimeout = timeout > 0 ? setTimeout((_ => {


nit: the ternary ends up stretching pretty far. What about just

let asyncTimeout; if (timeout > 0) { asyncTimeout = setTimeout(...) }

should also add a note about the 0 behavior to setNextProtocolTimeout

I would have gone that route first, but it would need to be let, and my personal preferences led me to choose the ternary before reaching for let. wdyt

whoops, yeah, let. And what's with that explicit undefined. Going to edit my comment because my brain was apparently off while writing that :)

The main issue is that you have to look down four lines to see the other half of the conditional, and the syntax makes it harder to parse than normal control flow mechanisms (like an if block). Generally long ternaries just aren't worth it, but at the very least, the null branch should be first

brendankenny · 2018-10-30T00:25:43Z

lighthouse-core/gather/driver.js

-        const result = await this._connection.sendCommand(method, ...params);
-        clearTimeout(asyncTimeout);
-        resolve(result);
+        resolve(await this._connection.sendCommand(method, ...params));


result is about as generic as you can get, but it does end up separating a bit of this (e.g. the await is significant if we want the timeout to work correctly. So I'd still be in favor of setting await this._connection.sendCommand to an intermediate result variable before resolving on it.

brendankenny · 2018-10-30T00:26:07Z

lighthouse-core/gather/driver.js

@@ -840,7 +841,7 @@ class Driver {
    // happen _after_ onload: https://crbug.com/768961
    this.sendCommand('Page.enable');
    this.sendCommand('Emulation.setScriptExecutionDisabled', {value: disableJS});
-    this.setNextProtocolTimeout(30 * 1000);
+    this.setNextProtocolTimeout(0); // We don't need a strict timeout for Page.navigate. See #6413.


nit: drop the strict here now that we don't have any at all? :)

brendankenny · 2018-10-30T00:58:50Z

lighthouse-core/gather/driver.js

@@ -259,6 +259,8 @@ class Driver {
  }

  /**
+   * If > 0, timeout is used for the next call to 'sendCommand'.
+   * If <= 0, no timeout is used.


The >/<= describe literally what's allowed, but probably aren't worth including. Maybe go with timeout is used for the next call to 'sendCommand'. If 0, no timeout is used.

brendankenny · 2018-10-30T01:06:10Z

lighthouse-core/gather/driver.js

@@ -286,18 +286,19 @@ class Driver {
      }
    }
    return new Promise(async (resolve, reject) => {
-      const asyncTimeout = setTimeout((_ => {
+      const asyncTimeout = timeout > 0 ? setTimeout((_ => {


whoops, yeah, let. And what's with that explicit undefined. Going to edit my comment because my brain was apparently off while writing that :)

The main issue is that you have to look down four lines to see the other half of the conditional, and the syntax makes it harder to parse than normal control flow mechanisms (like an if block). Generally long ternaries just aren't worth it, but at the very least, the null branch should be first

brendankenny

LGTM.

Seems like we do still want this?

paulirish · 2018-11-19T22:44:42Z

lighthouse-core/gather/driver.js

-        reject(err);
-      }), timeout);
+      let asyncTimeout;
+      if (timeout > 0) {


Taking another look at this.
I'm a bit hesitant to absorb this complexity.

What about adding a driver.innerSendCommand() which does the essence of sendCommand
And for the Page.Navigate command we use that.

paulirish

coolness thx

paulirish · 2018-11-20T00:21:40Z

@brendankenny @patrickhulce @exterkamp lgty?

brendankenny

I realize I this is something new (and it really should have been part of #6347), but looking at it now, it would be easy to throw in a test or two for timeout + sendCommand.

It should be just a mock Connection with a sendCommand that takes 10ms or whatever, call setNextProtocolTimeout(5), and make sure it throws (and the opposite timings don't). Not sure if we need any tests more complicated than that.

brendankenny · 2018-11-20T00:28:12Z

lighthouse-core/gather/driver.js

+   * @param {LH.CrdpCommands[C]['paramsType']} params
+   * @return {Promise<LH.CrdpCommands[C]['returnType']>}
+   */
+  innerSendCommand(method, ...params) {


Maybe add a leading _ and mark it with @private (someday tsc will support it) to make it super clear it's intended for internal use

patrickhulce

LGTM other than test! :)

patrickhulce · 2018-11-20T02:06:30Z

lighthouse-core/test/gather/driver-test.js

+      await driver.sendCommand('Page.disable');
+      assert.fail('expected driver.sendCommand to timeout');
+    } catch (err) {
+      // :)


well here we want to assert that we didn't fail with the assert.fail you added :)

let's make sure that the message matches roughly what we want it to .includes('PROTOCOL_TIMEOUT') maybe?

looks correct to me - if this command times out, the next statement in the try block should never be reached.

added check for err.code

looks correct to me - if this command times out, the next statement in the try block should never be reached.

If the command times out, it would've thrown. If it doesn't time out as expected, it would've also thrown on the following line with the fail. Both paths led to an empty catch block. New error code assertion looks perfect though!! 👌

Ah, of course. I convinced myself that assert.fail didn't throw an error.. I was applying a pattern that works for other testing frameworks. But totally doesn't work here :p

brendankenny · 2018-11-20T02:24:45Z

lighthouse-core/test/gather/driver-test.js

@@ -240,6 +240,24 @@ describe('Browser Driver', () => {
    });
  });

+  it('.sendCommand timesout when commands take too long', async () => {
+    const driver = new Driver(connection);


rather than overriding a method of Driver, can this mock a Connection instead? That way we're only testing how the public Driver.sendCommand obeys timeouts, regardless of how internals evolve in the future (and only mocking the public interface of Connection)

Something like

const mockConnection = { async connect() {} async disconnect() {} sendCommand() { return new Promise(resolve => setTimeout(resolve, 15)); } }; const driver = new Driver(mockConnection);

should work?

connorjclark requested review from patrickhulce and paulirish as code owners October 27, 2018 00:17

connorjclark changed the title ~~Issue 6407 increase page navigate timeout~~ core: increase page navigate timeout Oct 27, 2018

core: increase Page.navigate timeout (#6412)

d798047

connorjclark force-pushed the issue-6407-increase-page-navigate-timeout branch from a9fabde to d798047 Compare October 27, 2018 00:22

patrickhulce reviewed Oct 27, 2018

View reviewed changes

paulirish approved these changes Oct 29, 2018

View reviewed changes

paulirish reviewed Oct 29, 2018

View reviewed changes

lighthouse-core/gather/driver.js Outdated Show resolved Hide resolved

brendankenny approved these changes Oct 29, 2018

View reviewed changes

disable timeout for Page.navigate

9af14cc

exterkamp approved these changes Oct 29, 2018

View reviewed changes

brendankenny requested changes Oct 30, 2018

View reviewed changes

connorjclark added 2 commits October 29, 2018 17:52

add comment

273166c

pr changes

e0fa246

brendankenny reviewed Oct 30, 2018

View reviewed changes

connorjclark added 2 commits October 31, 2018 14:06

no ternary

7dd175a

edit comment

81c00a1

brendankenny changed the title ~~core: increase page navigate timeout~~ core: remove Page.navigate timeout Nov 19, 2018

brendankenny approved these changes Nov 19, 2018

View reviewed changes

brendankenny changed the title ~~core: remove Page.navigate timeout~~ core: remove timeout for Page.navigate Nov 19, 2018

brendankenny changed the title ~~core: remove timeout for Page.navigate~~ core: remove protocol timeout for Page.navigate Nov 19, 2018

paulirish requested changes Nov 19, 2018

View reviewed changes

connorjclark added 3 commits November 19, 2018 15:58

innerSendCommand

3c58396

update comment

0383285

Merge branch 'master' into issue-6407-increase-page-navigate-timeout

175cc67

paulirish approved these changes Nov 20, 2018

View reviewed changes

brendankenny reviewed Nov 20, 2018

View reviewed changes

add tests

6f88aaf

patrickhulce suggested changes Nov 20, 2018

View reviewed changes

brendankenny reviewed Nov 20, 2018

View reviewed changes

update test

c08cb46

patrickhulce approved these changes Nov 20, 2018

View reviewed changes

brendankenny merged commit e45020b into master Nov 20, 2018

brendankenny deleted the issue-6407-increase-page-navigate-timeout branch November 20, 2018 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: remove protocol timeout for Page.navigate #6413

core: remove protocol timeout for Page.navigate #6413

connorjclark commented Oct 27, 2018 •

edited by brendankenny

Loading

patrickhulce Oct 27, 2018

paulirish Oct 29, 2018

paulirish left a comment

paulirish Oct 29, 2018

brendankenny left a comment

connorjclark commented Oct 29, 2018

exterkamp left a comment

brendankenny Oct 30, 2018 •

edited

Loading

brendankenny Oct 30, 2018

connorjclark Oct 30, 2018

brendankenny Oct 30, 2018

brendankenny Oct 30, 2018

brendankenny Oct 30, 2018

brendankenny Oct 30, 2018 •

edited

Loading

brendankenny Oct 30, 2018

brendankenny left a comment

paulirish Nov 19, 2018

connorjclark Nov 19, 2018

paulirish left a comment

paulirish commented Nov 20, 2018

brendankenny left a comment

brendankenny Nov 20, 2018

patrickhulce left a comment

patrickhulce Nov 20, 2018

connorjclark Nov 20, 2018

patrickhulce Nov 20, 2018

connorjclark Nov 20, 2018

brendankenny Nov 20, 2018

connorjclark Nov 20, 2018

core: remove protocol timeout for Page.navigate #6413

core: remove protocol timeout for Page.navigate #6413

Conversation

connorjclark commented Oct 27, 2018 • edited by brendankenny Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

connorjclark commented Oct 29, 2018

exterkamp left a comment

Choose a reason for hiding this comment

brendankenny Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

paulirish commented Nov 20, 2018

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Oct 27, 2018 •

edited by brendankenny

Loading

brendankenny Oct 30, 2018 •

edited

Loading

brendankenny Oct 30, 2018 •

edited

Loading