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