ted-ross commented on a change in pull request #1018:
URL: https://github.com/apache/qpid-dispatch/pull/1018#discussion_r570488954
##########
File path: src/router_core/transfer.c
##########
@@ -219,14 +217,32 @@ int qdr_link_process_deliveries(qdr_core_t *core,
qdr_link_t *link, int credit)
dlv->where = QDR_DELIVERY_IN_UNSETTLED;
qd_log(core->log, QD_LOG_DEBUG, DLV_FMT"Delivery
transfer: qdr_link_process_deliveries: undelivered-list -> unsettled-list",
DLV_ARGS(dlv));
}
+ } else {
+ //
+ // This delivery is in the process of being transfered
+ // to a different link. Hack: the adaptor
+ // deliver_handler has issued a new link-attached
+ // action to the core, passing this dlv as the initial
Review comment:
It's possible also that this hasn't happened yet. In the TCP adaptor,
the creation of the new link with the initial-delivery is deferred to a
different IO thread context (because that link is on a different connection).
##########
File path: src/router_core/transfer.c
##########
@@ -219,14 +217,32 @@ int qdr_link_process_deliveries(qdr_core_t *core,
qdr_link_t *link, int credit)
dlv->where = QDR_DELIVERY_IN_UNSETTLED;
qd_log(core->log, QD_LOG_DEBUG, DLV_FMT"Delivery
transfer: qdr_link_process_deliveries: undelivered-list -> unsettled-list",
DLV_ARGS(dlv));
}
+ } else {
+ //
+ // This delivery is in the process of being transfered
+ // to a different link. Hack: the adaptor
+ // deliver_handler has issued a new link-attached
+ // action to the core, passing this dlv as the initial
Review comment:
I don't think that's needed. The MOVED_TO_NEW_LINK tells the core that
the delivery has been referenced for use as the initial delivery on a different
link. The core (qdr_process_deliveries) should use this information to remove
the delivery (complete or not) from the undelivered list because it is safely
referenced elsewhere. Just keep in mind that the delivery's new link may have
already move it by the time MOVED_TO_NEW_LINK is seen.
There are two places where the removal from undelivered can happen: in
qdr_process_deliveries when MOVED_TO_NEW_LINK is returned, and in
first_attach_CT when the delivery is supplied to a new link. Exactly one of
these must perform the switcheroo.
##########
File path: src/router_core/transfer.c
##########
@@ -219,14 +217,32 @@ int qdr_link_process_deliveries(qdr_core_t *core,
qdr_link_t *link, int credit)
dlv->where = QDR_DELIVERY_IN_UNSETTLED;
qd_log(core->log, QD_LOG_DEBUG, DLV_FMT"Delivery
transfer: qdr_link_process_deliveries: undelivered-list -> unsettled-list",
DLV_ARGS(dlv));
}
+ } else {
+ //
+ // This delivery is in the process of being transfered
+ // to a different link. Hack: the adaptor
+ // deliver_handler has issued a new link-attached
+ // action to the core, passing this dlv as the initial
+ // delivery on that new link. When the core gets
+ // around to processing that action it will move the
+ // dlv from this link to the new link. Problem: that
+ // _may_ have already happened before the lock was
+ // taken. This is a race. To work-around, check if the
+ // dlv is still on the current link and if so remove
+ // it. Once the core processes the link attach it will
+ // move the dlv to the new link.
+ //
+ assert(new_disp == QD_DELIVERY_MOVED_TO_NEW_LINK);
Review comment:
srsly?
##########
File path: src/router_core/transfer.c
##########
@@ -219,14 +217,32 @@ int qdr_link_process_deliveries(qdr_core_t *core,
qdr_link_t *link, int credit)
dlv->where = QDR_DELIVERY_IN_UNSETTLED;
qd_log(core->log, QD_LOG_DEBUG, DLV_FMT"Delivery
transfer: qdr_link_process_deliveries: undelivered-list -> unsettled-list",
DLV_ARGS(dlv));
}
+ } else {
+ //
+ // This delivery is in the process of being transfered
+ // to a different link. Hack: the adaptor
+ // deliver_handler has issued a new link-attached
+ // action to the core, passing this dlv as the initial
Review comment:
To clarify... it already is a two step process. Returning MOVED_TO...
is the dissociation and first_attach(..., initial_dlv, ...) is the
re-association. The issue is that these steps can occur in either order. In
fact, they do: HTTP2 does re-associate before dissociate and TCP does it the
other way around. In both cases, there are races between threads that can
cause reversals.
----------------------------------------------------------------
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]
