zookeeper-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [zookeeper] xoiss commented on issue #999: ZOOKEEPER-2891: Invalid processing of zookeeper_close for mutli-request
Date Tue, 02 Jul 2019 14:16:44 GMT
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

Mime
View raw message