You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@rocketmq.apache.org by Jaskey <gi...@git.apache.org> on 2017/02/23 03:20:30 UTC
[GitHub] incubator-rocketmq pull request #68: [ROCKETMQ-107] fix possible concurrency...
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 <li...@gmail.com>
Date: 2017-02-23T03:06:32Z
[ROCKETMQ-107] fix possible concurrency problem on ServiceState when consumer start/shutdown
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/68
[](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**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/68
[](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**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by shroman <gi...@git.apache.org>.
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 ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/68
[](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**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/68
Thank you @zhouxinyu
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by lizhanhui <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by Ah39 <gi...@git.apache.org>.
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;
}
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by Ah39 <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by Jaskey <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/68
[](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**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by shroman <gi...@git.apache.org>.
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_.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/68
[](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**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by shroman <gi...@git.apache.org>.
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 ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/68
[](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**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by Jaskey <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by Jaskey <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq pull request #68: [ROCKETMQ-107] fix possible concurrency...
Posted by Jaskey <gi...@git.apache.org>.
Github user Jaskey closed the pull request at:
https://github.com/apache/incubator-rocketmq/pull/68
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by vongosling <gi...@git.apache.org>.
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 :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by Jaskey <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by zhouxinyu <gi...@git.apache.org>.
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by Jaskey <gi...@git.apache.org>.
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/68
[](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**.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-rocketmq issue #68: [ROCKETMQ-107] fix possible concurrency proble...
Posted by zhouxinyu <gi...@git.apache.org>.
Github user zhouxinyu commented on the issue:
https://github.com/apache/incubator-rocketmq/pull/68
Merged, thanks @Jaskey , please help close this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---