[Asterisk-code-review] res_pjsip_pubsub: subscription cleanup changes (asterisk[20])
Michael Bradeen
asteriskteam at digium.com
Thu Mar 30 13:41:48 CDT 2023
Michael Bradeen has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/20017 )
Change subject: res_pjsip_pubsub: subscription cleanup changes
......................................................................
res_pjsip_pubsub: subscription cleanup changes
There are two main parts of the change associated with this
commit. These are driven by the change in call order of
pubsub_on_rx_refresh and pubsub_on_evsub_state by pjproject
when an in-dialog SUBSCRIBE is received.
First, the previous behavior was for pjproject to call
pubsub_on_rx_refresh before calling pubsub_on_evsub_state
when an in-dialog SUBSCRIBE was received that changes the
subscription state.
If that change was a termination due to a re-SUBSCRIBE with
an expires of 0, we used to use the call to pubsub_on_rx_refresh
to set the substate of the evsub to TERMINATE_PENDING before
pjproject could call pubsub_on_evsub_state.
This substate let pubsub_on_evsub_state know that the
subscription TERMINATED event could be ignored as there was
still a subsequent NOTIFY that needed to be generated and
another call to pubsub_on_evsub_state to come with it.
That NOTIFY was sent via serialized_pubsub_on_refresh_timeout
which would see the TERMINATE_PENDING state and transition it
to TERMINATE_IN_PROGRESS before triggering another call to
pubsub_on_evsub_state (which now would clean up the evsub.)
The new pjproject behavior is to call pubsub_on_evsub_state
before pubsub_on_rx_refresh. This means we no longer can set
the state to TERMINATE_PENDING to tell pubsub_on_evsub_state
that it can ignore the first TERMINATED event.
To handle this, we now look directly at the event type,
method type and the expires value to determine whether we
want to ignore the event or use it to trigger the evsub
cleanup.
Second, pjproject now expects the NOTIFY to actually be sent
during pubsub_on_rx_refresh and avoids the protocol violation
inherent in sending a NOTIFY before the SUBSCRIBE is
acknowledged by caching the sent NOTIFY then sending it
after responding to the SUBSCRIBE.
This requires we send the NOTIFY using the non-serialized
pubsub_on_refresh_timeout directly and let pjproject handle
the protocol violation.
ASTERISK-30469
Change-Id: I05c1d91a44fe28244ae93faa4a2268a3332b5fd7
---
M res/res_pjsip_pubsub.c
1 file changed, 108 insertions(+), 49 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/17/20017/1
diff --git a/res/res_pjsip_pubsub.c b/res/res_pjsip_pubsub.c
index bf07d5a..6ddb2fd 100644
--- a/res/res_pjsip_pubsub.c
+++ b/res/res_pjsip_pubsub.c
@@ -3773,29 +3773,29 @@
/*!
* \brief Callback sequence for subscription terminate:
*
+ * * Please note that the descriptions below represent pjproject behavior on versions
+ * >= 2.13.
* * Client initiated:
* pjproject receives SUBSCRIBE on the subscription's serializer thread
+ * calls pubsub_evsub_set_state with state = TERMINATED
+ * pubsub_on_evsub_state checks the event and finds it is due to a received
+ * SUBSCRIBE with an expires of 0 and so does nothing.
* calls pubsub_on_rx_refresh with dialog locked
* pubsub_on_rx_refresh sets TERMINATE_PENDING
- * pushes serialized_pubsub_on_refresh_timeout
+ * calls pubsub_on_refresh_timeout to push final NOTIFY to pjproject
+ * checks state == TERMINATE_PENDING
+ * sets TERMINATE_IN_PROGRESS
+ * calls send_notify (2)
+ * send_notify ultimately calls pjsip_evsub_send_request
+ * pjsip_evsub_send_request calls evsub's set_state
+ * set_state calls pubsub_evsub_set_state
+ * pubsub_on_evsub_state checks state == TERMINATE_IN_PROGRESS
+ * removes the subscriptions
+ * cleans up references to evsub
+ * sets state = TERMINATED
+ * pubsub_on_refresh_timeout unlocks dialog
* returns to pjproject
- * pjproject calls pubsub_on_evsub_state
- * pubsub_evsub_set_state checks state == TERMINATE_IN_PROGRESS (no)
- * ignore and return
* pjproject unlocks dialog
- * serialized_pubsub_on_refresh_timeout starts (1)
- * locks dialog
- * checks state == TERMINATE_PENDING
- * sets TERMINATE_IN_PROGRESS
- * calls send_notify (2)
- * send_notify ultimately calls pjsip_evsub_send_request
- * pjsip_evsub_send_request calls evsub's set_state
- * set_state calls pubsub_evsub_set_state
- * pubsub_on_evsub_state checks state == TERMINATE_IN_PROGRESS
- * removes the subscriptions
- * cleans up references to evsub
- * sets state = TERMINATED
- * serialized_pubsub_on_refresh_timeout unlocks dialog
*
* * Subscription timer expires:
* pjproject timer expires
@@ -3806,8 +3806,20 @@
* pushes serialized_pubsub_on_refresh_timeout
* returns to pjproject
* pjproject unlocks dialog
- * serialized_pubsub_on_refresh_timeout starts
- * See (1) Above
+ * serialized_pubsub_on_refresh_timeout starts (1)
+ * locks dialog
+ * checks state == TERMINATE_PENDING
+ * sets TERMINATE_IN_PROGRESS
+ * calls send_notify (2)
+ * send_notify ultimately calls pjsip_evsub_send_request
+ * pjsip_evsub_send_request calls evsub's set_state
+ * set_state calls pubsub_evsub_set_state
+ * pubsub_on_evsub_state checks state == TERMINATE_IN_PROGRESS
+ * checks that the event is not due to un-SUBSCRIBE
+ * removes the subscriptions
+ * cleans up references to evsub
+ * sets state = TERMINATED
+ * serialized_pubsub_on_refresh_timeout unlocks dialog
*
* * Transmission failure sending NOTIFY or error response from client
* pjproject transaction timer expires or non OK response
@@ -3839,20 +3851,14 @@
*/
-/* The code in this function was previously in pubsub_on_evsub_state. As of
- * pjsip 2.13 pubsub_on_evsub_state is called before pubsub_on_rx_refresh, so
- * if we clean the sub tree in pubsub_on_evsub_state it won't be available in
- * pubsub_on_rx_refresh. This means we won't be able to build or send the
- * corresponding NOTIFY (which also causes pjsip to assert.)
- * If HAVE_PJSIP_EVSUB_PENDING_NOTIFY is set based on configuration, this will
- * be called from pubsub_on_rx_refresh. If not set, the result is the legacy
- * behavior of calling this from pubsub_on_evsub_state.
- */
+/* The code in this function was previously in pubsub_on_evsub_state. */
static void clean_sub_tree(pjsip_evsub *evsub){
struct sip_subscription_tree *sub_tree;
sub_tree = pjsip_evsub_get_mod_data(evsub, pubsub_module.id);
+ ast_debug(3, "Cleaning subscription %p\n", evsub);
+
if (sub_tree->expiration_task) {
char task_name[256];
@@ -3915,12 +3921,22 @@
return;
}
-#ifndef HAVE_PJSIP_EVSUB_PENDING_NOTIFY
- /* for pjproject <2.13, this cleanup occurs here. For >=2.13, pubsub_on_evsub_state
- is called before pubsub_on_rx_refresh and so must be cleaned there.*/
- clean_sub_tree(evsub);
-#endif
+#ifdef HAVE_PJSIP_EVSUB_PENDING_NOTIFY
+ /* This check looks for re-subscribes with an expires of 0. If we receive one of those,
+ we don't want to clean the evsub because we still need it to send the final NOTIFY.
+ This was previously handled by pubsub_on_rx_refresh setting:
+ 'sub_tree->state = SIP_SUB_TREE_TERMINATE_PENDING' */
+ if (event->body.tsx_state.type == PJSIP_EVENT_RX_MSG &&
+ !pjsip_method_cmp(&event->body.tsx_state.tsx->method, &pjsip_subscribe_method) &&
+ pjsip_evsub_get_expires(evsub) == 0) {
+ ast_debug(3, "Subscription ending, do nothing.\n");
+ return;
+ }
+#endif
+ /* If we made it this far, we want to clean the sub tree. For pjproject <2.13, the sub_tree
+ state check makes sure the evsub is not cleaned at the wrong time */
+ clean_sub_tree(evsub);
}
static int pubsub_on_refresh_timeout(void *userdata)
@@ -4036,8 +4052,7 @@
* This includes both SUBSCRIBE requests that actually refresh the subscription
* as well as SUBSCRIBE requests that end the subscription.
*
- * In either case we push serialized_pubsub_on_refresh_timeout to send an
- * appropriate NOTIFY request.
+ * In either case we push an appropriate NOTIFY via pubsub_on_refresh_timeout.
*/
static void pubsub_on_rx_refresh(pjsip_evsub *evsub, pjsip_rx_data *rdata,
int *p_st_code, pj_str_t **p_st_text, pjsip_hdr *res_hdr, pjsip_msg_body **p_body)
@@ -4135,8 +4150,8 @@
/* As of pjsip 2.13, the NOTIFY has to be sent within this function as pjproject now
requires it. Previously this would have caused an early NOTIFY to go out before the
SUBSCRIBE's 200 OK. The previous solution was to push the NOTIFY, but now pjproject
- looks for the NOTIFY on send and delays it until after it auto-replies.
- If the NOTIFY is not there when it looks to send, pjproject will assert. */
+ looks for the NOTIFY to be sent from this function and caches it to send after it
+ auto-replies to the SUBSCRIBE. */
pubsub_on_refresh_timeout(sub_tree);
#else
if (ast_sip_push_task(sub_tree->serializer, serialized_pubsub_on_refresh_timeout, ao2_bump(sub_tree))) {
@@ -4150,18 +4165,6 @@
if (sub_tree->is_list) {
pj_list_insert_before(res_hdr, create_require_eventlist(rdata->tp_info.pool));
}
-
-#ifdef HAVE_PJSIP_EVSUB_PENDING_NOTIFY
- /* for pjproject <2.13, this cleanup occurs in pubsub_on_evsub_state. For >=2.13,
- pubsub_on_rx_refresh is called after pubsub_on_evsub_state and so the tree must be
- cleaned here. */
- if( pjsip_evsub_get_state(evsub) == PJSIP_EVSUB_STATE_TERMINATED &&
- sub_tree->state == SIP_SUB_TREE_TERMINATE_PENDING ) {
- clean_sub_tree(evsub);
- }
-#endif
-
-
}
static void pubsub_on_rx_notify(pjsip_evsub *evsub, pjsip_rx_data *rdata, int *p_st_code,
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/20017
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 20
Gerrit-Change-Id: I05c1d91a44fe28244ae93faa4a2268a3332b5fd7
Gerrit-Change-Number: 20017
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Bradeen <mbradeen at sangoma.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20230330/2d612376/attachment-0001.html>
More information about the asterisk-code-review
mailing list