Skip to content

Commit

Permalink
Inspector proxy: catch and report errors in device message handling (#…
Browse files Browse the repository at this point in the history
…43697)

Summary:
Pull Request resolved: #43697

Currently, messages from a device are handled by async handlers added to a promise chain.

If a handler rejects, the end of the chain becomes a rejected promise, picked up only asynchronously by Metro's global `unhandledRejection` handler.

This triggers a warning from Node.js, and worse, prevents any `then()` callback chained by subsequent messages from being invoked at all.

Handlers *should* attempt to gracefully deal with errors (as we do with source map fetching errors, for example), but this diff adds a catch-all fallback for anything we might've missed (in this case, a frontend socket disconnecting while we're busy fetching a source map). Errors are caught and logged to EventReporter.

**To follow**: Gracefully handle socket disconnections while an async handler is working or queued.

Changelog:
[General][Fixed] Inspector proxy: prevent errors proxying a device message from blocking the handler queue or spamming logs.

Reviewed By: EdmondChuiHW

Differential Revision: D55482735

fbshipit-source-id: bb726218495e105f9cb4f723a1d110c9815abdef
  • Loading branch information
robhogan authored and facebook-github-bot committed Mar 30, 2024
1 parent 04bf8cf commit e05319c
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 4 deletions.
22 changes: 18 additions & 4 deletions packages/dev-middleware/src/inspector-proxy/Device.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ export default class Device {

// $FlowFixMe[incompatible-call]
this.#deviceSocket.on('message', (message: string) => {
this.#messageFromDeviceQueue = this.#messageFromDeviceQueue.then(
async () => {
this.#messageFromDeviceQueue = this.#messageFromDeviceQueue
.then(async () => {
const parsedMessage = JSON.parse(message);
if (parsedMessage.event === 'getPages') {
// There's a 'getPages' message every second, so only show them if they change
Expand All @@ -156,8 +156,22 @@ export default class Device {
debug('(Debugger) (Proxy) <- (Device): ' + message);
}
await this.#handleMessageFromDevice(parsedMessage);
},
);
})
.catch(error => {
debug('%O\nHandling device message: %s', error, message);
try {
this.#deviceEventReporter?.logProxyMessageHandlingError(
'device',
error,
message,
);
} catch (loggingError) {
debug(
'Error logging message handling error to reporter: %O',
loggingError,
);
}
});
});
// Sends 'getPages' request to device every PAGES_POLLING_INTERVAL milliseconds.
this.#pagesPollingIntervalId = setInterval(
Expand Down
19 changes: 19 additions & 0 deletions packages/dev-middleware/src/inspector-proxy/DeviceEventReporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,25 @@ class DeviceEventReporter {
this.#pendingCommands.clear();
}

logProxyMessageHandlingError(
messageOrigin: 'device' | 'debugger',
error: Error,
message: string,
): void {
this.#eventReporter.logEvent({
type: 'proxy_error',
status: 'error',
messageOrigin,
message,
error: error.message,
errorStack: error.stack,
appId: this.#metadata.appId,
deviceId: this.#metadata.deviceId,
deviceName: this.#metadata.deviceName,
pageId: null,
});
}

#logExpiredCommand(pendingCommand: PendingCommand): void {
this.#eventReporter.logEvent({
type: 'debugger_command',
Expand Down
9 changes: 9 additions & 0 deletions packages/dev-middleware/src/types/EventReporter.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ export type ReportableEvent =
| 'UNMATCHED_REQUEST_ID'
| 'PROTOCOL_ERROR',
>,
}
| {
type: 'proxy_error',
status: 'error',
messageOrigin: 'debugger' | 'device',
message: string,
error: string,
errorStack: string,
...DebuggerSessionIDs,
};

/**
Expand Down

0 comments on commit e05319c

Please sign in to comment.