[Asterisk-code-review] res_pjsip_session: Fix off-nominal session refreshes. (asterisk[16])
Joshua Colp
asteriskteam at digium.com
Mon Feb 10 05:20:01 CST 2020
Joshua Colp has uploaded this change for review. ( https://gerrit.asterisk.org/c/asterisk/+/13735 )
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, 61 insertions(+), 11 deletions(-)
git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/35/13735/1
diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index 1371909..962adff 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;
@@ -1593,6 +1615,19 @@
media_state);
}
+ /* If this is a new session refresh attempt and there are outstanding delayed
+ * requests ensure they get processed first to ensure proper ordering
+ */
+ if (!queued && !AST_LIST_EMPTY(&session->delayed_requests)) {
+ ast_debug(3, "Delay sending request 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);
+ }
+
if (method == AST_SIP_SESSION_REFRESH_METHOD_INVITE) {
if (inv_session->invite_tsx) {
/* We can't send a reinvite yet, so delay it */
@@ -1742,7 +1777,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 +1833,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/+/13735
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Ied640280133871f77d3f332be62265e754605088
Gerrit-Change-Number: 13735
Gerrit-PatchSet: 1
Gerrit-Owner: Joshua Colp <jcolp at sangoma.com>
Gerrit-MessageType: newchange
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20200210/1ff90a57/attachment-0001.html>
More information about the asterisk-code-review
mailing list