[Asterisk-code-review] channel.c: Fix unbalanced read queue deadlocking local chann... (asterisk[13])

[George Joseph]

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

Change subject: channel.c: Fix unbalanced read queue deadlocking local channels.
......................................................................


channel.c: Fix unbalanced read queue deadlocking local channels.

Using the timerfd timing module can cause channel freezing, lingering, or
deadlock issues.  The problem is because this is the only timing module
that uses an associated alert-pipe.  When the alert-pipe becomes
unbalanced with respect to the number of frames in the read queue bad
things can happen.  If the alert-pipe has fewer alerts queued than the
read queue then nothing might wake up the thread to handle received frames
from the channel driver.  For local channels this is the only way to wake
up the thread to handle received frames.  Being unbalanced in the other
direction is less of an issue as it will cause unnecessary reads into the
channel driver.

ASTERISK-26716 is an example of this deadlock which was indirectly fixed
by the change that found the need for this patch.

* In channel.c:__ast_queue_frame(): Adding frame lists to the read queue
did not add the same number of alerts to the alert-pipe.  Correspondingly,
when there is an exceptionally long queue event, any removed frames did
not also remove the corresponding number of alerts from the alert-pipe.

ASTERISK-26632 #close

Change-Id: Ia98137c5bf6e9d6d202ce0eb36441851875863f6
---
M main/channel.c
1 file changed, 10 insertions(+), 3 deletions(-)

Approvals:
  Mark Michelson: Looks good to me, but someone else must approve
  George Joseph: Looks good to me, approved
  Anonymous Coward #1000019: Verified
  Joshua Colp: Looks good to me, but someone else must approve



diff --git a/main/channel.c b/main/channel.c
index 5301112..28cfa79 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -1147,6 +1147,9 @@
 				}
 				AST_LIST_REMOVE_CURRENT(frame_list);
 				ast_frfree(cur);
+
+				/* Read from the alert pipe for each flushed frame. */
+				ast_channel_internal_alert_read(chan);
 			}
 		}
 		AST_LIST_TRAVERSE_SAFE_END;
@@ -1163,9 +1166,13 @@
 	}
 
 	if (ast_channel_alert_writable(chan)) {
-		if (ast_channel_alert_write(chan)) {
-			ast_log(LOG_WARNING, "Unable to write to alert pipe on %s (qlen = %u): %s!\n",
-				ast_channel_name(chan), queued_frames, strerror(errno));
+		/* Write to the alert pipe for each added frame */
+		while (new_frames--) {
+			if (ast_channel_alert_write(chan)) {
+				ast_log(LOG_WARNING, "Unable to write to alert pipe on %s (qlen = %u): %s!\n",
+					ast_channel_name(chan), queued_frames, strerror(errno));
+				break;
+			}
 		}
 	} else if (ast_channel_timingfd(chan) > -1) {
 		ast_timer_enable_continuous(ast_channel_timer(chan));

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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia98137c5bf6e9d6d202ce0eb36441851875863f6
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>