From notifications-return-601-archive-asf-public=cust-asf.ponee.io@zookeeper.apache.org Tue Jul 2 14:16:45 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 49F2818060E for ; Tue, 2 Jul 2019 16:16:45 +0200 (CEST) Received: (qmail 54182 invoked by uid 500); 2 Jul 2019 14:16:44 -0000 Mailing-List: contact notifications-help@zookeeper.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zookeeper.apache.org Delivered-To: mailing list notifications@zookeeper.apache.org Received: (qmail 54173 invoked by uid 99); 2 Jul 2019 14:16:44 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 02 Jul 2019 14:16:44 +0000 From: GitBox To: notifications@zookeeper.apache.org Subject: [GitHub] [zookeeper] xoiss commented on issue #999: ZOOKEEPER-2891: Invalid processing of zookeeper_close for mutli-request Message-ID: <156207700427.1936.10116472874798770605.gitbox@gitbox.apache.org> Date: Tue, 02 Jul 2019 14:16:44 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit xoiss commented on issue #999: ZOOKEEPER-2891: Invalid processing of zookeeper_close for mutli-request URL: https://github.com/apache/zookeeper/pull/999#issuecomment-507697121 > add a test to demonstrate the problem and verify that we have fixed it Done! I have provided two tests: - `testCloseWhileMultiInProgressFromMain` - `testCloseWhileMultiInProgressFromCompletion` The reason to have namely two tests is about the same as for ZOOKEEPER-2894 (see https://github.com/apache/zookeeper/pull/1000). So, it's just two subcases of the same problem. **Note:** Right now, the first test of two is temporarily disabled because it requires fixes from ZOOKEEPER-2894. When it's merged, I'll rebase this PR and enable this test. Ok, BEFORE this patch both tests fail with `src/zookeeper.c:2033: deserialize_multi: Assertion 'entry' failed` (for the first test, assuming ZOOKEEPER-2894 is already merged, otherwise it fails with completion lost). And after the patch both tests pass successfully (again, for the first test, assuming ZOOKEEPER-2894 is already merged). --- > Can you explain why this is the right point to exit the loop and the method? What is the alternative?? Please, read this: https://issues.apache.org/jira/browse/ZOOKEEPER-2891 I have revised it again now, and indeed, I don't have more detailed explanation. The best is try to reproduce this under debugger. You may use unit-tests published here. If you still have questions, please, formulate more specific question, or a use-case (scenario) that I can reproduce. > To my eyes, it is cleaner to have rc_hint == ZOK as a conditional on the while statement or perhaps a return rc down a few lines where it is set on error. If we put `rc_hint == ZOK` directly into the `while`-conditional-expression then we have to put there also (1) assignment of `entry` and (2) analyzing the value assigned to `entry` -- i.e., two code lines: `completion_list_t *entry = dequeue_completion(clist);` and `if (entry == NULL)`. Note also that `entry` is a local variable of the while-loop-body, and C99 does not allow local variable declarations right in the while-expression. So, due to this, it's better to keep things at there original places. I understand that having additional loop-termination point is not good enough, but sometimes it's a kinda tradeoff. I wish to keep things as close to the code-before-patch as possible, to minimize the diff. > Could we also return immediately from the method is rc_hint is not 0? Currently 'yes', because there is nothing between the while-loop-body-end and the final `return` statement. But there is no lifetime warranty that nothing would be put here between them. Actually, I prefer `break` here because it performs the minimum of the necessary work -- I'm talking about the "Necessity and Sufficiency" -- here `return` is sufficient, but much more than necessary. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: users@infra.apache.org With regards, Apache Git Services