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

[Jaskey]

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

No Syncrinations in start or shutdown which may result in :

1. Start twice if two thread starts the same consumer in the same time, which leads to initizize the resources twice.
2. Shutdown may not be effective if thread A start and Thread b tries to shutdown in a very short time after A's starting.

Implemenations:

1. Add lock in getter and setter, and replace all direct read/write operations to getter and setter
2. Add lock before start and shutdown

[Ah39]

1.AtomicReference state = new AtomicReference()
state.compareAndSet

2.volatile

can fix the problem

[Jaskey]

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

[Ah39]

apache/curator TreeCache Start
if treeState.compareAndSet fail may throws Exception
dont need syncronized
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;
}

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[Jaskey]

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

[zhouxinyu]

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.

[shroman]

I also think that using synchronized fits well here.
But I would do it in the following way:

1. Make serviceState volatile

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 ;)

[coveralls]

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

[coveralls]

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

[coveralls]

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

[lizhanhui]

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

[vongosling]

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

[Jaskey]

@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

[shroman]

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 ;)

[Jaskey]

@shroman
Actully setServiceState should be synchroinzed in my opnion, say if thread A 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, thread A 's 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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[coveralls]

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

[shroman]

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

[Jaskey]

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

[zhouxinyu]

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

[Jaskey]

Thank you @zhouxinyu