fix: Fail immediately for a bad password on latest amqplib. #304
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
luddd3
approved these changes
Oct 24, 2022
|
I have the same concern. :) Or maybe we should reject nothing and just keep generating connection failed events? |
|
🎉 This PR is included in version 4.1.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
I think that we maybe should reject all 4xx errors since all reconnect attempts will fail every time. Without the reject, the error is silently ignored and the caller will not be informed of the wrong parameters, e.g wrong credentials. |
|
It’s just… how? In this particular case, the error didn’t have a “status” on it. We could write a regex that looks for “4/d/d” in the error message but that seems a bit sketchy.
… On Oct 24, 2022, at 15:34, Mathias Lundell ***@***.***> wrote:
I think that we maybe should reject all 4xx errors since all reconnect attempts will fail every time. Without the reject, the error is silently ignored and the caller will not be informed of the wrong parameters, e.g wrong credentials.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you modified the open/close state.
|
|
I think that is what we would have to do, unless amqplib is fixed so we get the error code as a property on the error. I saw an old issue at amqplib where they did something similar (but a regexp is better) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related to #303, it looks like now amqplib no longer has an
isOperationalflag on errors anymore. It looks like this was a BlueBird thing, and amqplib switch to native promises so it was removed.We used to fail immediately with a reject when trying to connect with a bad password, but because of the missing
isOperationalflag we don't anymore. I fix this, and verified it on amqplib v0.8.0, then bumped the development version of amqplib to v0.10.3.