Fix bug when connecting to the same stream name different vhosts #246
Conversation
This file contains hidden or 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
Contributor
Author
|
The failing test is also broken on |
Contributor
|
Hello, thanks a lot for taking the time to open the pr. We will check it and will comment it in the following days. |
l4mby
reviewed
Apr 9, 2025
l4mby
previously approved these changes
Apr 23, 2025
Contributor
|
@david-mohr can you please rebase your branch? we fixed the failing pipeline, so it should be correct now. |
david-mohr
force-pushed
the
fix/same-stream-different-vhost
branch
from
3ced3c3 to
bd064e3
Compare
david-mohr
force-pushed
the
fix/same-stream-different-vhost
branch
from
bd064e3 to
3cffc16
Compare
Contributor
Author
|
I ran the tests locally using node v20 and they passed, so this failing test is unexpected... |
Contributor
Author
|
@l4mby I tried adding a minor change to re-trigger the CI tests and now a different test is failing. I don't think these are related to my change and running the tests locally everything still passes. Not sure how to proceed from here... any advice is welcome. |
l4mby
approved these changes
Apr 28, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
During testing, we ran into a problem where we would create two connections to different vhosts, each with the same stream name. We could see in the RabbitMQ management interface that the configuration is valid and accessing them individually would work as expected, but when trying to access them at the same time, we would get messages leaking into the wrong vhost.
After debugging the issue, I tracked the root cause to the
ConnectionPool.getCacheKeyfunction which would build a cache using only the stream name, not the vhost. This meant that we were always getting the cached copy regardless of the actual vhost.All the other changes are to support that one change and there is a test to confirm the result.