create single path for handling errors from the protocol #977

brendankenny · 2016-11-19T01:44:57Z

starts addressing #976
also gives slightly more context for errors like #966 (as well as a real stack trace)

Moves the extension and the raw connection to use the same error handler for messages coming back from the debugging protocol, as well as normalizing the error object fed into it. This should aid debugging. Happens to unify catching of DOM.disable error, instead of in separate spots for extension and CLI.

Also removes old result.wasThrown from extension sendCommand, which as discussed in #976 was doing nothing but confusing us.

brendankenny · 2016-11-19T01:46:18Z

to be clear, this does not handle the main part of #976, dealing with exceptionDetails, but a simpler path for protocol errors (as opposed to errors from e.g. code running in the browser via the protocol) should make that easier

patrickhulce · 2016-11-19T01:56:02Z

lighthouse-core/gather/connections/extension.js

+          error = error || {message: 'Unknown debugger protocol error.'};
+
+          const callback = {
+            resolve,


the use of new Promise now feels pretty weird here since we're basically passing both resolve and reject to another function, thoughts on Promise.try or the stop-gap Promise.resolve().then instead and just having handleRawError return its own promise?

ah, I like it. This is kind of awkward because of the raw connection setup, but that should work for both

I haven't run into (too many) issues using the Promise.resolve().then pattern Patrick suggested above, so a 👍 to switching over to that and letting handleRawError do it's bizness.

ah, I wasn't going to do that part :) I'll consider it, but we use resolve(result); farther down in that function. It is a simplistic use of a promise, but I don't really mind resolve vs return, and exceptions are treated the same way. I'm sure others feel otherwise :)

incidentally, this is one more reason we need to always avoid Promise.alling any debugger protocol code since any refactor like this might introduce ticks between steps, even in driver functions that only run a single command :)

ok, the more I think about it, the more I like this pattern. I had avoided it in the past just because the `Promise.resolve() part feels like a waste (and the extra tick from it can be an issue for reasoning about things running in parallel), but the consistency in code is pretty nice. I'd be in favor of adopting this pattern in the future for functions returning a promise but also needing to swallow sync exceptions.

We need to be careful converting old code, though, because I know there are a few places we've written code explicitly assuming the executor will be executed synchronously (which is actually one more point in the consistency column if we always assume this pattern)

I'd be in favor of adopting this pattern in the future for functions returning a promise but also needing to swallow sync exceptions.

👏

patrickhulce · 2016-11-19T01:56:56Z

lighthouse-core/gather/connections/extension.js

+          const message = chrome.runtime.lastError.message;
+          let error;
+          try {
+            error = JSON.parse(message);


more just an aside here, i've had to do this a couple times in the past too and always thought there had to be a better way i was missing, i guess not? haha

ha, yeah. The docs on lastError are really unhelpful. Not only is message marked optional, there's no hint of what the format of the error is, it's just always JSON when I try it...

brendankenny · 2016-11-21T21:12:06Z

updated, PTAL
@addyosmani @patrickhulce

patrickhulce · 2016-11-21T21:51:29Z

lighthouse-core/gather/connections/extension.js

+          error = error || {message: 'Unknown debugger protocol error.'};
+
+          // handleRawError returns or throws synchronously, so try/catch awkwardly.
+          try {


I'm guessing chrome.debugger.sendCommand doesn't return a promise? otherwise we could follow the earlier pattern

no, only callback

ebidel

LGTM with the suggestions

ebidel · 2016-11-22T23:44:20Z

lighthouse-core/gather/connections/connection.js

+   * about and throwing on the rest.
+   * @param {{message: string}} error
+   * @param {string} method Protocol method that received the error response.
+   * @protected


Should this stay underscored if it's meant to be @protected?

Add a @throws?

no, no underscore for @protected (not that closure is looking at this, really only annotating to keep in the style of the rest of the file)

Add a @throws

done

ebidel · 2016-11-22T23:45:51Z

lighthouse-core/gather/connections/connection.js

-   * @param {{error: {message: string}}} object
-   * @param {{reject: function(*), method: string}} callback
-   * @private
+   * Handles error responses from the protocol, absorbing errors we don't care


absorbing errors we don't care...

it's only the DOM.disable atm.

addyosmani · 2016-11-23T00:17:36Z

lighthouse-core/gather/connections/extension.js

+            return resolve(this.handleRawError(error, command));
+          } catch (err) {
+            return reject(err);
+          }
        }


This pattern is a little awkward (as noted) but I think I prefer this flavor to what we had here before.

addyosmani · 2016-11-23T00:19:30Z

Waiting for travis to complete a run on your last set of commits, but looks like it's all good to go.

…e#977) * create single path for handling errors from the protocol * make handleRawError synchronous

patrickhulce reviewed Nov 19, 2016

View reviewed changes

ebidel mentioned this pull request Nov 20, 2016

Lighthouse Extension Error #966

Closed

brendankenny force-pushed the protocolerrors branch from d2d3dc7 to 0c549d0 Compare November 21, 2016 21:12

patrickhulce reviewed Nov 21, 2016

View reviewed changes

brendankenny force-pushed the protocolerrors branch from 0c549d0 to 835ce37 Compare November 21, 2016 23:39

create single path for handling errors from the protocol

d5ac9db

brendankenny force-pushed the protocolerrors branch from 835ce37 to c8d7556 Compare November 22, 2016 20:21

brendankenny mentioned this pull request Nov 22, 2016

evaluateAsync doesn't return rejected promises properly #976

Closed

ebidel approved these changes Nov 22, 2016

View reviewed changes

make handleRawError synchronous

41e9354

brendankenny force-pushed the protocolerrors branch from c8d7556 to 41e9354 Compare November 23, 2016 00:14

addyosmani reviewed Nov 23, 2016

View reviewed changes

addyosmani approved these changes Nov 23, 2016

View reviewed changes

addyosmani merged commit 74a7665 into master Nov 23, 2016

brendankenny deleted the protocolerrors branch November 23, 2016 00:56

andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017

create single path for handling errors from the protocol (GoogleChrom…

a80c243

…e#977) * create single path for handling errors from the protocol * make handleRawError synchronous

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create single path for handling errors from the protocol #977

create single path for handling errors from the protocol #977

brendankenny commented Nov 19, 2016

brendankenny commented Nov 19, 2016 •

edited

Loading

patrickhulce Nov 19, 2016

brendankenny Nov 19, 2016

addyosmani Nov 19, 2016

brendankenny Nov 19, 2016

brendankenny Nov 19, 2016

brendankenny Nov 20, 2016

addyosmani Nov 21, 2016

patrickhulce Nov 19, 2016

brendankenny Nov 19, 2016

brendankenny commented Nov 21, 2016

patrickhulce Nov 21, 2016

brendankenny Nov 21, 2016

ebidel left a comment

ebidel Nov 22, 2016

ebidel Nov 22, 2016

brendankenny Nov 23, 2016

brendankenny Nov 23, 2016

ebidel Nov 22, 2016

brendankenny Nov 23, 2016

addyosmani Nov 23, 2016

addyosmani commented Nov 23, 2016

create single path for handling errors from the protocol #977

create single path for handling errors from the protocol #977

Conversation

brendankenny commented Nov 19, 2016

brendankenny commented Nov 19, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel 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

Choose a reason for hiding this comment

addyosmani commented Nov 23, 2016

brendankenny commented Nov 19, 2016 •

edited

Loading