Skip to content

Inspector proxy: catch and report errors in device message handling#43697

Closed
robhogan wants to merge 2 commits into
facebook:mainfrom
robhogan:export-D55482735
Closed

Inspector proxy: catch and report errors in device message handling#43697
robhogan wants to merge 2 commits into
facebook:mainfrom
robhogan:export-D55482735

Conversation

@robhogan
Copy link
Copy Markdown
Contributor

Summary:
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.

Differential Revision: D55482735

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Mar 28, 2024
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D55482735

@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D55482735

…book#43686)

Summary:

Under Node 20, the use of `new Buffer(string)` is deprecated and logs a warning. This replaces it with the recommended `Buffer.from(string)`.

Changelog: 
[General][Fixed] FIx "Buffer() is deprecated" warning from debugger proxy.

Reviewed By: huntie

Differential Revision: D55472025
…acebook#43697)

Summary:

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.

Differential Revision: D55482735
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request was exported from Phabricator. Differential Revision: D55482735

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Mar 30, 2024
@facebook-github-bot
Copy link
Copy Markdown
Contributor

This pull request has been merged in e05319c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants