[asterisk-dev] tilghman: branch 1.6.0 r244072 - in /branches/1.6.0: ./ channels/ main/

[Tilghman Lesher]

On Monday 01 February 2010 17:06:04 Russell Bryant wrote:
> On 02/01/2010 04:47 PM, Tilghman Lesher wrote:
> > On Monday 01 February 2010 16:15:32 Russell Bryant wrote:
> >> On 02/01/2010 02:02 PM, Tilghman Lesher wrote:
> >>> Well, yes, it will, but the error wasn't entirely removed, either. 
> >>> Atis remarked on this and suggested yet another exception to the rule. 
> >>> This tends to suggest that with so many exceptions, the previous
> >>> approach wasn't the right way to do this.  With the patch above, the
> >>> error may still occur, but it will only occur once in a brief while,
> >>> not constantly, once the limit has been reached.
>
> This commit closed out the bug, though.  I do not consider it closed as
> you have reintroduced another problem.  Even if the message is not as
> frequent, it is still a problem, the warning will freak people out, and
> we _will_ get bug reports.

Actually, no, I have not reintroduced another problem.  The original report
was never really resolved, as evidenced by the below log.  So before anything
was done, we had the error message.  We have the error message before this
patch, and we have the error message after this patch (although finally, the
error message NOW will appear much less often).  So whether or not the warning
exists after the latest patch is mostly irrelevant to the discussion.  That it
occurs much less frequently than before is helpful to the situation.

> >> What was the additional exception proposed to be added?  For reference,
> >> this is what we previously had this in chan_local:
> >>
> >> if (other->pbx || other->_bridge || !ast_strlen_zero(other->appl)) {
> >>           ast_queue_frame(other, f);
> >> }
> >
> > [19:14:04]<atis_home>  Corydon76-dig: btw, i'm still seeing
> > "Exceptionally long voice queue" on Local channels with your patches to
> > chan_local :p
> >
> > [08:31:06]<atis_work>  so, there's one more situation when other->appl is
> > zero, but control frames need to be forwarded
> > [08:31:29]<atis_work>  if Originating Local/123/n and
> > Context/Exten/Priority
> >
> > [08:34:36]<Corydon76-dig>  It just seems like we keep piling on the
> > exceptions, and it's therefore clear that that wasn't the right approach
>
> Based on this discussion, I do not agree with the patch committed.  The
> code that was in chan_local already accounted for this situation.  In
> this situation, both sides of a Local channel will have a non-NULL pbx
> on the ast_channel.  So, something else is going on and we should figure
> out what it is.

It may have accounted for the situation, but it did not solve the problem,
because the frame queue still overflowed.  If we don't solve that problem,
any code change merely spits into the wind.  We have to avoid the frame
queue constantly overflowing, or we aren't really solving the problem.

> Take for example:
>
> *CLI> originate Local/123 at foo extension 456 at bar
>
> Here is what you have connected.  Both sides are running a PBX.
>
> (Some app) <-->
>     PBX <-->
>        [Local/123 at foo,1] <-->
>           [Local/123 at foo,2] <-->
>              PBX <-->
>                 (Some app)

What we are trying to do is prevent the frame queue from overflowing, by
detecting the particular reasons why a frame queue should not overflow.  So
what this is really showing is that whether or not a PBX is running on a
channel is not relevant in all cases as to whether the frame queue will
overflow.  Whether you intended to or not, you've just disproved that the
original patch was at all correct for the situation (because a PBX existed on
the channel AND it was bridged, but the frame queue overflowed anyway).
That is the problem that the latest patch solves (or at least deals with in a
way that it does not immediately recur).

-- 
Tilghman Lesher
Digium, Inc. | Senior Software Developer
twitter: Corydon76 | IRC: Corydon76-dig (Freenode)
Check us out at: www.digium.com & www.asterisk.org