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: only silence warnings (not fatal errors) #98

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

coolaj86
Copy link

Perhaps due to a change in he behavior of async.retry or perhaps by some other means, all connection errors (except verifying block warnings) are silenced on startup.

This means that any input that guarantees permanent failure - such as a wrong password or hostname or ip address or port - will just "sit there" without any feedback to the user.

Very confusing.

This changes the behavior so that the warning is logged and then ignored, but unexpected and fatal errors will throw.

If there are other non-fatal errors, then they can be added to the isNonFatalError filter.

Copy link
Collaborator

@pshenmic pshenmic left a comment

Choose a reason for hiding this comment

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

Thank you, looks good! But some of tests are failing

lib/services/dashd.js Outdated Show resolved Hide resolved
@coolaj86
Copy link
Author

Thank you, looks good! But some of tests are failing

I'll take a look in a few hours to update the tests.

I guess someone decided to update them to pass rather than figure out why they were failing the first time? 🤷‍♂️

It obviously should only retry 60 times when there's an error that is known to be temporary (verifying the blocks) and not for things like authentication errors or bad host and port.

@coolaj86 coolaj86 force-pushed the fix-dont-drop-real-errors branch 4 times, most recently from 493a29f to e80f0d7 Compare June 20, 2023 06:21
@coolaj86 coolaj86 changed the title fix: don't silence all (non-warning) errors on startup fix: only silence warnings (not fatal errors) Jun 20, 2023
@coolaj86 coolaj86 force-pushed the fix-dont-drop-real-errors branch from e80f0d7 to 3374cde Compare June 20, 2023 06:34
@coolaj86
Copy link
Author

coolaj86 commented Jun 20, 2023

Note: This is NOT a deal-breaker (because it's not even our fault), but zeromq errors are still silenced: zeromq/zeromq.js#574

A possible workaround may be to test the connection beforehand by opening a TCP socket and reporting any error.

Or maybe just setting a timeout.

test/services/dashd.unit.js Show resolved Hide resolved
lib/services/dashd.js Outdated Show resolved Hide resolved
@coolaj86 coolaj86 force-pushed the fix-dont-drop-real-errors branch from 3374cde to c5a6967 Compare June 23, 2023 20:45
.jshintrc Show resolved Hide resolved
@coolaj86
Copy link
Author

bump

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.

3 participants