[Broker] Fix producer getting incorrectly removed from topic's produc…

[michaeljmarshall]

…ers map

Motivation

There is currently a bug that allows a producer to be removed from a topic's producers map, but not from the ServerCnx#producers map. As a consequence, two producers with the same name can connect at the same time.

The fundamental issue here is with the definition of equality for the Producer class. This class's definition of equality has been an issue before. @merlimat fixed one such issue here: #11804. That fix addressed some of the problems, but I think we need to go a step further in making sure that the wrong producer cannot be removed from the topic's producers map.

In the new test that I add with this PR, I show that it is still possible to remove the wrong producer. In the test, we essentially get two callbacks on the getOrCreateTopic future. Once that future succeeds, we create one producer and then the second fails and calls producer.closeNow(true), which removes the successful producer from the topic. It does not remove the successful topic from the ServerCnx because of the changes introduced in #9256.

The producer's equals method is only used when ensuring that the right producer is removed from the producers map and when determining if a producer is allowed to overwrite another producer. In my view, the simplest solution is to remove the overriding of equals and put the logic from equals into another method. This way removing a producer from a map is not too complicated (object equality is now reference equality), and we can still determine if a producer should override another.

Modifications

• Remove Producer#equals and Producer#hashCode to rely on the default object implementations.
• Add Producer#isSuccessorTo to retain the logic required to determine if a producer is okay to overwrite an existing producer.

Other Benefits

When a producer is not present in a topic's producers map, the producer does not report metrics.

Verifying this change

I added a new test that shows a way to reproduce the issue. The new test fails on master but succeeds on my branch. There are also many tests that verify this code path to ensure this change does not introduce a regression.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

• Dependencies (does it add or upgrade a dependency): no
• The public API: no
• The schema: no
• The default values of configurations: no
• The wire protocol: no
• The rest endpoints: no
• The admin cli options: no
• Anything that affects deployment: no

Documentation

• no-need-doc

This is an internal bug fix.

[michaeljmarshall]

@ivankelly - in looking at #9256, I am wondering if I should also submit a PR similar to this one but for consumers. I haven't researched the consumer path for removal yet, so I cannot say for sure, but if there was a race for closing the wrong consumers, it's possible that the definition of equality is too broad for the Consumer class, and if so, the wrong consumer could be removed from the topic. Let me know, thanks.

[eolivelli]

Great catch

[michaeljmarshall]

/pulsarbot run-failure-checks

[nicoloboschi]

+1

[merlimat]

Should we leave hashCode() & equals() with explanation on why it needs to be identity equal? Otherwise, someone will add it back later on...

[michaeljmarshall]

The tests I've added/updated should fail if the definition of equality changes or becomes too broad. If you are worried that the tests are insufficient, I'm happy to add the overrides back in. Let me know.