You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/08/09 12:46:21 UTC

[GitHub] [pulsar] BewareMyPower opened a new pull request #11607: Fix null MessageId may be passed to its compareTo() method

BewareMyPower opened a new pull request #11607:
URL: https://github.com/apache/pulsar/pull/11607


   Fixes #11604 
   
   ### Motivation
   
   There's a chance that null `MessageId` might be passed to its `compareTo()` method. Even if #10586 added the null check, it's still not thread safe because `lastCumulativeAck` might be modified after the null check between line 118 and line 121 in following code snippet:
   
   https://github.com/apache/pulsar/blob/1e9f0cde6b473f786273bd4ce0df8b43622a61ed/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java#L118-L121
   
   ### Modifications
   
   Add a local variable `messageIdOfLastAck` to cache `messageId` field of `lastCumulative` so that the `MessageId` won't change during the null check and `compareTo()` method.
   
   ### Verifying this change
   
   - [ ] Make sure that the change passes the CI checks.
   
   This change is a trivial rework / code cleanup without any test coverage.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

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

[GitHub] [pulsar] BewareMyPower commented on a change in pull request #11607: Fix null MessageId may be passed to its compareTo() method

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #11607:
URL: https://github.com/apache/pulsar/pull/11607#discussion_r685165830



##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java
##########
@@ -115,10 +115,11 @@ public PersistentAcknowledgmentsGroupingTracker(ConsumerImpl<?> consumer, Consum
      */
     @Override
     public boolean isDuplicate(@NonNull MessageId messageId) {
-        if (lastCumulativeAck.messageId == null) {
+        final MessageId messageIdOfLastAck = lastCumulativeAck.messageId;

Review comment:
       Make sense. I'll fix it.




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

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

[GitHub] [pulsar] BewareMyPower commented on pull request #11607: Fix null MessageId may be passed to its compareTo() method

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11607:
URL: https://github.com/apache/pulsar/pull/11607#issuecomment-895206201


   @leizhiyuan 
   
   > if you changed lastCumulativeAck.messageId to null, messageIdOfLastAck also will be null.
   
   I think not. They both reference to the same object, but if you set one of them to null, the other still references to the original object.
   
   ![image](https://user-images.githubusercontent.com/18204803/128710832-4547104a-c14b-4086-8b1c-02f5b081ea07.png)


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

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

[GitHub] [pulsar] leizhiyuan commented on pull request #11607: Fix null MessageId may be passed to its compareTo() method

Posted by GitBox <gi...@apache.org>.
leizhiyuan commented on pull request #11607:
URL: https://github.com/apache/pulsar/pull/11607#issuecomment-895224778


   ok👍


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

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

[GitHub] [pulsar] BewareMyPower commented on pull request #11607: Fix null MessageId may be passed to its compareTo() method

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on pull request #11607:
URL: https://github.com/apache/pulsar/pull/11607#issuecomment-895193377


   @leizhiyuan Please also take a look.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

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

[GitHub] [pulsar] gaozhangmin commented on a change in pull request #11607: Fix null MessageId may be passed to its compareTo() method

Posted by GitBox <gi...@apache.org>.
gaozhangmin commented on a change in pull request #11607:
URL: https://github.com/apache/pulsar/pull/11607#discussion_r685166407



##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java
##########
@@ -115,10 +115,11 @@ public PersistentAcknowledgmentsGroupingTracker(ConsumerImpl<?> consumer, Consum
      */
     @Override
     public boolean isDuplicate(@NonNull MessageId messageId) {
-        if (lastCumulativeAck.messageId == null) {
+        final MessageId messageIdOfLastAck = lastCumulativeAck.messageId;

Review comment:
       agree




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

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

[GitHub] [pulsar] leizhiyuan commented on a change in pull request #11607: Fix null MessageId may be passed to its compareTo() method

Posted by GitBox <gi...@apache.org>.
leizhiyuan commented on a change in pull request #11607:
URL: https://github.com/apache/pulsar/pull/11607#discussion_r685191270



##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java
##########
@@ -115,10 +115,11 @@ public PersistentAcknowledgmentsGroupingTracker(ConsumerImpl<?> consumer, Consum
      */
     @Override
     public boolean isDuplicate(@NonNull MessageId messageId) {
-        if (lastCumulativeAck.messageId == null) {
+        final MessageId messageIdOfLastAck = lastCumulativeAck.messageId;

Review comment:
       maybe need a lock，when writing or reading the messageId




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

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

[GitHub] [pulsar] leizhiyuan commented on a change in pull request #11607: Fix null MessageId may be passed to its compareTo() method

Posted by GitBox <gi...@apache.org>.
leizhiyuan commented on a change in pull request #11607:
URL: https://github.com/apache/pulsar/pull/11607#discussion_r685164858



##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java
##########
@@ -115,10 +115,11 @@ public PersistentAcknowledgmentsGroupingTracker(ConsumerImpl<?> consumer, Consum
      */
     @Override
     public boolean isDuplicate(@NonNull MessageId messageId) {
-        if (lastCumulativeAck.messageId == null) {
+        final MessageId messageIdOfLastAck = lastCumulativeAck.messageId;

Review comment:
       messageId is an reference Type. 
   
   if you changed lastCumulativeAck.messageId to null, messageIdOfLastAck also will be null.
   




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

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

[GitHub] [pulsar] merlimat merged pull request #11607: Fix null MessageId may be passed to its compareTo() method

Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #11607:
URL: https://github.com/apache/pulsar/pull/11607


   


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

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

[GitHub] [pulsar] BewareMyPower commented on a change in pull request #11607: Fix null MessageId may be passed to its compareTo() method

Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on a change in pull request #11607:
URL: https://github.com/apache/pulsar/pull/11607#discussion_r685168151



##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/PersistentAcknowledgmentsGroupingTracker.java
##########
@@ -115,10 +115,11 @@ public PersistentAcknowledgmentsGroupingTracker(ConsumerImpl<?> consumer, Consum
      */
     @Override
     public boolean isDuplicate(@NonNull MessageId messageId) {
-        if (lastCumulativeAck.messageId == null) {
+        final MessageId messageIdOfLastAck = lastCumulativeAck.messageId;

Review comment:
       I think the best way is to replace all the external access to `messageId` with a synchronized method of `LastCumulativeAck`, what do you think?




-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

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