[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Flavio Paiva Junqueira updated ZOOKEEPER-59: -------------------------------------------- Attachment: ZOOKEEPER-59.patch I have updated the patch in the following way: * I have changed synchronization blocks to reflect the discussion of this issue. In the trunk code, there is at least one instance increment outstandingRequests that is not synchronized with "this". I have also tried to make sure that all necessary blocks of code that update the SelectionKey are synchronized with factory. * I have not included the change of ZooKeeperServer that corresponds to making requestsInProcess an AtomicInteger. This is really a minor change, and not strictly necessary. Ben made this comment that the methods of SelectionKey are thread-safe. However, I believe the synchronization blocks are there to make the block of code atomic, and not because of the individual calls. As mentioned above, I have tried to make sure that all blocks are properly synchronized. I'm not exactly sure, though, why some of the blocks related to SelectionKey operations are synchronized. I have the impression that they have been introduced conservatively, but I would appreciate if someone with an opinion could shed some light here. > 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 > Assignee: Flavio Paiva Junqueira > Fix For: 3.3.0 > > Attachments: ZOOKEEPER-59.patch, ZOOKEEPER-59.patch > > > 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} > nioservercnxn.readrequ...@444 > ... > 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} > nioservercnxn.sendrespo...@740 > ... > 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.