[Asterisk-code-review] Frame deferral: Will this be the last rework on it? (asterisk[13])

[Richard Mudgett]

Hello Mark Michelson, Anonymous Coward #1000019,

I'd like you to reexamine a change.  Please visit

    https://gerrit.asterisk.org/4771

to look at the new patch set (#2).

Change subject: Frame deferral: Will this be the last rework on it?
......................................................................

Frame deferral: Will this be the last rework on it?

There are several issues with deferring frames that are handled by this
patch.

1) 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.

* 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.

2) Deferrable frames can come directly from the channel driver as well as
the read queue.  These frames need to be added to the deferred queue.

* In channel.c extracted read_core() out of __ast_read() so __ast_read()
can save off deferrable frames from any source.

3) Whoever is deferring frames is really only doing the __ast_read() to
collect deferred frames and doesn't care about the returned frames except
to detect a hangup event.  When frame deferral is completed we must make
the normal frame processing see the hangup as a frame anyway.  As such,
there is no need to have varying hangup frame deferral methods.  We also
need to be aware of the AST_SOFTHANGUP_ASYNCGOTO hangup that isn't real.
That fake hangup is to cause the PBX thread to break out of loops to go
execute a new dialplan location.

* Removed the defer_hangups parameter from
ast_channel_start_defer_frames() as it is no longer needed.

* Made channel.c:ast_channel_destructor() destroy any frames that may
still exist in the deferred read queue.  This is a fail safe as there
should never be any frames remaining in the deferred queue when a channel
is destroyed.

* Added a comment note in channel.c:channel_do_masquerade() that the
deferred read queue does not participate in masquerades.

4) To properly deal with deferrable frames from the channel driver as
pointed out by (2) above, means that it is possible to process a dialplan
interception routine while frames are deferred because of the
AST_CONTROL_READ_ACTION control frame.  Deferring frames is not
implemented as a re-entrant operation so you could have the unsupported
case of two sections of code thinking they have control of the media
stream.

A dialplan intercept routine is equivalent to an interrupt routine.  As
such, the routine must be done quickly and you do not have access to the
media stream.  These restrictions are necessary because the media stream
is the responsibility of some other code and interfering with or delaying
that processing is bad.  A possible future dialplan processing
architecture change may allow the interception routine to run in a
different thread from the main thread handling the media and remove the
execution time restriction.

* Made res_agi.c:run_agi() running an AGI in an interception routine run
in DeadAGI mode.  No touchy channel frames.

ASTERISK-26716 #close

Change-Id: If8383a8b23ba9a335c138a9c0e79fca24b78343c
---
M include/asterisk/channel.h
M main/autoservice.c
M main/channel.c
M res/res_agi.c
4 files changed, 182 insertions(+), 85 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/71/4771/2
-- 
To view, visit https://gerrit.asterisk.org/4771
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If8383a8b23ba9a335c138a9c0e79fca24b78343c
Gerrit-PatchSet: 2
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>