[Asterisk-code-review] res pjsip session: Add cleanup to ast sip session terminate (asterisk[14])

[Jenkins2]

Jenkins2 has submitted this change and it was merged. ( https://gerrit.asterisk.org/5547 )

Change subject: res_pjsip_session:  Add cleanup to ast_sip_session_terminate
......................................................................


res_pjsip_session:  Add cleanup to ast_sip_session_terminate

If you use ast_request to create a PJSIP channel but then hang it
up without causing a transaction to be sent, the session will
never be destroyed.  This is due ot the fact that it's pjproject
that triggers the session cleanup when the transaction ends.
app_chanisavail was doing this to get more granular channel state
and it's also possible for this to happen via ARI.

* ast_sip_session_terminate was modified to explicitly call the
  cleanup tasks and unreference session if the invite state is NULL
  AND invite_tsx is NULL (meaning we never sent a transaction).

* chan_pjsip/hangup was modified to bump session before it calls
  ast_sip_session_terminate to insure that session stays valid
  while it does its own cleanup.

* Added test events to session_destructor for a future testsuite
  test.

ASTERISK-26908 #close
Reported-by: Richard Mudgett

Change-Id: I52daf6f757184e5544c261f64f6fe9602c4680a9
---
M channels/chan_pjsip.c
M include/asterisk/res_pjsip_session.h
M res/res_pjsip_session.c
3 files changed, 47 insertions(+), 5 deletions(-)

Approvals:
  Kevin Harwell: Looks good to me, but someone else must approve
  Mark Michelson: Looks good to me, approved
  Jenkins2: Approved for Submit
  Joshua Colp: Looks good to me, but someone else must approve



diff --git a/channels/chan_pjsip.c b/channels/chan_pjsip.c
index e8e3666..851c913 100644
--- a/channels/chan_pjsip.c
+++ b/channels/chan_pjsip.c
@@ -2054,11 +2054,16 @@
 	struct ast_sip_session *session = channel->session;
 	int cause = h_data->cause;
 
-	ast_sip_session_terminate(session, cause);
+	/*
+	 * It's possible that session_terminate might cause the session to be destroyed
+	 * immediately so we need to keep a reference to it so we can NULL session->channel
+	 * afterwards.
+	 */
+	ast_sip_session_terminate(ao2_bump(session), cause);
 	clear_session_and_channel(session, ast, pvt);
+	ao2_cleanup(session);
 	ao2_cleanup(channel);
 	ao2_cleanup(h_data);
-
 	return 0;
 }
 
diff --git a/include/asterisk/res_pjsip_session.h b/include/asterisk/res_pjsip_session.h
index d4d3f70..10e55f1 100644
--- a/include/asterisk/res_pjsip_session.h
+++ b/include/asterisk/res_pjsip_session.h
@@ -459,6 +459,10 @@
  *
  * \param session The session to terminate
  * \param response The response code to use for termination if possible
+ *
+ * \warning Calling this function MAY cause the last session reference to be
+ * released and the session destructor to be called.  If you need to do something
+ * with session after this call, be sure to bump the ref count before calling terminate.
  */
 void ast_sip_session_terminate(struct ast_sip_session *session, int response);
 
diff --git a/res/res_pjsip_session.c b/res/res_pjsip_session.c
index d98bc0a..59bd9d7 100644
--- a/res/res_pjsip_session.c
+++ b/res/res_pjsip_session.c
@@ -1300,9 +1300,19 @@
 	struct ast_sip_session *session = obj;
 	struct ast_sip_session_supplement *supplement;
 	struct ast_sip_session_delayed_request *delay;
+	const char *endpoint_name = session->endpoint ?
+		ast_sorcery_object_get_id(session->endpoint) : "<none>";
 
-	ast_debug(3, "Destroying SIP session with endpoint %s\n",
-		session->endpoint ? ast_sorcery_object_get_id(session->endpoint) : "<none>");
+	ast_debug(3, "Destroying SIP session with endpoint %s\n", endpoint_name);
+
+	ast_test_suite_event_notify("SESSION_DESTROYING",
+		"Endpoint: %s\r\n"
+		"AOR: %s\r\n"
+		"Contact: %s"
+		, endpoint_name
+		, session->aor ? ast_sorcery_object_get_id(session->aor) : "<none>"
+		, session->contact ? ast_sorcery_object_get_id(session->contact) : "<none>"
+		);
 
 	while ((supplement = AST_LIST_REMOVE_HEAD(&session->supplements, next))) {
 		if (supplement->session_destroy) {
@@ -1331,6 +1341,8 @@
 	if (session->inv_session) {
 		pjsip_dlg_dec_session(session->inv_session->dlg, &session_module);
 	}
+
+	ast_test_suite_event_notify("SESSION_DESTROYED", "Endpoint: %s", endpoint_name);
 }
 
 static int add_supplements(struct ast_sip_session *session)
@@ -1791,6 +1803,9 @@
 	return ret_session;
 }
 
+static int session_end(void *vsession);
+static int session_end_completion(void *vsession);
+
 void ast_sip_session_terminate(struct ast_sip_session *session, int response)
 {
 	pj_status_t status;
@@ -1807,7 +1822,25 @@
 
 	switch (session->inv_session->state) {
 	case PJSIP_INV_STATE_NULL:
-		pjsip_inv_terminate(session->inv_session, response, PJ_TRUE);
+		if (!session->inv_session->invite_tsx) {
+			/*
+			 * Normally, it's pjproject's transaction cleanup that ultimately causes the
+			 * final session reference to be released but if both STATE and invite_tsx are NULL,
+			 * we never created a transaction in the first place.  In this case, we need to
+			 * do the cleanup ourselves.
+			 */
+			/* Transfer the inv_session session reference to the session_end_task */
+			session->inv_session->mod_data[session_module.id] = NULL;
+			pjsip_inv_terminate(session->inv_session, response, PJ_TRUE);
+			session_end(session);
+			/*
+			 * session_end_completion will cleanup the final session reference unless
+			 * ast_sip_session_terminate's caller is holding one.
+			 */
+			session_end_completion(session);
+		} else {
+			pjsip_inv_terminate(session->inv_session, response, PJ_TRUE);
+		}
 		break;
 	case PJSIP_INV_STATE_CONFIRMED:
 		if (session->inv_session->invite_tsx) {

-- 
To view, visit https://gerrit.asterisk.org/5547
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I52daf6f757184e5544c261f64f6fe9602c4680a9
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 14
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>