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

Socket errors don't get caught #1

Closed
TheDevMinerTV opened this issue Nov 8, 2023 · 4 comments
Closed

Socket errors don't get caught #1

TheDevMinerTV opened this issue Nov 8, 2023 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@TheDevMinerTV
Copy link

Description

The Node.js socket errors don't get caught at the moment and there is no way to do so. Node.js will exit the process when a EventEmitter (such as node:net.Socket) emits an event called "error".

Current behaviour

When the code receives a Connection refused, it will exit the process.

Expected behaviour

It throws an error.

Proposal

Add a hook for the "error" socket event and on an error rethrow the error to the caller of lookup().

@TheDevMinerTV
Copy link
Author

TheDevMinerTV commented Nov 8, 2023

I fixed this by patching the index.ts file with this:

diff --git a/dist/index.js b/dist/index.js
index 608a23e75edd07db90aa393eec89a02394c4fdf6..5fd3730a8b6bc5391872d9dda12b89d30c68770d 100644
--- a/dist/index.js
+++ b/dist/index.js
@@ -66,8 +66,7 @@ export async function lookup(options) {
         });
         portal.once("error", (netError) => {
             clearTimeout(timeoutFunc);
-            reject();
-            throw netError;
+            reject(netError);
         });
         let timeoutFunc = setTimeout(() => {
             portal.destroy();

It seems like the first reject is throwing an error but the second reject will also throw an error which makes my outer try/catch not get the second error. Super strange

@woodendoors7 woodendoors7 self-assigned this Nov 8, 2023
@woodendoors7 woodendoors7 added the bug Something isn't working label Nov 8, 2023
@woodendoors7
Copy link
Owner

Bug fixed in v1.1.3. The bug was caused by a slightly older build being published to NPM in the name of the update that fixed error catching, as such the single throw remained.

Thanks for pointing this out.

@TheDevMinerTV
Copy link
Author

TheDevMinerTV commented Nov 13, 2023

Please reopen, socket timeouts crash the application and cannot be caught in the consumer.

file:///app/node_modules/.pnpm/minecraftstatuspinger@1.1.2_patch_hash=pcn5zlr7zqzsypluleoqwxgkpu/node_modules/minecraftstatuspinger/dist/index.js:73
            throw new Error("Timed out.");
            ^

Error: Timed out.
    at Timeout._onTimeout (file:///app/node_modules/.pnpm/minecraftstatuspinger@1.1.2_patch_hash=pcn5zlr7zqzsypluleoqwxgkpu/node_modules/minecraftstatuspinger/dist/index.js:73:19)
    at listOnTimeout (node:internal/timers:569:17)
    at process.processTimers (node:internal/timers:512:7)

(I've patched the library myself to remove the throw)

@woodendoors7
Copy link
Owner

woodendoors7 commented Nov 13, 2023

@TheDevMinerTV You need to update to v1.1.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants