[GitHub] [zookeeper] xoiss commented on a change in pull request #999: ZOOKEEPER-2891: Invalid processing of zookeeper_close for mutli-request

[GitBox <gi...@apache.org>]

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(
     case COMPLETION_VOID:
         break;
     case COMPLETION_MULTI:
-        sc->rc = deserialize_multi(cptr->xid, cptr, ia);
+        sc->rc = deserialize_multi(cptr->xid, cptr, ia, sc->rc);
         break;
     default:
         LOG_DEBUG(("Unsupported completion type=%d", cptr->c.type));
         break;
     }
 }
 
-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), STRUCT_INITIALIZER(err , 0) };
     assert(clist);
     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 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