[GitHub] [qpid-dispatch] kgiusti commented on a change in pull request #725: DISPATCH-1626 - Modified AMQP_rx_handler to call pn_delivery_update()…

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

kgiusti commented on a change in pull request #725:
URL: https://github.com/apache/qpid-dispatch/pull/725#discussion_r414057678



##########
File path: src/router_node.c
##########
@@ -1829,11 +1838,16 @@ static void CORE_delivery_update(void *context, qdr_delivery_t *dlv, uint64_t di
         // If the delivery is still arriving, don't push out the disposition change yet.
         //
         if (qd_message_receive_complete(msg)) {
-            pn_delivery_update(pnd, disp);
+            if (disp != pn_delivery_local_state(pnd)) {
+                pn_delivery_update(pnd, disp);
+                if (qdr_delivery_disposition(dlv) != disp)

Review comment:
       This should not be necessary (and it's a race): The disp passed into this function comes from the qdr_delivery_t so this check and set is a no-op.  I would remove these.

##########
File path: src/router_node.c
##########
@@ -1829,11 +1838,16 @@ static void CORE_delivery_update(void *context, qdr_delivery_t *dlv, uint64_t di
         // If the delivery is still arriving, don't push out the disposition change yet.
         //
         if (qd_message_receive_complete(msg)) {
-            pn_delivery_update(pnd, disp);
+            if (disp != pn_delivery_local_state(pnd)) {
+                pn_delivery_update(pnd, disp);
+                if (qdr_delivery_disposition(dlv) != disp)
+                    qdr_delivery_set_disposition(dlv, disp);
+            }
         } else {
             // just update the local disposition for now - AMQP_rx_handler will
             // write this to proton once the message is fully received.
-            qdr_delivery_set_disposition(dlv, disp);
+            if (qdr_delivery_disposition(dlv) != disp)

Review comment:
       Same here - this should not be necessary.  You can replace these with an assert(qdr_delivery_disposition(dlv) == disp) to ensure this routine isn't called inappropriately just to be safe.

##########
File path: src/router_node.c
##########
@@ -349,8 +349,17 @@ static bool AMQP_rx_handler(void* context, qd_link_t *link)
             next_delivery = pn_link_current(pn_link) != 0;
 
             uint64_t local_disp = qdr_delivery_disposition(delivery);
-            if (local_disp != 0) {
-                pn_delivery_update(pnd, local_disp);
+            //
+            // Call pn_delivery_update only if the local disposition is different than the pn_delivery's local disposition.
+            // This will make sure we call pn_delivery_update only when necessary.
+            //
+            if (local_disp != 0 && local_disp != pn_delivery_local_state(pnd)) {
+                //
+                // DISPATCH-1626 - This enables pn_delivery_update() and pn_delivery_settle() to be called back to back in the same function call.
+                // CORE_delivery_update() will handle most of the other cases where we need to call pn_delivery_update() followed by pn_delivery_settle().
+                //
+                if (qd_message_is_discard(msg))

Review comment:
       If the only point where the pn_delivery_update() needs to be done is if qd_message_is_discard(), then why not move all this dispo logic into the qd_message_is_discard(msg) logic right before the qd_delivery_settle() is done (linke #383)




----------------------------------------------------------------
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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org