You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by GitBox <gi...@apache.org> on 2022/04/12 11:21:00 UTC

[GitHub] [rocketmq] mz0113 opened a new issue, #4154: maybe there is a Thread Safe problem when send a message in the 'MQFaultStrategy#selectOneMessageQueue' code

mz0113 opened a new issue, #4154:
URL: https://github.com/apache/rocketmq/issues/4154

   ![1649761765(1)](https://user-images.githubusercontent.com/42512469/162947438-82a56beb-a899-4022-a7ee-17f44a65d154.png)
   
   mq.setBrokerName(notBestBroker);
   mq.setQueueId(tpInfo.getSendWhichQueue().getAndIncrement() % writeQueueNums);
   
   this method of selectOneMessageQueue is executed by multiThread when send a message but the mq (messageQueue) is a shared object , thread A modify it`s brokerName to brokerB,but have not modify it queueId, still 1 , then thread B send msg and use this queue,but now,this queue `s brokerName is brokerB but it`s queueId is 1, in this case , wo can`t sure that brokerB really have queue which queueId is 1
   
   Isn't that really a problem?
                      


-- 
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: dev-unsubscribe@rocketmq.apache.org.apache.org

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


[GitHub] [rocketmq] ni-ze commented on issue #4154: maybe there is a Thread Safe problem when send a message in the 'MQFaultStrategy#selectOneMessageQueue' code

Posted by GitBox <gi...@apache.org>.
ni-ze commented on issue #4154:
URL: https://github.com/apache/rocketmq/issues/4154#issuecomment-1097608746

   ```java
       int writeQueueNums = tpInfo.getQueueIdByBroker(notBestBroker);
      ...
       mq.setBrokerName(notBestBroker);
       mq.setQueueId(tpInfo.getSendWhichQueue().incrementAndGet() % writeQueueNums);
   ```
   a messageQueue's brokerName and queueId is consistent, because writeQueueNums is belong to the same brokerName, which is notBestBroker in this case. 
   
   


-- 
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: dev-unsubscribe@rocketmq.apache.org

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


[GitHub] [rocketmq] mz0113 commented on issue #4154: maybe there is a Thread Safe problem when send a message in the 'MQFaultStrategy#selectOneMessageQueue' code

Posted by GitBox <gi...@apache.org>.
mz0113 commented on issue #4154:
URL: https://github.com/apache/rocketmq/issues/4154#issuecomment-1097543543

   > I do not think so.
   > 
   > ```java
   > tpInfo.getSendWhichQueue().incrementAndGet()
   > ```
   > 
   > the index is a thread local variable, would change with thread.if thread switched, index will change.
   
   It is precisely because the index has changed that other threads have the opportunity to use this queue


-- 
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: dev-unsubscribe@rocketmq.apache.org

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


[GitHub] [rocketmq] mz0113 closed issue #4154: maybe there is a Thread Safe problem when send a message in the 'MQFaultStrategy#selectOneMessageQueue' code

Posted by GitBox <gi...@apache.org>.
mz0113 closed issue #4154: maybe there is a Thread Safe problem when send a message in the  'MQFaultStrategy#selectOneMessageQueue' code
URL: https://github.com/apache/rocketmq/issues/4154


-- 
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: dev-unsubscribe@rocketmq.apache.org

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


[GitHub] [rocketmq] lwclover commented on issue #4154: maybe there is a Thread Safe problem when send a message in the 'MQFaultStrategy#selectOneMessageQueue' code

Posted by GitBox <gi...@apache.org>.
lwclover commented on issue #4154:
URL: https://github.com/apache/rocketmq/issues/4154#issuecomment-1097467953

   > ![1649761765(1)](https://user-images.githubusercontent.com/42512469/162947438-82a56beb-a899-4022-a7ee-17f44a65d154.png)
   > 
   > mq.setBrokerName(notBestBroker); mq.setQueueId(tpInfo.getSendWhichQueue().getAndIncrement() % writeQueueNums);
   > 
   > this method of selectOneMessageQueue is executed by multiThread when send a message but the mq (messageQueue) is a shared object , thread A modify it`s brokerName to brokerB,but have not modify it queueId, still 1 , then thread B send msg and use this queue,but now,this queue `s brokerName is brokerB but it`s queueId is 1, in this case , wo can`t sure that brokerB really have queue which queueId is 1
   > 
   > Isn't that really a problem?
   
   I think you are right, to be more precise, constructors should be used.


-- 
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: dev-unsubscribe@rocketmq.apache.org

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


[GitHub] [rocketmq] ni-ze commented on issue #4154: maybe there is a Thread Safe problem when send a message in the 'MQFaultStrategy#selectOneMessageQueue' code

Posted by GitBox <gi...@apache.org>.
ni-ze commented on issue #4154:
URL: https://github.com/apache/rocketmq/issues/4154#issuecomment-1097489919

   I do not think so.
   ```java
   tpInfo.getSendWhichQueue().incrementAndGet()
   ```
   the index is a thread local variable, would change with thread.if thread switched, index will change.


-- 
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: dev-unsubscribe@rocketmq.apache.org

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


[GitHub] [rocketmq] lwclover commented on issue #4154: maybe there is a Thread Safe problem when send a message in the 'MQFaultStrategy#selectOneMessageQueue' code

Posted by GitBox <gi...@apache.org>.
lwclover commented on issue #4154:
URL: https://github.com/apache/rocketmq/issues/4154#issuecomment-1097511342

   > I do not think so.
   > 
   > ```java
   > tpInfo.getSendWhichQueue().incrementAndGet()
   > ```
   > 
   > the index is a thread local variable, would change with thread.if thread switched, index will change.
   
   There's a mod


-- 
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: dev-unsubscribe@rocketmq.apache.org

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


[GitHub] [rocketmq] MatrixHB commented on issue #4154: maybe there is a Thread Safe problem when send a message in the 'MQFaultStrategy#selectOneMessageQueue' code

Posted by GitBox <gi...@apache.org>.
MatrixHB commented on issue #4154:
URL: https://github.com/apache/rocketmq/issues/4154#issuecomment-1100837679

   The core problem is that  the shared object `messageQueueList.get(pos)` cannot be modified after being obtained.
   
   I've proposed a PR [#4179 ](https://github.com/apache/rocketmq/pull/4179) please review it. @mz0113 


-- 
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: dev-unsubscribe@rocketmq.apache.org

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


[GitHub] [rocketmq] ni-ze commented on issue #4154: maybe there is a Thread Safe problem when send a message in the 'MQFaultStrategy#selectOneMessageQueue' code

Posted by GitBox <gi...@apache.org>.
ni-ze commented on issue #4154:
URL: https://github.com/apache/rocketmq/issues/4154#issuecomment-1274193768

   seems like does not matter.
   
   sunhangda ***@***.***>于2022年4月13日 周三11:20写道:
   
   > I do not think so.
   >
   > tpInfo.getSendWhichQueue().incrementAndGet()
   >
   > the index is a thread local variable, would change with thread.if thread
   > switched, index will change.
   >
   > There's a mod
   >
   > —
   > Reply to this email directly, view it on GitHub
   > <https://github.com/apache/rocketmq/issues/4154#issuecomment-1097511342>,
   > or unsubscribe
   > <https://github.com/notifications/unsubscribe-auth/AHN3EQXJXKOH2TQGCSXHBQLVEY4RLANCNFSM5TG6KRVA>
   > .
   > You are receiving this because you commented.Message ID:
   > ***@***.***>
   >
   


-- 
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: dev-unsubscribe@rocketmq.apache.org

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


[GitHub] [rocketmq] mz0113 commented on issue #4154: maybe there is a Thread Safe problem when send a message in the 'MQFaultStrategy#selectOneMessageQueue' code

Posted by GitBox <gi...@apache.org>.
mz0113 commented on issue #4154:
URL: https://github.com/apache/rocketmq/issues/4154#issuecomment-1097686739

   no, that is not my mean , you are right in some aspect,but what i want to say is  messagequeue it self is a shared object , it means all threads will use the same messageQueue object, for current thread, the messageQueue with is consistent,but for other thread, it may not  be consistent ,
   `org.apache.rocketmq.client.impl.producer.TopicPublishInfo#selectOneMessageQueue() `
   
   `    public MessageQueue selectOneMessageQueue() {
           int index = this.sendWhichQueue.getAndIncrement();
           int pos = Math.abs(index) % this.messageQueueList.size();
           if (pos < 0)
               pos = 0;
           return this.messageQueueList.get(pos);
       }
   `
   
   this method will not sure it`s return value(messageQueue) is always belong to notBestBroker,so the method may be return a mq from brokerA or brokerB ,etc .for example, when this.sendWhichQueue is too big to becoming a number less than zero, the pos will be zero, and     
   return this.messageQueueList.get(pos); will return a messageQueue object which may not belong to notBestBroker


-- 
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: dev-unsubscribe@rocketmq.apache.org

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