yfxhust commented on a change in pull request #1235: ZOOKEEPER-3706:
ZooKeeper.close() would leak SendThread when the netw…
URL: https://github.com/apache/zookeeper/pull/1235#discussion_r379970390
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java
##########
@@ -971,10 +980,20 @@ void readResponse(ByteBuffer incomingBuffer) throws
IOException {
*
* @return
*/
- ZooKeeper.States getZkState() {
+ synchronized ZooKeeper.States getZkState() {
return state;
Review comment:
Thank you for your comments.
checkstyle may report exception if remove `synchronized` from getkState.
Another reason of adding `synchronized` is to prevent future risk if we add
more complex task in setZkState() in future..
In current implementation, we have implicit convention for setZkSate() -
state modification must be placed at the end of setZkState() otherwise
incomplete internal state may be exposed to outside. Adding `synchronized` to
getZkState() can avoid this risk because getZkState() only be allowed to
interleave execution with the whole setZkState().
Anyway I accept your suggestion. Maybe it has potential performance benefit
if I remove `synchronized` from getZkState(). I will add more comments to
clarify the implicit convention of state modification.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
