[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12611754#action_12611754
 ] 

Andrew Kornev commented on ZOOKEEPER-59:
----------------------------------------

I agree with Flavio's comment in 
https://issues.apache.org/jira/browse/ZOOKEEPER-59?focusedCommentId=12611527#action_12611527
 : incInProcess() should be called prior to calling first request processor. 

Also, getInProcess() should be a synchronized method. Even better solution 
would be to make requestsInProcess an atomic integer: there is really no need 
to synchronize on the ZooKeeper instance to update/access the variable.

Having said that, I don't think this will fix the the timeout problem. From 
what I saw in the server logs during my investigation of a similar incident was 
that the server was still receiving and processing client requests (in other 
words, the counters we're trying to fix here didn't cause throttling). The 
problem was that the responses weren't being sent to the client which was 
causing the client to timeout.

> 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: Benjamin Reed
>         Attachments: ZOOKEEPER-59_1.patch, ZOOKEEPER-59_2.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}
> [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.

Reply via email to