[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12609250#action_12609250 ]
Benjamin Reed commented on ZOOKEEPER-59: ---------------------------------------- There are actually two things to guard here: the outstandingBuffers and the selector loop. In read request, we really just need: synchronized (this) { outstandingRequests++; } In sendResponse we need to synchronize on this for outstandingRequests and on factory for the selector: synchronized (this) { outstandingRequests--; } synchronized (this.factory) { // check throttling if (zk.getInProcess() < factory.outstandingLimit || outstandingRequests < 1) { sk.selector().wakeup(); enableRecv(); } } I don't think the synchronize block is needed in doIO. The SectionKey methods are supposed to be thread safe. I think you might have found something! If the outstandingRequests count gets messed up, bad things will happen including the server becoming unresponsive to a client. > Synchronized block in NIOServerCnxn > ----------------------------------- > > Key: ZOOKEEPER-59 > URL: https://issues.apache.org/jira/browse/ZOOKEEPER-59 > Project: Zookeeper > Issue Type: Bug > Components: server > Reporter: Flavio Paiva Junqueira > > There are two synchronized blocks locking on different objects, and to me > they should be guarded by the same object. Here are the parts of the code I'm > talking about: > {noformat} > [EMAIL PROTECTED] > ... > synchronized (this) { > outstandingRequests++; > // check throttling > if (zk.getInProcess() > factory.outstandingLimit) { > disableRecv(); > // following lines should not be needed since we are > already > // reading > // } else { > // enableRecv(); > } > } > {noformat} > {noformat} > [EMAIL PROTECTED] > ... > synchronized (this.factory) { > outstandingRequests--; > // check throttling > if (zk.getInProcess() < factory.outstandingLimit > || outstandingRequests < 1) { > sk.selector().wakeup(); > enableRecv(); > } > } > {noformat} > I think the second one is correct, and the first synchronized block should be > guarded by "this.factory". > This could be related to issue ZOOKEEPER-57, but I have no concrete > indication that this is the case so far. -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.