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