Skip to content

Commit

Permalink
Catch DNS errors to avoid unhandled exceptions (#1215)
Browse files Browse the repository at this point in the history
Catches potential DNS errors produced in outbound network requests to
avoid unhandled exceptions.

## What problem is it solving?
Fixes the problem described in
#535.

## How is the problem addressed?
The PR introduces a `try-catch` statement to catch errors produced when
invoking `lookup` to resolve the target host address. When this happens,
it sends empty binary data as a notification that the connection
initiated but soon after, it closes the connection with error code
`3000`.

## Testing Instructions
The [original
issue](#535)
references `wp-now`, so it's recommended to test the changes using a
local build of `wp-now` from the [`playground-tools`
repository](https://github.com/WordPress/playground-tools).

1. In `wordpress-playground` repository, run `nx build php-wasm-node`.
2. Check out [`playground-tools`
repository](https://github.com/WordPress/playground-tools).
3. In `playground-rools` repository, update the `@php-wasm/node`
dependency to point to the local path of the `wordpress-playground`
repository.

**Example:**
```patch
diff --git forkSrcPrefix/package.json forkDstPrefix/package.json
index cc623dc57810509691383fa068cc8f921c957f48..ac84b7f467c2f7d23b9fa8c5f9487cafdb4644e6 100644
--- forkSrcPrefix/package.json
+++ forkDstPrefix/package.json
@@ -32,7 +32,7 @@
 		"@codemirror/state": "6.2.0",
 		"@codemirror/theme-one-dark": "6.1.1",
 		"@codemirror/view": "6.9.3",
-		"@php-wasm/node": "0.6.10",
+		"@php-wasm/node": "file:../wordpress-playground/dist/packages/php-wasm/node",
 		"@php-wasm/progress": "0.6.10",
 		"@php-wasm/universal": "0.6.10",
 		"@php-wasm/web": "0.6.10",

```
4. Run `npm i`.
5. Run `nx build wp-now`.
6. Run `node dist/packages/wp-now/cli.js start`.
7. Turn off internet connection.
8. Navigate to the site's WP admin (`/wp-admin`).
9. Observe no errors are logged and that `wp-now` doesn't crash.
10. Navigate to `/wp-admin/plugin-install.php`.
11. Observe no errors are logged and that `wp-now` doesn't crash.
12. Observe that the page shows an error message due to being offline.
13. Navigate to `/wp-admin/update-core.php`
14. Observe no errors are logged and that `wp-now` doesn't crash.

> [!NOTE]
> It's recommended to disable the network cache in your browser via the
DevTools Network tab.
  • Loading branch information
fluiddot authored Apr 9, 2024
1 parent 6ad6031 commit c340b0d
Showing 1 changed file with 14 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,9 @@ async function onWsConnect(client: any, request: http.IncomingMessage) {
clientLog(
'WebSocket client disconnected: ' + code + ' [' + reason + ']'
);
target.end();
if (target) {
target.end();
}
} as any);
client.on('error', function (a: string | Buffer) {
clientLog('WebSocket client error: ' + a);
Expand All @@ -196,9 +198,17 @@ async function onWsConnect(client: any, request: http.IncomingMessage) {
let reqTargetIp;
if (net.isIP(reqTargetHost) === 0) {
clientLog('resolving ' + reqTargetHost + '... ');
const resolution = await lookup(reqTargetHost);
reqTargetIp = resolution.address;
clientLog('resolved ' + reqTargetHost + ' -> ' + reqTargetIp);
try {
const resolution = await lookup(reqTargetHost);
reqTargetIp = resolution.address;
clientLog('resolved ' + reqTargetHost + ' -> ' + reqTargetIp);
} catch (e) {
clientLog("can't resolve " + reqTargetHost + ' due to:', e);
// Send empty binary data to notify requester that connection was initiated
client.send([]);
client.close(3000);
return;
}
} else {
reqTargetIp = reqTargetHost;
}
Expand Down

0 comments on commit c340b0d

Please sign in to comment.