[Home]

Summary:ASTERISK-12237: REGRESSION: chan_iax2.c patch 122259 results in chan_iax2.c sending no PING or LAGRQs
Reporter:Steve Davies . (stevedavies)Labels:
Date Opened:2008-06-20 08:05:59Date Closed:2008-07-01 13:45:45
Priority:MajorRegression?No
Status:Closed/CompleteComponents:Channels/chan_iax2
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20080620__bug12903__2.diff.txt
Description:In latest (124182) 1.4 branch, chan_iax2.c no longer sends any PING or LAGRQ frames.

It looks like the reason is the change between revs 120168 and 122259, which has send_ping and send_lagrq reset the pingid and lagid to -1.  

The logged reason for the change was to "Fix some race conditions that cause ast_assert() to report that chan_iax2 tried to remove an entry that wasn't in the scheduler"

But if you look at the __send_ping and send_ping taken together (appended), you can see that once pingid is -1, no more pings will ever get sent.

(Strange use of a while and break, BTW.  What's wrong with an ordinary if?)

This change probably got rid of the warning, but it did it by stopping all PING/LAGRQ.  I think it should be reverted, and instead just check for -1 before ast_sched_delling the pingid/laqid

----------------------------------------------
static int send_ping(const void *data);

static void __send_ping(const void *data)
{
       int callno = (long) data;

       ast_mutex_lock(&iaxsl[callno]);

       while (iaxs[callno] && iaxs[callno]->pingid != -1) {
               if (iaxs[callno]->peercallno) {
                       send_command(iaxs[callno], AST_FRAME_IAX, IAX_COMMAND_PING, 0, NULL, 0, -1);
               }
               iaxs[callno]->pingid = iax2_sched_add(sched, ping_time * 1000, send_ping, data);
               break;
       }

       ast_mutex_unlock(&iaxsl[callno]);
}

static int send_ping(const void *data)
{
       int callno = (long) data;

       ast_mutex_lock(&iaxsl[callno]);
       if (iaxs[callno]) {
               iaxs[callno]->pingid = -1;
       }
       ast_mutex_unlock(&iaxsl[callno]);

#ifdef SCHED_MULTITHREADED
       if (schedule_action(__send_ping, data))
#endif
               __send_ping(data);

       return 0;
}
----------------------------------------------



Comments:By: Tilghman Lesher (tilghman) 2008-06-20 10:27:41

Let's try this patch.  In addition to reverting the changes, I've also found a possible place where the scheduled items may need to be cancelled, which may avoid the issue in the future.
By: Tilghman Lesher (tilghman) 2008-06-20 18:46:11

Oops.  First patch contained modifications from another patch.  New patch uploaded.
By: Tilghman Lesher (tilghman) 2008-06-26 12:38:08

stevedavies:  need testing and feedback on this patch, please
By: Steve Davies . (stevedavies) 2008-06-26 14:56:11

Hi,

I reverted the original change to fix the problem.  I'm doing other changes tomorrow so I'll apply your patch and report back.

Steve
By: mike240se (mike240se) 2008-06-27 02:07:27

does this also fix the hanging CLI?  Cause those of us with the iax2 issue in 1.4.21 also have the CLI hang and then the only way to kill it is to kill -9 asterisk.
By: Steve Davies . (stevedavies) 2008-07-01 09:16:15

OK - Corydon's patch now running on my test box; results to follow shortly
By: Steve Davies . (stevedavies) 2008-07-01 09:21:42

Hi,

I can confirm that with Corydon's patch applied, my Asterisk is still sending its PING and LAGRQs.

Thanks,
Steve
By: Digium Subversion (svnbot) 2008-07-01 13:45:33

Repository: asterisk
Revision: 127068

U   branches/1.4/channels/chan_iax2.c

------------------------------------------------------------------------
r127068 | tilghman | 2008-07-01 13:45:29 -0500 (Tue, 01 Jul 2008) | 8 lines

Change around how we schedule pings and lagrqs, and fix a reason why the
jobs were not getting properly cancelled.
(closes issue ASTERISK-12237)
Reported by: stevedavies
Patches:
      20080620__bug12903__2.diff.txt uploaded by Corydon76 (license 14)
Tested by: stevedavies

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=127068