[GitHub] [pulsar] massakam commented on a change in pull request #10920: [broker] Fix issue where Key_Shared consumers could get stuck

[GitBox <gi...@apache.org>]

massakam commented on a change in pull request #10920:
URL: https://github.com/apache/pulsar/pull/10920#discussion_r651210993



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentStickyKeyDispatcherMultipleConsumers.java
##########
@@ -121,8 +121,14 @@ public synchronized void addConsumer(Consumer consumer) throws BrokerServiceExce
 
     @Override
     public synchronized void removeConsumer(Consumer consumer) throws BrokerServiceException {
-        super.removeConsumer(consumer);
+        // The consumer must be removed from the selector before calling the superclass removeConsumer method.
+        // In the superclass removeConsumer method, the pending acks that the consumer has are added to
+        // messagesToRedeliver. If the consumer has not been removed from the selector at this point,

Review comment:
       @codelipenghui Perhaps this happens if the messages to be redelivered is in the managed ledger cache. In this case,
   ```
   readMoreEntries()
   ↓
   readEntriesComplete()
   ↓
   sendMessagesToConsumers()
   ```
   are executed and completed synchronously in `PersistentDispatcherMultipleConsumers#removeConsumer()`.
   https://github.com/apache/pulsar/blob/894d92b2be3bee334e7ce32760c4d2e7978603aa/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java#L184-L195
   
   `synchronized` protection does not work because all of these methods are executed by the same thread.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org