Uploaded image for project: 'Apache RocketMQ'
  1. Apache RocketMQ
  2. ROCKETMQ-107

Access ServiceState is not thread safe when start() or shutdown()

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 4.0.0-incubating
    • Fix Version/s: 4.1.0-incubating
    • Component/s: rocketmq-client
    • Labels:
      None

      Description

      When start() or shutdown(), service's state is not thread safe which may break happen-before.

      For example:

      switch (this.serviceState) {
                  case CREATE_JUST:
                      log.info("the consumer [{}] start beginning. messageModel={}, isUnitMode={}", this.defaultMQPushConsumer.getConsumerGroup(),
                          this.defaultMQPushConsumer.getMessageModel(), this.defaultMQPushConsumer.isUnitMode());
                      this.serviceState = ServiceState.START_FAILED;
                      ..// do some start job here
                      this.serviceState = ServiceState.RUNNING;
                      break;
                  case RUNNING:
                  case START_FAILED:
                  case SHUTDOWN_ALREADY:
                      throw new MQClientException("The PushConsumer service state not OK, maybe started once, "//
                          + this.serviceState//
                          + FAQUrl.suggestTodo(FAQUrl.CLIENT_SERVICE_NOT_OK),
                          null);
                  default:
                      break;
              }
      

      1. If the user is start twice in two thread, the resources may initize twice.
      2. if the user start in threadA and shutdown very quicky in another thread B, shutdown may not reclaim the resources.

      Though the sceniro is very uncommon, but it is indeed a bug here. Fix is actually quite trivial.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user Jaskey opened a pull request:

          https://github.com/apache/incubator-rocketmq/pull/68

          ROCKETMQ-107 fix possible concurrency problem on ServiceState when consumer start/shutdown

          JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-107

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-107-synchroization-on-ServiceState

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/incubator-rocketmq/pull/68.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #68


          commit 91c41dae09405be95ed9ad8696959f79683dfd31
          Author: Jaskey <linjunjie1103@gmail.com>
          Date: 2017-02-23T03:06:32Z

          ROCKETMQ-107 fix possible concurrency problem on ServiceState when consumer start/shutdown


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user Jaskey opened a pull request: https://github.com/apache/incubator-rocketmq/pull/68 ROCKETMQ-107 fix possible concurrency problem on ServiceState when consumer start/shutdown JIRA: https://issues.apache.org/jira/browse/ROCKETMQ-107 You can merge this pull request into a Git repository by running: $ git pull https://github.com/Jaskey/incubator-rocketmq ROCKETMQ-107 -synchroization-on-ServiceState Alternatively you can review and apply these changes as the patch at: https://github.com/apache/incubator-rocketmq/pull/68.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #68 commit 91c41dae09405be95ed9ad8696959f79683dfd31 Author: Jaskey <linjunjie1103@gmail.com> Date: 2017-02-23T03:06:32Z ROCKETMQ-107 fix possible concurrency problem on ServiceState when consumer start/shutdown
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Ah39 commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          1.AtomicReference<State> State = new AtomicReference()
          state.compareAndSet

          2.volatile

          can fix the problem

          Show
          githubbot ASF GitHub Bot added a comment - Github user Ah39 commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 1.AtomicReference<State> State = new AtomicReference() state.compareAndSet 2.volatile can fix the problem
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          @Ah39
          The problem is not only the synchrinaztion on state's read/write, but on `shutdown` and `start` , `shutdown` and `start` should be syncronized

          For example, two thread may start in the same time and the the resources will be initized twice, actually the later one should be rejected, which is the problem I issue.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @Ah39 The problem is not only the synchrinaztion on state's read/write, but on `shutdown` and `start` , `shutdown` and `start` should be syncronized For example, two thread may start in the same time and the the resources will be initized twice, actually the later one should be rejected, which is the problem I issue.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Ah39 commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

              1. apache/curator TreeCache Start**

          public TreeCache start() throws Exception
          {
          *Preconditions.checkState(treeState.compareAndSet(TreeState.LATENT, TreeState.STARTED)*, "already started");
          if ( createParentNodes )

          { client.createContainers(root.path); }

          client.getConnectionStateListenable().addListener(connectionStateListener);
          if ( client.getZookeeperClient().isConnected() )

          { root.wasCreated(); }

          return this;
          }

          Show
          githubbot ASF GitHub Bot added a comment - Github user Ah39 commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 apache/curator TreeCache Start** public TreeCache start() throws Exception { * Preconditions.checkState(treeState.compareAndSet(TreeState.LATENT, TreeState.STARTED) *, "already started"); if ( createParentNodes ) { client.createContainers(root.path); } client.getConnectionStateListenable().addListener(connectionStateListener); if ( client.getZookeeperClient().isConnected() ) { root.wasCreated(); } return this; }
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10308840/badge)](https://coveralls.io/builds/10308840)

          Coverage increased (+0.3%) to 31.803% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10308840/badge)](https://coveralls.io/builds/10308840 ) Coverage increased (+0.3%) to 31.803% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10308840/badge)](https://coveralls.io/builds/10308840)

          Coverage increased (+0.3%) to 31.803% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10308840/badge)](https://coveralls.io/builds/10308840 ) Coverage increased (+0.3%) to 31.803% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10308840/badge)](https://coveralls.io/builds/10308840)

          Coverage increased (+0.3%) to 31.803% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10308840/badge)](https://coveralls.io/builds/10308840 ) Coverage increased (+0.3%) to 31.803% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10308981/badge)](https://coveralls.io/builds/10308981)

          Coverage increased (+0.2%) to 31.72% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10308981/badge)](https://coveralls.io/builds/10308981 ) Coverage increased (+0.2%) to 31.72% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10308981/badge)](https://coveralls.io/builds/10308981)

          Coverage increased (+0.2%) to 31.72% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10308981/badge)](https://coveralls.io/builds/10308981 ) Coverage increased (+0.2%) to 31.72% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10308981/badge)](https://coveralls.io/builds/10308981)

          Coverage increased (+0.2%) to 31.72% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10308981/badge)](https://coveralls.io/builds/10308981 ) Coverage increased (+0.2%) to 31.72% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10309014/badge)](https://coveralls.io/builds/10309014)

          Coverage increased (+0.1%) to 31.627% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10309014/badge)](https://coveralls.io/builds/10309014 ) Coverage increased (+0.1%) to 31.627% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10309014/badge)](https://coveralls.io/builds/10309014)

          Coverage increased (+0.1%) to 31.627% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10309014/badge)](https://coveralls.io/builds/10309014 ) Coverage increased (+0.1%) to 31.627% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10309014/badge)](https://coveralls.io/builds/10309014)

          Coverage increased (+0.1%) to 31.627% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10309014/badge)](https://coveralls.io/builds/10309014 ) Coverage increased (+0.1%) to 31.627% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10309078/badge)](https://coveralls.io/builds/10309078)

          Coverage increased (+0.1%) to 31.636% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10309078/badge)](https://coveralls.io/builds/10309078 ) Coverage increased (+0.1%) to 31.636% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10309078/badge)](https://coveralls.io/builds/10309078)

          Coverage increased (+0.1%) to 31.636% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10309078/badge)](https://coveralls.io/builds/10309078 ) Coverage increased (+0.1%) to 31.636% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10309078/badge)](https://coveralls.io/builds/10309078)

          Coverage increased (+0.1%) to 31.636% when pulling *57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10309078/badge)](https://coveralls.io/builds/10309078 ) Coverage increased (+0.1%) to 31.636% when pulling * 57c2c6893b3fe1173aa7a68c15d63f309bd89138 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          @Ah39
          I guess you still not understand my concern or I don't get it, you may try post your suggested ways and lets discuss it , a simpler solution is certainlly better if we have.

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @Ah39 I guess you still not understand my concern or I don't get it, you may try post your suggested ways and lets discuss it , a simpler solution is certainlly better if we have.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhouxinyu commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          IMO, consider that `start/shutdown` is not a high frequency usage method, use `synchronized` may be more graceful and readable.

          please @lizhanhui @shroman help review at your convenience.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhouxinyu commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 IMO, consider that `start/shutdown` is not a high frequency usage method, use `synchronized` may be more graceful and readable. please @lizhanhui @shroman help review at your convenience.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user shroman commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          I also think that using `synchronized` fits well here.
          But I would do it in the following way:
          1. Make `serviceState` volatile
          ```java
          private volatile ServiceState serviceState;
          ```
          2. Synchronize only blocks that needs to be atomic (possibly making synchronized methods – current huge `start` chunk of code doesn't look nice)

          And, of course, provide comments to public methods

          Show
          githubbot ASF GitHub Bot added a comment - Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 I also think that using `synchronized` fits well here. But I would do it in the following way: 1. Make `serviceState` volatile ```java private volatile ServiceState serviceState; ``` 2. Synchronize only blocks that needs to be atomic (possibly making synchronized methods – current huge `start` chunk of code doesn't look nice) And, of course, provide comments to public methods
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10329086/badge)](https://coveralls.io/builds/10329086)

          Coverage increased (+0.03%) to 31.548% when pulling *d4dd3c7d4ae80c96c3f927575b312bef3ae0647a on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10329086/badge)](https://coveralls.io/builds/10329086 ) Coverage increased (+0.03%) to 31.548% when pulling * d4dd3c7d4ae80c96c3f927575b312bef3ae0647a on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10329086/badge)](https://coveralls.io/builds/10329086)

          Coverage increased (+0.03%) to 31.548% when pulling *d4dd3c7d4ae80c96c3f927575b312bef3ae0647a on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10329086/badge)](https://coveralls.io/builds/10329086 ) Coverage increased (+0.03%) to 31.548% when pulling * d4dd3c7d4ae80c96c3f927575b312bef3ae0647a on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10329086/badge)](https://coveralls.io/builds/10329086)

          Coverage increased (+0.03%) to 31.548% when pulling *d4dd3c7d4ae80c96c3f927575b312bef3ae0647a on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10329086/badge)](https://coveralls.io/builds/10329086 ) Coverage increased (+0.03%) to 31.548% when pulling * d4dd3c7d4ae80c96c3f927575b312bef3ae0647a on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user lizhanhui commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          As pointed out, I agree marking shutdown and start `synchronized` to keep code as concise as possible.

          Show
          githubbot ASF GitHub Bot added a comment - Github user lizhanhui commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 As pointed out, I agree marking shutdown and start `synchronized` to keep code as concise as possible.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user vongosling commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          @Jaskey Could you continue to polish this PR as we have pointed out

          Show
          githubbot ASF GitHub Bot added a comment - Github user vongosling commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @Jaskey Could you continue to polish this PR as we have pointed out
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          @shroman @vongosling

          Do all guys consider makeing method synchrinized which is locking the consumer object is a better way instead of a more fine-grained lock.

          If so, I will do the following changes:

          1. Making state as votilte
          2. synchronize `start` and `shutdown` and `setServiceState` method

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @shroman @vongosling Do all guys consider makeing method synchrinized which is locking the consumer object is a better way instead of a more fine-grained lock. If so, I will do the following changes: 1. Making state as votilte 2. synchronize `start` and `shutdown` and `setServiceState` method
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user shroman commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          Probably you don't want to synchronize `setServiceState`. For other methods, it's only large chunks like that of `CREATE_JUST` the has to be synchronized

          Show
          githubbot ASF GitHub Bot added a comment - Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 Probably you don't want to synchronize `setServiceState`. For other methods, it's only large chunks like that of `CREATE_JUST` the has to be synchronized
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          @shroman
          Actully setServiceState should be synchroinzed in my opnion, say if I am trying to shutdown, and some other thread B is trying to setServiceState ,which should be waiting for `shutdown` , or shutdown may not be completed since only `RUNNIG` state can be really shutdowned .

          If thread B is setting the state as SHUTDOWN_AREADY afterwards, shutdown will actually do nothing. So with the setServiceState, I do think synchrinization is needed, but I do NOT think `setServiceState` should be public which is dangerous

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @shroman Actully setServiceState should be synchroinzed in my opnion, say if I am trying to shutdown, and some other thread B is trying to setServiceState ,which should be waiting for `shutdown` , or shutdown may not be completed since only `RUNNIG` state can be really shutdowned . If thread B is setting the state as SHUTDOWN_AREADY afterwards, shutdown will actually do nothing. So with the setServiceState, I do think synchrinization is needed, but I do NOT think `setServiceState` should be public which is dangerous
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10339657/badge)](https://coveralls.io/builds/10339657)

          Coverage decreased (-0.02%) to 31.504% when pulling *605a34b58140513e4e6663ac98349279f60be2e3 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10339657/badge)](https://coveralls.io/builds/10339657 ) Coverage decreased (-0.02%) to 31.504% when pulling * 605a34b58140513e4e6663ac98349279f60be2e3 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10339657/badge)](https://coveralls.io/builds/10339657)

          Coverage decreased (-0.02%) to 31.504% when pulling *605a34b58140513e4e6663ac98349279f60be2e3 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10339657/badge)](https://coveralls.io/builds/10339657 ) Coverage decreased (-0.02%) to 31.504% when pulling * 605a34b58140513e4e6663ac98349279f60be2e3 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10339657/badge)](https://coveralls.io/builds/10339657)

          Coverage decreased (-0.02%) to 31.504% when pulling *605a34b58140513e4e6663ac98349279f60be2e3 on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10339657/badge)](https://coveralls.io/builds/10339657 ) Coverage decreased (-0.02%) to 31.504% when pulling * 605a34b58140513e4e6663ac98349279f60be2e3 on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10339753/badge)](https://coveralls.io/builds/10339753)

          Coverage decreased (-0.07%) to 31.449% when pulling *fcd7ef68c9a8d2fa32d4eb6f8b3773e4411fcefd on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10339753/badge)](https://coveralls.io/builds/10339753 ) Coverage decreased (-0.07%) to 31.449% when pulling * fcd7ef68c9a8d2fa32d4eb6f8b3773e4411fcefd on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10339753/badge)](https://coveralls.io/builds/10339753)

          Coverage decreased (-0.07%) to 31.449% when pulling *fcd7ef68c9a8d2fa32d4eb6f8b3773e4411fcefd on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10339753/badge)](https://coveralls.io/builds/10339753 ) Coverage decreased (-0.07%) to 31.449% when pulling * fcd7ef68c9a8d2fa32d4eb6f8b3773e4411fcefd on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user coveralls commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          [![Coverage Status](https://coveralls.io/builds/10339753/badge)](https://coveralls.io/builds/10339753)

          Coverage decreased (-0.07%) to 31.449% when pulling *fcd7ef68c9a8d2fa32d4eb6f8b3773e4411fcefd on Jaskey:ROCKETMQ-107-synchroization-on-ServiceState* into *573b22c37806a21543b90707bcce6022243a62da on apache:master*.

          Show
          githubbot ASF GitHub Bot added a comment - Github user coveralls commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 [! [Coverage Status] ( https://coveralls.io/builds/10339753/badge)](https://coveralls.io/builds/10339753 ) Coverage decreased (-0.07%) to 31.449% when pulling * fcd7ef68c9a8d2fa32d4eb6f8b3773e4411fcefd on Jaskey: ROCKETMQ-107 -synchroization-on-ServiceState * into * 573b22c37806a21543b90707bcce6022243a62da on apache:master *.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user shroman commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          @Jaskey I agree that `setServiceState` should be deprecated. I overlooked it was public.

          Show
          githubbot ASF GitHub Bot added a comment - Github user shroman commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @Jaskey I agree that `setServiceState` should be deprecated. I overlooked it was public .
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          @lizhanhui @zhouxinyu @shroman
          Any more advice on this pr? and when can this be merged?

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 @lizhanhui @zhouxinyu @shroman Any more advice on this pr? and when can this be merged?
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit f508f131f7dee2bcb86e66a6beb7bbdedbe31bc6 in incubator-rocketmq's branch refs/heads/develop from Jaskey Lam
          [ https://git-wip-us.apache.org/repos/asf?p=incubator-rocketmq.git;h=f508f13 ]

          ROCKETMQ-107 Fix possible concurrency problem on ServiceState when consumer start/shutdown, closes apache/incubator-rocketmq#68

          Show
          jira-bot ASF subversion and git services added a comment - Commit f508f131f7dee2bcb86e66a6beb7bbdedbe31bc6 in incubator-rocketmq's branch refs/heads/develop from Jaskey Lam [ https://git-wip-us.apache.org/repos/asf?p=incubator-rocketmq.git;h=f508f13 ] ROCKETMQ-107 Fix possible concurrency problem on ServiceState when consumer start/shutdown, closes apache/incubator-rocketmq#68
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user zhouxinyu commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          Merged, thanks @Jaskey , please help close this PR.

          Show
          githubbot ASF GitHub Bot added a comment - Github user zhouxinyu commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 Merged, thanks @Jaskey , please help close this PR.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey commented on the issue:

          https://github.com/apache/incubator-rocketmq/pull/68

          Thank you @zhouxinyu

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey commented on the issue: https://github.com/apache/incubator-rocketmq/pull/68 Thank you @zhouxinyu
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user Jaskey closed the pull request at:

          https://github.com/apache/incubator-rocketmq/pull/68

          Show
          githubbot ASF GitHub Bot added a comment - Github user Jaskey closed the pull request at: https://github.com/apache/incubator-rocketmq/pull/68

            People

            • Assignee:
              Jaskey Jaskey Lam
              Reporter:
              Jaskey Jaskey Lam
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development