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 a change in pull request #999: ZOOKEEPER-2891: Invalid processing of zookeeper_close for mutli-request
Date Mon, 29 Jul 2019 17:16:47 GMT
xoiss commented on a change in pull request #999: ZOOKEEPER-2891: Invalid processing of zookeeper_close
for mutli-request
URL: https://github.com/apache/zookeeper/pull/999#discussion_r308342478

 File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
 @@ -2010,24 +2010,28 @@ static void process_sync_completion(
-        sc->rc = deserialize_multi(cptr->xid, cptr, ia);
+        sc->rc = deserialize_multi(cptr->xid, cptr, ia, sc->rc);
         LOG_DEBUG(("Unsupported completion type=%d", cptr->c.type));
-static int deserialize_multi(int xid, completion_list_t *cptr, struct iarchive *ia)
+static int deserialize_multi(int xid, completion_list_t *cptr, struct iarchive *ia, int rc_hint)
-    int rc = 0;
+    int rc = rc_hint;
     completion_head_t *clist = &cptr->c.clist;
     struct MultiHeader mhdr = { STRUCT_INITIALIZER(type , 0), STRUCT_INITIALIZER(done , 0),
     deserialize_MultiHeader(ia, "multiheader", &mhdr);
     while (!mhdr.done) {
         completion_list_t *entry = dequeue_completion(clist);
-        assert(entry);
+        if (rc_hint == ZOK) {
+            assert(entry);
+        } else if (entry == NULL) {
+            break;
 Review comment:
   > 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
   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:

With regards,
Apache Git Services

View raw message