<p>George Joseph <strong>submitted</strong> this change.</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/13791">View Change</a></p><div style="white-space:pre-wrap">Approvals:
  Joshua Colp: Looks good to me, but someone else must approve
  Benjamin Keith Ford: Looks good to me, but someone else must approve
  Kevin Harwell: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved; Approved for Submit

</div><pre style="font-family: monospace,monospace; white-space: pre-wrap;">res_pjsip_outbound_registration: Fix SRV failover on timeout<br><br>In order to retry outbound registrations for some situations, we<br>need access to the tdata from the original request.  For instance,<br>for 401/407 responses we need it to properly construct the<br>subsequent request with the authentication.  We also need it if<br>we're iterating over a DNS SRV response record set so we can skip<br>entries we've already tried.<br><br>We've been getting the tdata from the server response rdata and<br>transaction but that only works for the failures where there was<br>actually a response (4XX, 5XX, etc).  For timeouts there's no<br>response and therefore no rdata or transaction from which to get<br>the tdata.  When processing a single A/AAAA record for a server,<br>this wasn't an issue as we just retried that same server after the<br>retry timer expired.  If we got an SRV record set for the server<br>though, without the state from the tdata, we just kept trying the<br>first entry in the set repeatedly instead of skipping to the next<br>one in the list.<br><br>* Added a "last_tdata" member to the client state structure to keep<br>  track of the sent tdata.<br><br>* Updated registration_client_send() to save the tdata it used into<br>  the client_state.<br><br>* Updated sip_outbound_registration_response_cb() to use the tdata<br>  saved in client_state when we don't get a response from the<br>  server. We still use the tdata from the transaction when we DO<br>  get a response from the server so we can properly handle 4XX<br>  responses where our new request depends on it.<br><br>General note on timeouts:<br><br>Although res_pjsip_outbound_registration skips to the next record<br>immediately when a timeout occurs during SRV set traversal, it's<br>pjproject that determines how long to wait before a timeout is<br>declared.  As with other SIP message types, pjproject will continue<br>trying the same server at an interval specified by "timer_t1" until<br>"timer_b" expires.  Both of those timers are set in the pjsip.conf<br>"system" section.<br><br>ASTERISK-28746<br><br>Change-Id: I199b8274392d17661dd3ce3b4d69a3968368fa06<br>---<br>M res/res_pjsip_outbound_registration.c<br>1 file changed, 47 insertions(+), 2 deletions(-)<br><br></pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"><span>diff --git a/res/res_pjsip_outbound_registration.c b/res/res_pjsip_outbound_registration.c</span><br><span>index acc4b1d..2c58986 100644</span><br><span>--- a/res/res_pjsip_outbound_registration.c</span><br><span>+++ b/res/res_pjsip_outbound_registration.c</span><br><span>@@ -350,6 +350,14 @@</span><br><span>          * module unload.</span><br><span>     */</span><br><span>  pjsip_regc *client;</span><br><span style="color: hsl(120, 100%, 40%);">+   /*!</span><br><span style="color: hsl(120, 100%, 40%);">+    * \brief Last tdata sent</span><br><span style="color: hsl(120, 100%, 40%);">+      * We need the original tdata to resend a request on auth failure</span><br><span style="color: hsl(120, 100%, 40%);">+      * or timeout.  On an auth failure, we use the original tdata</span><br><span style="color: hsl(120, 100%, 40%);">+  * to initialize the new tdata for the authorized response.  On a timeout</span><br><span style="color: hsl(120, 100%, 40%);">+      * we need it to skip failed SRV entries if any.</span><br><span style="color: hsl(120, 100%, 40%);">+       */</span><br><span style="color: hsl(120, 100%, 40%);">+   pjsip_tx_data *last_tdata;</span><br><span>   /*! \brief Timer entry for retrying on temporal responses */</span><br><span>         pj_timer_entry timer;</span><br><span>        /*! \brief Optional line parameter placed into Contact */</span><br><span>@@ -563,6 +571,13 @@</span><br><span>     /* Due to the message going out the callback may now be invoked, so bump the count */</span><br><span>        ao2_ref(client_state, +1);</span><br><span>   /*</span><br><span style="color: hsl(120, 100%, 40%);">+     * We also bump tdata in expectation of saving it to client_state->last_tdata.</span><br><span style="color: hsl(120, 100%, 40%);">+      * We have to do it BEFORE pjsip_regc_send because if it succeeds, it decrements</span><br><span style="color: hsl(120, 100%, 40%);">+       * the ref count on its own.</span><br><span style="color: hsl(120, 100%, 40%);">+   */</span><br><span style="color: hsl(120, 100%, 40%);">+   pjsip_tx_data_add_ref(tdata);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+       /*</span><br><span>    * Set the transport in case transports were reloaded.</span><br><span>        * When pjproject removes the extraneous error messages produced,</span><br><span>     * we can check status and only set the transport and resend if there was an error</span><br><span>@@ -573,13 +588,26 @@</span><br><span> </span><br><span>       status = pjsip_regc_send(client_state->client, tdata);</span><br><span> </span><br><span style="color: hsl(0, 100%, 40%);">-   /* If the attempt to send the message failed and the callback was not invoked we need to</span><br><span style="color: hsl(0, 100%, 40%);">-         * drop the reference we just added</span><br><span style="color: hsl(120, 100%, 40%);">+   /*</span><br><span style="color: hsl(120, 100%, 40%);">+     * If the attempt to send the message failed and the callback was not invoked we need to</span><br><span style="color: hsl(120, 100%, 40%);">+       * drop the references we just added</span><br><span>          */</span><br><span>  if ((status != PJ_SUCCESS) && !(*callback_invoked)) {</span><br><span style="color: hsl(120, 100%, 40%);">+         pjsip_tx_data_dec_ref(tdata);</span><br><span>                ao2_ref(client_state, -1);</span><br><span style="color: hsl(120, 100%, 40%);">+            return status;</span><br><span>       }</span><br><span> </span><br><span style="color: hsl(120, 100%, 40%);">+ /*</span><br><span style="color: hsl(120, 100%, 40%);">+     * Decref the old last_data before replacing it.</span><br><span style="color: hsl(120, 100%, 40%);">+       * BTW, it's quite possible that last_data == tdata</span><br><span style="color: hsl(120, 100%, 40%);">+        * if we're trying successive servers in an SRV set.</span><br><span style="color: hsl(120, 100%, 40%);">+       */</span><br><span style="color: hsl(120, 100%, 40%);">+   if (client_state->last_tdata) {</span><br><span style="color: hsl(120, 100%, 40%);">+            pjsip_tx_data_dec_ref(client_state->last_tdata);</span><br><span style="color: hsl(120, 100%, 40%);">+   }</span><br><span style="color: hsl(120, 100%, 40%);">+     client_state->last_tdata = tdata;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span>       return status;</span><br><span> }</span><br><span> </span><br><span>@@ -1202,11 +1230,25 @@</span><br><span>            retry_after = pjsip_msg_find_hdr(param->rdata->msg_info.msg, PJSIP_H_RETRY_AFTER,</span><br><span>                      NULL);</span><br><span>               response->retry_after = retry_after ? retry_after->ivalue : 0;</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span style="color: hsl(120, 100%, 40%);">+                /*</span><br><span style="color: hsl(120, 100%, 40%);">+             * If we got a response from the server, we have to use the tdata</span><br><span style="color: hsl(120, 100%, 40%);">+              * from the transaction, not the tdata saved when we sent the</span><br><span style="color: hsl(120, 100%, 40%);">+          * request.  If we use the saved tdata, we won't process responses</span><br><span style="color: hsl(120, 100%, 40%);">+                 * like 423 Interval Too Brief correctly and we'll wind up sending</span><br><span style="color: hsl(120, 100%, 40%);">+                 * the bad Expires value again.</span><br><span style="color: hsl(120, 100%, 40%);">+                */</span><br><span style="color: hsl(120, 100%, 40%);">+           pjsip_tx_data_dec_ref(client_state->last_tdata);</span><br><span style="color: hsl(120, 100%, 40%);">+</span><br><span>                tsx = pjsip_rdata_get_tsx(param->rdata);</span><br><span>          response->old_request = tsx->last_tx;</span><br><span>          pjsip_tx_data_add_ref(response->old_request);</span><br><span>             pjsip_rx_data_clone(param->rdata, 0, &response->rdata);</span><br><span style="color: hsl(120, 100%, 40%);">+     } else {</span><br><span style="color: hsl(120, 100%, 40%);">+              /* old_request steals the reference */</span><br><span style="color: hsl(120, 100%, 40%);">+                response->old_request = client_state->last_tdata;</span><br><span>      }</span><br><span style="color: hsl(120, 100%, 40%);">+     client_state->last_tdata = NULL;</span><br><span> </span><br><span>      /*</span><br><span>    * Transfer response reference to serializer task so the</span><br><span>@@ -1252,6 +1294,9 @@</span><br><span>     ast_taskprocessor_unreference(client_state->serializer);</span><br><span>  ast_free(client_state->transport_name);</span><br><span>   ast_free(client_state->registration_name);</span><br><span style="color: hsl(120, 100%, 40%);">+ if (client_state->last_tdata) {</span><br><span style="color: hsl(120, 100%, 40%);">+            pjsip_tx_data_dec_ref(client_state->last_tdata);</span><br><span style="color: hsl(120, 100%, 40%);">+   }</span><br><span> }</span><br><span> </span><br><span> /*! \brief Allocator function for registration state */</span><br><span></span><br></pre><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/13791">change 13791</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/+/13791"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I199b8274392d17661dd3ce3b4d69a3968368fa06 </div>
<div style="display:none"> Gerrit-Change-Number: 13791 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: George Joseph <gjoseph@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Benjamin Keith Ford <bford@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </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: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-MessageType: merged </div>