[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn
[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12842403#action_12842403 ] Hadoop QA commented on ZOOKEEPER-59: -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12438130/ZOOKEEPER-59.patch against trunk revision 919706. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/135/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/135/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/135/console This message is automatically generated. 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, 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.
[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn
[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12842248#action_12842248 ] Flavio Paiva Junqueira commented on ZOOKEEPER-59: - As I already mentioned above, I don't think it requires a test. Ben, could you please check my previous post to see if you agree? 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.
[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn
[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12842281#action_12842281 ] Benjamin Reed commented on ZOOKEEPER-59: ah yes, i see what you mean, but i think i would prefer a outstandingRequests that might be stale to a possible deadlock. i agree that this doesn't need a test. this is a latent bug that is hard to be reproduced and a test would be very specific to the implementation of the bug. 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.
[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn
[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12842186#action_12842186 ] Hadoop QA commented on ZOOKEEPER-59: -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12437656/ZOOKEEPER-59.patch against trunk revision 919640. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/131/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/131/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/131/console This message is automatically generated. 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.
[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn
[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12840735#action_12840735 ] Benjamin Reed commented on ZOOKEEPER-59: i like how you removed the nested locking, but it looks like you missed an opportunity in the last block. don't you really want to {noformat} synchronized(this) {outstandingRequests--;} {noformat} so that you aren't locking this and this.factory? 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.
[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn
[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12840800#action_12840800 ] Flavio Paiva Junqueira commented on ZOOKEEPER-59: - Good point, Ben. I haven't done it because of the predicate: {noformat} outstandingRequests 1 {noformat} in that block. 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.
[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn
[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12840464#action_12840464 ] Hadoop QA commented on ZOOKEEPER-59: -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12437656/ZOOKEEPER-59.patch against trunk revision 915956. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/125/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/125/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h8.grid.sp2.yahoo.net/125/console This message is automatically generated. 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.
[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn
[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12800933#action_12800933 ] Mahadev konar commented on ZOOKEEPER-59: flavio, will you be updating this patch? 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 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.
[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn
[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12763567#action_12763567 ] Patrick Hunt commented on ZOOKEEPER-59: --- Hi Flavio, what's the status of this patch? 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 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.
[jira] Commented: (ZOOKEEPER-59) Synchronized block in NIOServerCnxn
[ https://issues.apache.org/jira/browse/ZOOKEEPER-59?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12611796#action_12611796 ] Flavio Paiva Junqueira commented on ZOOKEEPER-59: - Mahadev, Thanks for fixing the patch. My eclipse still hasn't leaned how to generate diffs appropriately. Your assessment of the patch is right, and the observation on outstandingRequests seems correct to me. Andrew, I agree with you on getInProcess(). It is probably best to make it synchronized or simply to use an atomic integer for requestsInProcess. Now, on your description of the connection problem, I think this is an important observation because when I observed the problem, the log messages did not show the server receiving anything. Also, if the server receives and we don't disable sending, then either ping requests are not going through the pipeline of request processors (once it gets to FinalRequestProcessor, the server sends a response immediately) or sendResponse is not doing its job. 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.