[Asterisk-code-review] res_pjsip_session: Fix off-nominal session refreshes. (asterisk[master])
Joshua Colp
asteriskteam at digium.com
Thu Feb 13 19:02:03 CST 2020
Joshua Colp has submitted this change. ( https://gerrit.asterisk.org/c/asterisk/+/13779 )
Change subject: res_pjsip_session: Fix off-nominal session refreshes.
......................................................................
res_pjsip_session: Fix off-nominal session refreshes.
Given a scenario where session refreshes occur close to
each other while another is finishing it was possible for
the session refreshes to occur out of order. It was
also possible for session refreshes to be delayed for
quite some time if a session refresh did not result in
a topology change.
For the out of order session refreshes the first session
refresh would be queued due to a transaction in progress.
This transaction would then finish. When finished a
separate task to process the delayed requests queue
would be queued for handling. A second refresh would
be requested internally before this delayed request
queued task was processed. As no transaction was in
progress this session refresh would be immediately
handled before the queued session refresh.
The code will now check if any delayed requests exist
before allowing a session refresh to immediately occur.
If any exist then the session refresh is queued.
For the delayed session refreshes if a session refresh
did not result in a topology change the attempt would
be immediately stopped and no other delayed requests would
be processed.
The code will now go through the entire delayed requests
queue until a delayed request results in a request
actually being sent.
ASTERISK-28730
Change-Id: Ied640280133871f77d3f332be62265e754605088
---
M res/res_pjsip_session.c
1 file changed, 62 insertions(+), 11 deletions(-)
Approvals:
George Joseph: Looks good to me, but someone else must approve
Kevin Harwell: Looks good to me, approved
Joshua Colp: Approved for Submit
diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 76740c1..0eafb9c 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -69,6 +69,13 @@
enum ast_sip_session_response_priority response_priority);
static void handle_outgoing_request(struct ast_sip_session *session, pjsip_tx_data *tdata);
static void handle_outgoing_response(struct ast_sip_session *session, pjsip_tx_data *tdata);
+static int sip_session_refresh(struct ast_sip_session *session,
+ ast_sip_session_request_creation_cb on_request_creation,
+ ast_sip_session_sdp_creation_cb on_sdp_creation,
+ ast_sip_session_response_cb on_response,
+ enum ast_sip_session_refresh_method method, int generate_new_sdp,
+ struct ast_sip_session_media_state *media_state,
+ int queued);
/*! \brief NAT hook for modifying outgoing messages with SDP */
static struct ast_sip_nat_hook *nat_hook;
@@ -1244,8 +1251,18 @@
ast_free(delay);
}
+/*!
+ * \internal
+ * \brief Send a delayed request
+ *
+ * \retval -1 failure
+ * \retval 0 success
+ * \retval 1 refresh request not sent as no change would occur
+ */
static int send_delayed_request(struct ast_sip_session *session, struct ast_sip_session_delayed_request *delay)
{
+ int res;
+
ast_debug(3, "Endpoint '%s(%s)' sending delayed %s request.\n",
ast_sorcery_object_get_id(session->endpoint),
session->channel ? ast_channel_name(session->channel) : "",
@@ -1253,19 +1270,19 @@
switch (delay->method) {
case DELAYED_METHOD_INVITE:
- ast_sip_session_refresh(session, delay->on_request_creation,
+ res = sip_session_refresh(session, delay->on_request_creation,
delay->on_sdp_creation, delay->on_response,
- AST_SIP_SESSION_REFRESH_METHOD_INVITE, delay->generate_new_sdp, delay->media_state);
+ AST_SIP_SESSION_REFRESH_METHOD_INVITE, delay->generate_new_sdp, delay->media_state, 1);
/* Ownership of media state transitions to ast_sip_session_refresh */
delay->media_state = NULL;
- return 0;
+ return res;
case DELAYED_METHOD_UPDATE:
- ast_sip_session_refresh(session, delay->on_request_creation,
+ res = sip_session_refresh(session, delay->on_request_creation,
delay->on_sdp_creation, delay->on_response,
- AST_SIP_SESSION_REFRESH_METHOD_UPDATE, delay->generate_new_sdp, delay->media_state);
+ AST_SIP_SESSION_REFRESH_METHOD_UPDATE, delay->generate_new_sdp, delay->media_state, 1);
/* Ownership of media state transitions to ast_sip_session_refresh */
delay->media_state = NULL;
- return 0;
+ return res;
case DELAYED_METHOD_BYE:
ast_sip_session_terminate(session, 0);
return 0;
@@ -1300,7 +1317,9 @@
AST_LIST_REMOVE_CURRENT(next);
res = send_delayed_request(session, delay);
delayed_request_free(delay);
- found = 1;
+ if (!res) {
+ found = 1;
+ }
break;
case DELAYED_METHOD_BYE:
/* A BYE is pending so don't bother anymore. */
@@ -1354,7 +1373,9 @@
AST_LIST_REMOVE_CURRENT(next);
res = send_delayed_request(session, delay);
delayed_request_free(delay);
- break;
+ if (!res) {
+ break;
+ }
}
}
AST_LIST_TRAVERSE_SAFE_END;
@@ -1558,12 +1579,13 @@
}
}
-int ast_sip_session_refresh(struct ast_sip_session *session,
+static int sip_session_refresh(struct ast_sip_session *session,
ast_sip_session_request_creation_cb on_request_creation,
ast_sip_session_sdp_creation_cb on_sdp_creation,
ast_sip_session_response_cb on_response,
enum ast_sip_session_refresh_method method, int generate_new_sdp,
- struct ast_sip_session_media_state *media_state)
+ struct ast_sip_session_media_state *media_state,
+ int queued)
{
pjsip_inv_session *inv_session = session->inv_session;
pjmedia_sdp_session *new_sdp = NULL;
@@ -1630,6 +1652,20 @@
int type_streams[AST_MEDIA_TYPE_END] = {0};
struct ast_stream *stream;
+ /* Media state conveys a desired media state, so if there are outstanding
+ * delayed requests we need to ensure we go into the queue and not jump
+ * ahead. If we sent this media state now then updates could go out of
+ * order.
+ */
+ if (!queued && !AST_LIST_EMPTY(&session->delayed_requests)) {
+ ast_debug(3, "Delay sending reinvite to %s because of outstanding requests...\n",
+ ast_sorcery_object_get_id(session->endpoint));
+ return delay_request(session, on_request_creation, on_sdp_creation,
+ on_response, generate_new_sdp,
+ method == AST_SIP_SESSION_REFRESH_METHOD_INVITE
+ ? DELAYED_METHOD_INVITE : DELAYED_METHOD_UPDATE, media_state);
+ }
+
/* Prune the media state so the number of streams fit within the configured limits - we do it here
* so that the index of the resulting streams in the SDP match. If we simply left the streams out
* of the SDP when producing it we'd be in trouble. We also enforce formats here for media types that
@@ -1742,7 +1778,11 @@
/* If the resulting media state matches the existing active state don't bother doing a session refresh */
if (ast_stream_topology_equal(session->active_media_state->topology, media_state->topology)) {
ast_sip_session_media_state_free(media_state);
- return 0;
+ /* For external consumers we return 0 to say success, but internally for
+ * send_delayed_request we return a separate value to indicate that this
+ * session refresh would be redundant so we didn't send it
+ */
+ return queued ? 1 : 0;
}
}
@@ -1794,6 +1834,17 @@
return 0;
}
+int ast_sip_session_refresh(struct ast_sip_session *session,
+ ast_sip_session_request_creation_cb on_request_creation,
+ ast_sip_session_sdp_creation_cb on_sdp_creation,
+ ast_sip_session_response_cb on_response,
+ enum ast_sip_session_refresh_method method, int generate_new_sdp,
+ struct ast_sip_session_media_state *media_state)
+{
+ return sip_session_refresh(session, on_request_creation, on_sdp_creation,
+ on_response, method, generate_new_sdp, media_state, 0);
+}
+
int ast_sip_session_regenerate_answer(struct ast_sip_session *session,
ast_sip_session_sdp_creation_cb on_sdp_creation)
{
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/13779
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ied640280133871f77d3f332be62265e754605088
Gerrit-Change-Number: 13779
Gerrit-PatchSet: 2
Gerrit-Owner: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200213/a2b91a93/attachment-0001.html>
More information about the asterisk-code-review
mailing list