Fix race condition on producer/consumer maps in ServerCnx #9256
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
ServerCnx had a callback that was called from Producer/Consumer which would remove the producer/consumer from its map using only the ID. However, it is possible that this callback runs when the producer/consumer had already been removed from the map and another producer/consumer added in its place. The solution is to use both the key and value when removing from the map. The change also updates the log messages to include the producerId and consumerId in a format that all log messages for an individual producerId/consumerId can be easier found. A test has been changed because the test was depending on the broken behaviour. What was happening was that the fail topic producer was failing to create a producer, and when this happened it removed the producer future for the successful producer. Then, when the third producer tries to connect, it sees manages to create the producer on the connection, but fails as there is already a producer with that name on the topic. The correct behaviour is that it should see the successful producer future for that ID and respond with success.
aahmed-se
approved these changes
Jan 21, 2021
sijie
approved these changes
Jan 21, 2021
codelipenghui
approved these changes
Jan 21, 2021
merlimat
changed the title
merlimat
approved these changes
Jan 21, 2021
codelipenghui
pushed a commit
that referenced
this pull request
Jan 26, 2021
…9256) ServerCnx had a callback that was called from Producer/Consumer which would remove the producer/consumer from its map using only the ID. However, it is possible that this callback runs when the producer/consumer had already been removed from the map and another producer/consumer added in its place. The solution is to use both the key and value when removing from the map. The change also updates the log messages to include the producerId and consumerId in a format that all log messages for an individual producerId/consumerId can be easier found. A test has been changed because the test was depending on the broken behaviour. What was happening was that the fail topic producer was failing to create a producer, and when this happened it removed the producer future for the successful producer. Then, when the third producer tries to connect, it sees manages to create the producer on the connection, but fails as there is already a producer with that name on the topic. The correct behaviour is that it should see the successful producer future for that ID and respond with success. Co-authored-by: Ivan Kelly <ikelly@splunk.com> (cherry picked from commit 1e4c3ec)
merlimat
pushed a commit
to merlimat/pulsar
that referenced
this pull request
Apr 6, 2021
…pache#9256) ServerCnx had a callback that was called from Producer/Consumer which would remove the producer/consumer from its map using only the ID. However, it is possible that this callback runs when the producer/consumer had already been removed from the map and another producer/consumer added in its place. The solution is to use both the key and value when removing from the map. The change also updates the log messages to include the producerId and consumerId in a format that all log messages for an individual producerId/consumerId can be easier found. A test has been changed because the test was depending on the broken behaviour. What was happening was that the fail topic producer was failing to create a producer, and when this happened it removed the producer future for the successful producer. Then, when the third producer tries to connect, it sees manages to create the producer on the connection, but fails as there is already a producer with that name on the topic. The correct behaviour is that it should see the successful producer future for that ID and respond with success.
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
ServerCnx had a callback that was called from Producer/Consumer which
would remove the producer/consumer from its map using only the
ID. However, it is possible that this callback runs when the
producer/consumer had already been removed from the map and another
producer/consumer added in its place.
The solution is to use both the key and value when removing from the
map.
The change also updates the log messages to include the producerId and
consumerId in a format that all log messages for an individual
producerId/consumerId can be easier found.
A test has been changed because the test was depending on the broken
behaviour. What was happening was that the fail topic producer was
failing to create a producer, and when this happened it removed the
producer future for the successful producer. Then, when the third
producer tries to connect, it sees manages to create the producer on
the connection, but fails as there is already a producer with that
name on the topic. The correct behaviour is that it should see the
successful producer future for that ID and respond with success.