[asterisk-commits] russell: branch 1.6.1 r179535 - in /branches/1.6.1: ./ apps/app_meetme.c

[SVN commits to the Asterisk project]

Author: russell
Date: Mon Mar  2 17:39:56 2009
New Revision: 179535

URL: http://svn.digium.com/svn-view/asterisk?view=rev&rev=179535
Log:
Merged revisions 179533 via svnmerge from 
https://origsvn.digium.com/svn/asterisk/trunk

................
r179533 | russell | 2009-03-02 17:36:38 -0600 (Mon, 02 Mar 2009) | 48 lines

Merged revisions 179532 via svnmerge from 
https://origsvn.digium.com/svn/asterisk/branches/1.4

........
r179532 | russell | 2009-03-02 17:34:13 -0600 (Mon, 02 Mar 2009) | 40 lines

Move ast_waitfor() down to avoid the results of the API call becoming stale.

This call to ast_waitfor() was being done way too soon in this section of code.
Specifically, there was code in between the call to waitfor and the code that
uses the result that puts the channel in autoservice.  By putting the channel
in autoservice, the previous results of ast_waitfor() become meaningless,
as the autoservice thread will do it's own ast_waitfor() and ast_read()
on the channel.

So, when we came back out of autoservice and eventually hit the block of code
that calls ast_read() on the channel, there may not actually be any input on
the channel available.  Even though the previous call to ast_waitfor() in
app_meetme said there was input, the autoservice thread has since serviced
the channel for some period of time.

This bug manifested itself while dvossel was doing some testing of MeetMe in
Asterisk trunk.  He was using the timerfd timing module.  When the code hit
ast_read() erroneously, it determined that it must have been called because of
input on the timer fd, as chan->fdno was set to AST_TIMING_FD, since that was 
the cause of the last legitimate call to ast_read() done by autoservice.  

In this test, an IAX2 channel was calling into the MeetMe conference.  It was
_much_ more likely to be seen with an IAX2 channel because of the way audio
is handled.  Every audio frame that comes in results in a call to
ast_queue_frame(), which then uses ast_timer_enable_continuous() to notify
the channel thread that a frame is waiting to be handled.  So, the chances
of ast_waitfor() indicating that a channel needs servicing due to a timer
event on an IAX2 event is very high.

Finally, it is interesting to note that if a different timing interface was
being used, this bug would probably not be noticed.  When ast_read() is called
and erroneously thinks that there is a timer event to handle, it calls the
ast_timer_ack() function.  The pthread and dahdi timing modules handle the
ack() function being called when there is no event by simply ignoring it.
In the case of the timerfd module, it results in a read() on the timer fd
that will block forever, as there is no data to read.  This caused Asterisk
to lock up very quickly.

Thanks to dvossel and mmichelson for the fun debugging session.  :-)

........

................

Modified:
    branches/1.6.1/   (props changed)
    branches/1.6.1/apps/app_meetme.c

Propchange: branches/1.6.1/
------------------------------------------------------------------------------
Binary property 'trunk-merged' - no diff available.

Modified: branches/1.6.1/apps/app_meetme.c
URL: http://svn.digium.com/svn-view/asterisk/branches/1.6.1/apps/app_meetme.c?view=diff&rev=179535&r1=179534&r2=179535
==============================================================================
--- branches/1.6.1/apps/app_meetme.c (original)
+++ branches/1.6.1/apps/app_meetme.c Mon Mar  2 17:39:56 2009
@@ -2280,8 +2280,6 @@
 						ast_waitstream(chan, "");
 			}
 
-			c = ast_waitfor_nandfds(&chan, 1, &fd, nfds, NULL, &outfd, &ms);
-
 			/* Update the struct with the actual confflags */
 			user->userflags = confflags;
 
@@ -2436,6 +2434,8 @@
 			/* Perform an extra hangup check just in case */
 			if (ast_check_hangup(chan))
 				break;
+
+			c = ast_waitfor_nandfds(&chan, 1, &fd, nfds, NULL, &outfd, &ms);
 
 			if (c) {
 				char dtmfstr[2] = "";