Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch DNS errors to avoid unhandled exceptions #1215

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Apr 9, 2024

What is this PR doing?

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 references wp-now, so it's recommended to test the changes using a local build of wp-now from the playground-tools repository.

  1. In wordpress-playground repository, run nx build php-wasm-node.
  2. Check out playground-tools repository.
  3. In playground-rools repository, update the @php-wasm/node dependency to point to the local path of the wordpress-playground repository.

Example:

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

@fluiddot
Copy link
Contributor Author

fluiddot commented Apr 9, 2024

Following this comment, @sejas @adamziel it's my first PR on the wordpress-playground repository, so I welcome all feedback and thoughts. Thanks for the help 🙇 !

@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Aspect] Networking labels Apr 9, 2024
@adamziel
Copy link
Collaborator

adamziel commented Apr 9, 2024

This should do the trick, thank you so much for contributing @fluiddot! 🎉 I just initiated a new npm packages release so wp-now could upgrade – cc @sejas

@adamziel adamziel merged commit c340b0d into WordPress:trunk Apr 9, 2024
5 checks passed
@fluiddot fluiddot deleted the catch-dns-errors branch April 9, 2024 11:07
@sejas
Copy link
Collaborator

sejas commented Apr 9, 2024

@fluiddot Thanks for the PR! Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Aspect] Networking [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants