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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
