ZOOKEEPER-3652: Synchronize ClientCnxn outgoing queue flush on a stable internal value

[swallez]

When packets are added to ClientCnxn's outgoing packet queue we ensure there's no conflict with an ongoing flush of that queue because of connection loss.

Synchronization used to be on the state field's value. This value is both not stable (its value changes over time), possibly causing improper synchronization, and global, which can cause contention in applications that run several ZooKeeper clients.

We now synchronize on outgoingQueue which is both local to a ClientCnxn's instance and stable.

[maoling]

@swallez

• [ERROR] Synchronization performed on java.util.concurrent.LinkedBlockingDeque in org.apache.zookeeper.ClientCnxn.queuePacket(RequestHeader, ReplyHeader, Record, Record, AsyncCallback, String, String, Object, ZooKeeper$WatchRegistration, WatchDeregistration) [org.apache.zookeeper.ClientCnxn] At ClientCnxn.java:[line 1645]

• JLM_JSR166_UTILCONCURRENT_MONITORENTER: This method performs synchronization an object that is an instance of a class from the java.util.concurrent package  (or its subclasses). Instances of these classes have their own concurrency  control mechanisms that are orthogonal to the synchronization provided by the  Java keyword synchronized. For example, synchronizing on an AtomicBoolean  will not prevent other threads from modifying the AtomicBoolean. Such code may be correct, but should be  carefully reviewed and documented, and may confuse people who have to  maintain the code at a later date.

[swallez]

@maoling I added a commit to instruct SpotBugs that we know what we're doing, following the approach used to allow synchronized (waitingEvents) in that same class.

[maoling]

looks good. I still have another thought(no binding for the following, you can ignore it):

• Since the root cause is a narrow windows between queuePacket and cleanup, so synchronized objectLock is also an alternative way? which one is better? Since outgoingQueue is a critical Queue for client to talk with server, synchronized outgoingQueue will have performance and future program extensibility issue?

Haha, I also test for the global and inner wording by the following way:

• new two different zookeeper clients, create some znodes, printing the hashcode of outgoingQueue and state. They really hold different outgoingQueue, but the same hashcode of state. it really confuses me.

• I believe different clients will have different state instance, otherwise when one client calls close()(set state to CLOSE), it will affect another client. However, using following ways cannot reason about it.

System.out.println(zk1.getState() == zk2.getState()); //true
System.out.println(zk1.getState().equals(zk2.getState())); //true
System.out.println(zk1.getState().hashCode() == zk2.getState().hashCode()); //true
System.out.println(System.identityHashCode(zk1.getState()) == System.identityHashCode(zk2.getState().hashCode()); //true

javap to see the bytecode, the value set to state is public static final, so it's really global-shared by multi-clients.

public final class org.apache.zookeeper.States extends java.lang.Enum<org.apache.zookeeper.States> {
  public static final org.apache.zookeeper.States CONNECTING;
  public static final org.apache.zookeeper.States NOT_CONNECTED;
  ********************************************
  private static final org.apache.zookeeper.States[] $VALUES;
  public static org.apache.zookeeper.States[] values();

• synchronized Enum is also not thread-safe. Look at my demo attached in JIRA.
• In a word, Enum is a heresy:)

[swallez]

Thanks for the review @maoling!

Synchronizing on outgoingQueue or a separate objectLock has the exact same performance since no other synchronization statement exists for outgoingQueue, including in its implementation (I checked the code for LinkedBlockingDeque).

In terms of future extensibility I don't think this is a concern as outgoingQueue is used in very few places, which are where synchronization is needed.

And yes, enum is great to represent enumerated values but not so great for thread synchronization 😉

[arshadmohammad]

LGTM +1

[arshadmohammad]

LGTM +1

[eolivelli]

Can you please revase this patch into latest master?

[swallez]

@eolivelli rebase done.

The pr-merge job failed on zookeeper-client-c because of a missing C header file, unrelated to this change.

[laxmanprabhu]

The cleanUp method is also called by cleanAndNotifyState(). Is there a reason why cleanUp method call isn't synchronized in this code path as well.

When cleanAndNotifyState() calls cleanUp, the outgoingQueue is iterated without any synchronization. During this stage more callers can queue requests to the outgoingQueue. This causes the cleanUp method to keep iterating and possible take long time to complete.