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

fix inifinte loop with invalid hostname #212

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

arnesetzer
Copy link
Contributor

@arnesetzer arnesetzer commented Jul 25, 2024

👋 I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.


Here's the reason for this change 🚀


Here's what actually got changed 👏

Add another check for the error type. If it is "ENOTFOUND" it now will throw an error and abort the execution.
https://github.com/arnesetzer/pelias-fuzzy-tester/blob/fixInvalidHost/lib/request_urls.js#L83


Here's how others can test the changes 👀

Try a invalid hostname. This should now throw a new error at the first occurance and exit instead of trying to reconnect to this host till ctrl+c.

@missinglink
Copy link
Member

missinglink commented Jul 25, 2024

I'm on mobile so it's hard to tell, is this code valid?

Seems to me the braces don't match?

Maybe you can delete the else { as the throw precludes the need for an else?

It's kinda hard to tell since half of the PR seems to be white spaces changes, can we just keep the whitespace the same to make the diffs easier to read plz 🙏

@arnesetzer
Copy link
Contributor Author

Is more readable now. Removed unused else statement. And you were right, there was a brace to much. 🙈

@orangejulius
Copy link
Member

I like this feature.

I can also confirm the code was initially broken with a syntax error but is fixed now.

I'm ok to merge it but can we possibly add a feature where it prints the hostname that can't be found? It should be findable either directly from the options we pass to request or possibly with a call to url.parse.

@arnesetzer
Copy link
Contributor Author

The last commit adds the hostname to the error message.

arne@workstation:~/temp/fuzzy-tester$ ./bin/fuzzy-tester -e fail
Error: getaddrinfo ENOTFOUND what.ever
    at GetAddrInfoReqWrap.onlookupall [as oncomplete] (node:dns:118:26) {
  errno: -3008,
  code: 'ENOTFOUND',
  syscall: 'getaddrinfo',
  hostname: 'what.ever'
}
/home/arne/temp/fuzzy-tester/lib/request_urls.js:84
          throw new Error( 'Invalid hostname: "'+ err.hostname + '". Stopping execution.' );
          ^

Error: Invalid hostname: "what.ever". Stopping execution.
    at Request._callback (/home/arne/temp/fuzzy-tester/lib/request_urls.js:84:17)
    at ...

Can you find it quickly enough or should it throw a console.err( err.hostname ) before the error? The node stack trace inflates the exception somewhat.😅

Copy link
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@missinglink missinglink merged commit 92101aa into pelias:master Aug 7, 2024
@arnesetzer arnesetzer deleted the fixInvalidHost branch August 7, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invaild hostname wont abort execution
3 participants