> On Dec. 11, 2014, 11:16 p.m., fpj wrote: > > src/java/main/org/apache/zookeeper/ClientCnxn.java, line 134 > > <https://reviews.apache.org/r/27244/diff/28/?file=789547#file789547line134> > > > > It sounds like making pendingQueue and outgoingQueue are optimizations > > that are independent from this issue. Shouldn't we make this change in a > > different jira? > > Hongchao Deng wrote: > outgoingQueue isn't optimization... My previous patch used a semaphore > which will be waken up by WakeupCnxn(). Your suggestion is use a blocking > outgoingQueue. What do you think? I am fine since I know both changes well > and I can do it in separate JIRAs. > > fpj wrote: > My concern here is that it ended up implying more changes than just > removing the semaphore. For example, the patch removes this synchronized > block (not sure it is going to format it right): > > for (Packet p : outgoingQueue) { > conLossPacket(p); > } > outgoingQueue.clear(); > > and checking the other synchronized blocks removed, it isn't entirely > straightforward that you can remove all of them. > > Hongchao Deng wrote: > I can add those synchronized back. However, findbugs will complain that > so I will add rules too. > > On Netty side I can assure your concern: > 1. In ClientCnxn, primeConnection(), cleanup(), queuePacket(). > primeConnection(), cleanup() is guaranteed to be start and end of life > cycles. queuePacket() is appending packets to outgoingQueue and waking up the > blocking queue. > > > What do you think? > > fpj wrote: > I'm not suggesting that you bring back the synchronized blocks. I'm > saying that I haven't checked carefully and it isn't straightforward that you > can remove them. For example, the one in ClientCnxnSocketNIO line 163 has > multiple accesses to outgoingQueue, so removing the synchronized block > doesn't necessarily gives us the same behavior. Can you actually justify the > removal of each one of those synchronized blocks? If you aren't sure, then > I'll check it carefully. > > Hongchao Deng wrote: > That would be great if you can check it. Actually I just found a possible > race. > > Here's my version of analysis: > 1. primeConnection: The start of life cycle. No one else can touch > outgoingQueue except queuePacket(). primeConn prepends packets while > queuePacket appends. > 2. !!cleanup: The end of life cycle. No one else except queuePacket(). A > possible race with queuePacket() in check closing and add packets. I will fix > it shortly. > 3. queuePacket: ... > 4. findSendablePacket and doIO: this is the only place that remove items > from outgoing Queue > - the check in findSendablePacket() guarantees not empty. Because no > where will remove items > - enumeration will give a consistent current view. Adding new packets > doesn't count in. > - found non-priming packet, remove and addFirst wouldn't have race > because here only queuePacket() will be called. And queuePacket() appends > only. > 5. last check in doTransport: this is duplicate..
I just realize between cleanup() and queuePacket() there is race regardless of the synchronized.. Even though cleanup() finishes, it doesn't guarantee that state is CLOSED or closeSession packet was sent. Thus queuePacket() doesn't call conLossPacket(). - Hongchao ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27244/#review64807 ----------------------------------------------------------- On Dec. 12, 2014, 7:16 p.m., Hongchao Deng wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27244/ > ----------------------------------------------------------- > > (Updated Dec. 12, 2014, 7:16 p.m.) > > > Review request for zookeeper. > > > Repository: zookeeper-git > > > Description > ------- > > ZOOKEEPER-2069 > > > Diffs > ----- > > build.xml bb5ff4f > src/java/main/org/apache/zookeeper/ClientCnxn.java b4ece07 > src/java/main/org/apache/zookeeper/ClientCnxnSocket.java 5ca0ba7 > src/java/main/org/apache/zookeeper/ClientCnxnSocketNIO.java adb27ee > src/java/main/org/apache/zookeeper/ClientCnxnSocketNetty.java PRE-CREATION > src/java/main/org/apache/zookeeper/ZooKeeperTestable.java 775d1a2 > src/java/main/org/apache/zookeeper/client/ZooKeeperSaslClient.java dbc1080 > src/java/test/org/apache/zookeeper/test/ClientTest.java dbe595c > src/java/test/org/apache/zookeeper/test/NettyNettySuiteBase.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteHammerTest.java > PRE-CREATION > src/java/test/org/apache/zookeeper/test/NettyNettySuiteTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/27244/diff/ > > > Testing > ------- > > 1. use LinkedBlockingDeque. > > > Thanks, > > Hongchao Deng > >
