[Asterisk-code-review] pbx: Use same thread if AST OUTGOING WAIT COMPLETE specified (asterisk[master])

[George Joseph]

George Joseph has submitted this change and it was merged. ( https://gerrit.asterisk.org/5508 )

Change subject: pbx: Use same thread if AST_OUTGOING_WAIT_COMPLETE specified
......................................................................


pbx: Use same thread if AST_OUTGOING_WAIT_COMPLETE specified

Both ast_pbx_outgoing_app() and ast_pbx_outgoing_exten() cause the core
to spawn a new thread to perform the dial. When AST_OUTGOING_WAIT_COMPLETE
is passed to these functions, the calling thread will be blocked until
the newly created channel has been hung up.

After this patch, we run the dial on the current thread rather than
spawning a new one. The only in-tree code that passes
AST_OUTGOING_WAIT_COMPLETE is pbx_spool, so you should see reduced
thread usage if you are using .call files.

Change-Id: I512735d243f0a9da2bcc128f7a96dece71f2d913
---
M main/pbx.c
1 file changed, 44 insertions(+), 36 deletions(-)

Approvals:
  Mark Michelson: Looks good to me, but someone else must approve
  Jenkins2: Verified
  Joshua Colp: Looks good to me, approved



diff --git a/main/pbx.c b/main/pbx.c
index dc4b91f..28027c0 100644
--- a/main/pbx.c
+++ b/main/pbx.c
@@ -7529,8 +7529,8 @@
 	int dial_res;
 	/*! \brief Set when dialing is completed */
 	unsigned int dialed:1;
-	/*! \brief Set when execution is completed */
-	unsigned int executed:1;
+	/*! \brief Set if we've spawned a thread to do our work */
+	unsigned int in_separate_thread:1;
 };
 
 /*! \brief Destructor for outgoing structure */
@@ -7553,13 +7553,19 @@
 	RAII_VAR(struct pbx_outgoing *, outgoing, data, ao2_cleanup);
 	enum ast_dial_result res;
 
-	/* Notify anyone interested that dialing is complete */
 	res = ast_dial_run(outgoing->dial, NULL, 0);
-	ao2_lock(outgoing);
-	outgoing->dial_res = res;
-	outgoing->dialed = 1;
-	ast_cond_signal(&outgoing->cond);
-	ao2_unlock(outgoing);
+
+	if (outgoing->in_separate_thread) {
+		/* Notify anyone interested that dialing is complete */
+		ao2_lock(outgoing);
+		outgoing->dial_res = res;
+		outgoing->dialed = 1;
+		ast_cond_signal(&outgoing->cond);
+		ao2_unlock(outgoing);
+	} else {
+		/* We still need the dial result, but we don't need to lock */
+		outgoing->dial_res = res;
+	}
 
 	/* If the outgoing leg was not answered we can immediately return and go no further */
 	if (res != AST_DIAL_RESULT_ANSWERED) {
@@ -7598,12 +7604,6 @@
 			ast_dial_answered_steal(outgoing->dial);
 		}
 	}
-
-	/* Notify anyone else again that may be interested that execution is complete */
-	ao2_lock(outgoing);
-	outgoing->executed = 1;
-	ast_cond_signal(&outgoing->cond);
-	ao2_unlock(outgoing);
 
 	return NULL;
 }
@@ -7810,34 +7810,42 @@
 		}
 	}
 
+	/* This extra reference is dereferenced by pbx_outgoing_exec */
 	ao2_ref(outgoing, +1);
-	if (ast_pthread_create_detached(&thread, NULL, pbx_outgoing_exec, outgoing)) {
-		ast_log(LOG_WARNING, "Unable to spawn dialing thread for '%s/%s'\n", type, addr);
-		ao2_ref(outgoing, -1);
-		if (locked_channel) {
-			if (!synchronous) {
-				ast_channel_unlock(dialed);
+
+	if (synchronous == AST_OUTGOING_WAIT_COMPLETE) {
+		/*
+		 * Because we are waiting until this is complete anyway, there is no
+		 * sense in creating another thread that we will just need to wait
+		 * for, so instead we commandeer the current thread.
+		 */
+		pbx_outgoing_exec(outgoing);
+	} else {
+		outgoing->in_separate_thread = 1;
+
+		if (ast_pthread_create_detached(&thread, NULL, pbx_outgoing_exec, outgoing)) {
+			ast_log(LOG_WARNING, "Unable to spawn dialing thread for '%s/%s'\n", type, addr);
+			ao2_ref(outgoing, -1);
+			if (locked_channel) {
+				if (!synchronous) {
+					ast_channel_unlock(dialed);
+				}
+				ast_channel_unref(dialed);
 			}
-			ast_channel_unref(dialed);
+			return -1;
 		}
-		return -1;
+
+		if (synchronous) {
+			ao2_lock(outgoing);
+			/* Wait for dialing to complete */
+			while (!outgoing->dialed) {
+				ast_cond_wait(&outgoing->cond, ao2_object_get_lockaddr(outgoing));
+			}
+			ao2_unlock(outgoing);
+		}
 	}
 
 	if (synchronous) {
-		ao2_lock(outgoing);
-		/* Wait for dialing to complete */
-		while (!outgoing->dialed) {
-			ast_cond_wait(&outgoing->cond, ao2_object_get_lockaddr(outgoing));
-		}
-		if (1 < synchronous
-			&& outgoing->dial_res == AST_DIAL_RESULT_ANSWERED) {
-			/* Wait for execution to complete */
-			while (!outgoing->executed) {
-				ast_cond_wait(&outgoing->cond, ao2_object_get_lockaddr(outgoing));
-			}
-		}
-		ao2_unlock(outgoing);
-
 		/* Determine the outcome of the dialing attempt up to it being answered. */
 		if (reason) {
 			*reason = pbx_dial_reason(outgoing->dial_res,

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I512735d243f0a9da2bcc128f7a96dece71f2d913
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Sean Bright <sean.bright at gmail.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>