Fixes #1700: refactor the AMQP link lifecycle #1701
Conversation
This change removes some of the old link attach routing logic and attempts to clean up the link API. The logic that used to track the exchange of Attach/Detach performatives has been simplified. The various counters and booleans maintained by the qdr_link_t structure for tracking this exchange has been reduced to a mask/flag implementation similar to Protons implementation of endpoint state. This patch refactors the link detach adaptor API to be more like the existing AMQP connection API: there is now an explict API call to release the link instance at the end of its lifecycle. The adaptor API is modified by separating the AMQP detach handling from the release of the link instance. The old qdr_link_detach() adaptor function has been refactored into two functions: qdr_link_detach_received() and qdr_link_close(). The qdr_link_detach_received() call is made by the AMQP adaptor when a Detach Peformative has been received by the peer. It is only used by the AMQP adaptor. The new qdr_link_closed() API call is made by all adaptors when the link instance is destroyed. This is similar to the existing qdr_connection_closed() call but for links. It is used by all adaptors to indicate to the core that the link is no longer in use and can be cleaned up. In the case of the AMQP adaptor this call will be made after the link detach handshake has completed. Test coverage by the test-sender AMQP client has been increased by adding a clean connection close function.
| } | ||
| } | ||
|
|
||
| qdr_link_cleanup_protected_CT(core, link->conn, link, |
There was a problem hiding this comment.
Can the qdr_link_cleanup_protected_CT() function be removed and its contents copied into qdr_link_closed_CT()
There was a problem hiding this comment.
Good eye - not quite yet but soon. My next patch will (hopefully) rework that "protected" stuff to remove the whole "Is the core finished? Is the I/O finished? Can it be freed?" flags handshake. Hopefully that qdr_link_cleanup_protected_CT() goes away and we simply have a qdr_link_free_CT() call (like you recommend below).
Stay tuned.
|
Can the Also can we rename the |
| * @param error The link error from the detach frame or 0 if none. | ||
| */ | ||
| void qdr_link_detach(qdr_link_t *link, qd_detach_type_t dt, qdr_error_t *error); | ||
| void qdr_link_detach_received(qdr_link_t *link, qdr_error_t *error); |
There was a problem hiding this comment.
I like renaming the function qdr_link_detach() to qdr_link_detach_received(). The new function name makes it clear that a detach (first or second) has been received from the remote peer.
| * @param link The link pointer returned by qdr_link_first_attach or in a FIRST_ATTACH event. | ||
| * @param forced True if the link was closed due to failure or shutdown. False if closed by clean detach handshake. | ||
| */ | ||
| void qdr_link_closed(qdr_link_t *link, bool forced); |
There was a problem hiding this comment.
I looked for the usage of this function in the tcp adaptor, say in close_connection_XSIDE_IO() -
if (!!conn->outbound_link) {
qdr_link_closed(conn->outbound_link, true);
}
In the above usage, we are calling the qdr_link_closed() function to close a link. Should the function be called qdr_link_close() or qd_close_link() instead ? closed seems to mean that the link has already been closed
There was a problem hiding this comment.
closed seems to mean that the link has already been closed
Indeed that's the idea: the adaptor has detected the link is closed and needs to notify the core that the link no longer exists (hence qdr_link_closed rather than qd_link_closed: it's a Core API call not an adaptor call).
I chose qdr_link_closed because the function performs essentially the same thing as the exising core function qdr_connection_closed except the new one deals with qdr_link_t's not qdr_connection_t's).
You'll see that both adaptors call qdr_connection_close() right before they clean up their connection state objects (qd_tcp_connection_t or qd_connection_t - depending on the adaptor). The TCP adaptor used to call qdr_link_detach() even though there's not such thing as a detach performative for TCP! For TCP we basically simulate a network connection drop since there's no AMQP close handshake going on.
What do you think?
| * This is the last callback for the given link - the link will be freed on return from this call! Forced is true if the | ||
| * link has not properly closed (detach handshake completed). | ||
| */ | ||
| static void AMQP_link_closed_handler(qd_router_t *router, qd_link_t *qd_link, bool forced) |
There was a problem hiding this comment.
I was wondering if the AMQP_ prefix should be used only for functions that are called directly as a response to proton AMQP events. This function can be called qd_link_close() ?
Yes I think that's a great name change. |
This change removes some of the old link attach routing logic and attempts to clean up the link API.
The logic that used to track the exchange of Attach/Detach performatives has been simplified. The various counters and booleans maintained by the qdr_link_t structure for tracking this exchange has been reduced to a mask/flag implementation similar to Protons implementation of endpoint state.
This patch refactors the link detach adaptor API to be more like the existing AMQP connection API: there is now an explict API call to release the link instance at the end of its lifecycle.
The adaptor API is modified by separating the AMQP detach handling from the release of the link instance. The old qdr_link_detach() adaptor function has been refactored into two functions: qdr_link_detach_received() and qdr_link_close().
The qdr_link_detach_received() call is made by the AMQP adaptor when a Detach Peformative has been received by the peer. It is only used by the AMQP adaptor.
The new qdr_link_closed() API call is made by all adaptors when the link instance is destroyed. This is similar to the existing qdr_connection_closed() call but for links. It is used by all adaptors to indicate to the core that the link is no longer in use and can be cleaned up. In the case of the AMQP adaptor this call will be made after the link detach handshake has completed.
Test coverage by the test-sender AMQP client has been increased by adding a clean connection close function.