fix(connection): gracefully handle missing method #2351
Conversation
Yeah using |
|
did anyone have any more insight into the cause of this? We don't want to crash in the faces of this, but without error tracking this will start to get lost with this change. The devtools case is more understandable, but feedback commands we didn't send is something we should probably track down |
Nope :) it seems impossible from the stack trace in the issue since if it fails in the resolve it should have definitely failed the first time when it was sending the message (because it actually gets the method when the callback was set)
Good thing we're approved for sentry now let's get it installed post-branch :D |
| @@ -92,7 +92,7 @@ class Connection { | |||
| const object = JSON.parse(message); | |||
| // Remote debugging protocol is JSON RPC 2.0 compiant. In terms of that transport, | |||
| // responses to the commands carry "id" property, while notifications do not. | |||
| if (object.id) { | |||
| if (this._callbacks.has(object.id)) { | |||
There was a problem hiding this comment.
what do you think about making this more explicit than treating it like an event if no callback (it's not really an event since the message does have an id)? Could do a third case with a call to formatProtocol with an explicit ????? (and no emit).
Also with level error so it hits log.error? Not sure about that one if this is a regular occurrence in devtools land
file an issue in that case? We don't want to forget that we're silencing this |
|
@brendankenny filed #2364 |
| @@ -106,11 +106,15 @@ class Connection { | |||
| {method: callback.method, params: object.result}, 'verbose'); | |||
| return object.result; | |||
| })); | |||
| } else if (object.id) { | |||
| const error = object.error && object.error.message; | |||
There was a problem hiding this comment.
add a comment about why this might happen (pavel's old "while notifications do not" comment above probably should also be updated as it implies only two states)
| } else if (object.id) { | ||
| const error = object.error && object.error.message; | ||
| log.formatProtocol(`disowned method <= browser ${error ? 'ERR' : 'OK'}`, | ||
| {method: object.method, params: error || object.result}, 'verbose'); |
There was a problem hiding this comment.
normalize object.method here rather than in formatProtocol?
There was a problem hiding this comment.
if it's an error, params probably shouldn't be set to the message?
There was a problem hiding this comment.
and should it still throw if there's an error?
There was a problem hiding this comment.
normalize object.method here rather than in formatProtocol?
I'd really rather not, that's exactly what got us into this mess of fatally erroring on unexpected input, unless I'm misunderstanding what you mean by "normalize" haha
if it's an error, params probably shouldn't be set to the message?
where would you like to log the message then?
and should it still throw if there's an error?
that would make these fatal in the future which seems bad. the point is that we don't care about these "disowned" (better name pending you 😄 ) events
There was a problem hiding this comment.
normalize object.method here rather than in formatProtocol?
I'd really rather not, that's exactly what got us into this mess of fatally erroring on unexpected input, unless I'm misunderstanding what you mean by "normalize"
I mean const method = object.method || '?????'; here instead of in formatProtocol just for the logging, not for how it's handled.
where would you like to log the message then?
For better or worse, formatProtocol expects a data.method and data.params, so it's better to be strict in sending that than to just let it support whatever. formatProtocol's existence now is kind of dumb (all it's really doing beyond log.verbose is clipping to the end of the line), but rather than eliminating, maybe we can just quickly revise the parameters rather than overload them?
that would make these fatal in the future which seems bad
yeah, makes sense.
| @@ -106,11 +106,15 @@ class Connection { | |||
| {method: callback.method, params: object.result}, 'verbose'); | |||
| return object.result; | |||
| })); | |||
| } else if (object.id) { | |||
| const error = object.error && object.error.message; | |||
| log.formatProtocol(`disowned method <= browser ${error ? 'ERR' : 'OK'}`, | |||
There was a problem hiding this comment.
disowned may not be a good word here (sounds like LH did send the command but doesn't care about it now :), but not sure of a good replacement.
fixes #2321
also observed errors in devtools when we get command callbacks for commands LH didn't send