[asterisk-commits] Frame deferral: Revert API refactoring. (asterisk[master])
SVN commits to the Asterisk project
asterisk-commits at lists.digium.com
Thu Feb 2 18:08:43 CST 2017
Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/4878 )
Change subject: Frame deferral: Revert API refactoring.
......................................................................
Frame deferral: Revert API refactoring.
There are several issues with deferring frames that are caused by the
refactoring.
1) The code deferring frames mishandles adding a deferred frame to the
deferred queue. As a result the deferred queue can only be one frame
long.
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.
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.
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 worse problem is because of the bad implementation of the AMI PlayDTMF
action. It can cause two threads to be deferring frames on the same
channel at the same time. (ASTERISK_25940)
* Rather than fix all these problems simply revert the API refactoring as
there is going to be only autoservice and safe_sleep deferring frames
anyway.
ASTERISK-26343
ASTERISK-26716 #close
Change-Id: I45069c779aa3a35b6c863f65245a6df2c7865496
---
M include/asterisk/channel.h
M main/autoservice.c
M main/channel.c
M main/channel_internal_api.c
4 files changed, 89 insertions(+), 138 deletions(-)
Approvals:
Mark Michelson: Looks good to me, approved
Joshua Colp: Verified
diff --git a/include/asterisk/channel.h b/include/asterisk/channel.h
index 1b7246b..9cf313f 100644
--- a/include/asterisk/channel.h
+++ b/include/asterisk/channel.h
@@ -967,16 +967,6 @@
* The channel is executing a subroutine or macro
*/
AST_FLAG_SUBROUTINE_EXEC = (1 << 27),
- /*!
- * The channel is currently in an operation where
- * frames should be deferred.
- */
- AST_FLAG_DEFER_FRAMES = (1 << 28),
- /*!
- * The channel is currently deferring hangup frames
- * in addition to other frame types.
- */
- AST_FLAG_DEFER_HANGUP_FRAMES = (1 << 29),
};
/*! \brief ast_bridge_config flags */
@@ -4726,44 +4716,5 @@
* \brief Get error code for latest channel operation.
*/
enum ast_channel_error ast_channel_errno(void);
-
-/*!
- * \brief Retrieve the deferred read queue.
- */
-struct ast_readq_list *ast_channel_deferred_readq(struct ast_channel *chan);
-
-/*!
- * \brief Start deferring deferrable frames on this channel
- *
- * Sometimes, a channel gets entered into a mode where a "main" application
- * is tasked with servicing frames on the channel, but that application does
- * not need to act on those frames. However, it would be imprudent to simply
- * drop important frames. This function can be called so that important frames
- * will be deferred, rather than placed in the channel frame queue as normal.
- *
- * Hangups are an interesting frame type. Hangups will always be detectable by
- * a reader when a channel is deferring frames. If the defer_hangups parameter
- * is non-zero, then the hangup frame will also be duplicated and deferred, so
- * that the next reader of the channel will get the hangup frame, too.
- *
- * \pre chan MUST be locked before calling
- *
- * \param chan The channel on which frames should be deferred
- * \param defer_hangups Defer hangups in addition to other deferrable frames
- */
-void ast_channel_start_defer_frames(struct ast_channel *chan, int defer_hangups);
-
-/*!
- * \brief Stop deferring deferrable frames on this channel
- *
- * When it is time to stop deferring frames on the channel, all deferred frames
- * will be queued onto the channel's read queue so that the next servicer of
- * the channel can handle those frames as necessary.
- *
- * \pre chan MUST be locked before calling
- *
- * \param chan The channel on which to stop deferring frames.
- */
-void ast_channel_stop_defer_frames(struct ast_channel *chan);
#endif /* _ASTERISK_CHANNEL_H */
diff --git a/main/autoservice.c b/main/autoservice.c
index c3f2427..11c9eab 100644
--- a/main/autoservice.c
+++ b/main/autoservice.c
@@ -59,6 +59,10 @@
unsigned int use_count;
unsigned int orig_end_dtmf_flag:1;
unsigned int ignore_frame_types;
+ /*! Frames go on at the head of deferred_frames, so we have the frames
+ * from newest to oldest. As we put them at the head of the readq, we'll
+ * end up with them in the right order for the channel's readq. */
+ AST_LIST_HEAD_NOLOCK(, ast_frame) deferred_frames;
AST_LIST_ENTRY(asent) list;
};
@@ -73,13 +77,19 @@
static void *autoservice_run(void *ign)
{
ast_callid callid = 0;
+ struct ast_frame hangup_frame = {
+ .frametype = AST_FRAME_CONTROL,
+ .subclass.integer = AST_CONTROL_HANGUP,
+ };
while (!asexit) {
struct ast_channel *mons[MAX_AUTOMONS];
+ struct asent *ents[MAX_AUTOMONS];
struct ast_channel *chan;
struct asent *as;
- int x = 0, ms = 50;
+ int i, x = 0, ms = 50;
struct ast_frame *f = NULL;
+ struct ast_frame *defer_frame = NULL;
AST_LIST_LOCK(&aslist);
@@ -94,6 +104,7 @@
AST_LIST_TRAVERSE(&aslist, as, list) {
if (!ast_check_hangup(as->chan)) {
if (x < MAX_AUTOMONS) {
+ ents[x] = as;
mons[x++] = as->chan;
} else {
ast_log(LOG_WARNING, "Exceeded maximum number of automatic monitoring events. Fix autoservice.c\n");
@@ -121,9 +132,51 @@
ast_callid_threadassoc_change(callid);
f = ast_read(chan);
- if (f) {
+
+ if (!f) {
+ /* No frame means the channel has been hung up.
+ * A hangup frame needs to be queued here as ast_waitfor() may
+ * never return again for the condition to be detected outside
+ * of autoservice. So, we'll leave a HANGUP queued up so the
+ * thread in charge of this channel will know. */
+
+ defer_frame = &hangup_frame;
+ } else if (ast_is_deferrable_frame(f)) {
+ defer_frame = f;
+ } else {
+ /* Can't defer. Discard and continue with next. */
ast_frfree(f);
+ continue;
}
+
+ for (i = 0; i < x; i++) {
+ struct ast_frame *dup_f;
+
+ if (mons[i] != chan) {
+ continue;
+ }
+
+ if (!f) { /* defer_frame == &hangup_frame */
+ if ((dup_f = ast_frdup(defer_frame))) {
+ AST_LIST_INSERT_HEAD(&ents[i]->deferred_frames, dup_f, frame_list);
+ }
+ } else {
+ if ((dup_f = ast_frisolate(defer_frame))) {
+ AST_LIST_INSERT_HEAD(&ents[i]->deferred_frames, dup_f, frame_list);
+ }
+ if (dup_f != defer_frame) {
+ ast_frfree(defer_frame);
+ }
+ }
+
+ break;
+ }
+ /* The ast_waitfor_n() call will only read frames from
+ * the channels' file descriptors. If ast_waitfor_n()
+ * returns non-NULL, then one of the channels in the
+ * mons array must have triggered the return. It's
+ * therefore impossible that we got here while (i >= x).
+ * If we did, we'd need to ast_frfree(f) if (f). */
}
ast_callid_threadassoc_change(0);
@@ -162,7 +215,6 @@
as->orig_end_dtmf_flag = ast_test_flag(ast_channel_flags(chan), AST_FLAG_END_DTMF_ONLY) ? 1 : 0;
if (!as->orig_end_dtmf_flag)
ast_set_flag(ast_channel_flags(chan), AST_FLAG_END_DTMF_ONLY);
- ast_channel_start_defer_frames(chan, 1);
ast_channel_unlock(chan);
AST_LIST_LOCK(&aslist);
@@ -196,6 +248,7 @@
{
int res = -1;
struct asent *as, *removed = NULL;
+ struct ast_frame *f;
int chan_list_state;
AST_LIST_LOCK(&aslist);
@@ -247,7 +300,12 @@
}
ast_channel_lock(chan);
- ast_channel_stop_defer_frames(chan);
+ while ((f = AST_LIST_REMOVE_HEAD(&as->deferred_frames, frame_list))) {
+ if (!((1 << f->frametype) & as->ignore_frame_types)) {
+ ast_queue_frame_head(chan, f);
+ }
+ ast_frfree(f);
+ }
ast_channel_unlock(chan);
ast_free(as);
diff --git a/main/channel.c b/main/channel.c
index 6e88a29..7c8d3a9 100644
--- a/main/channel.c
+++ b/main/channel.c
@@ -1062,25 +1062,6 @@
return tmp;
}
-void ast_channel_start_defer_frames(struct ast_channel *chan, int defer_hangups)
-{
- ast_set_flag(ast_channel_flags(chan), AST_FLAG_DEFER_FRAMES);
- ast_set2_flag(ast_channel_flags(chan), defer_hangups, AST_FLAG_DEFER_HANGUP_FRAMES);
-}
-
-void ast_channel_stop_defer_frames(struct ast_channel *chan)
-{
- struct ast_frame *f;
-
- ast_clear_flag(ast_channel_flags(chan), AST_FLAG_DEFER_FRAMES);
-
- /* Move the deferred frames onto the channel read queue, ahead of other queued frames */
- while ((f = AST_LIST_REMOVE_HEAD(ast_channel_deferred_readq(chan), frame_list))) {
- ast_queue_frame_head(chan, f);
- ast_frfree(f);
- }
-}
-
static int __ast_queue_frame(struct ast_channel *chan, struct ast_frame *fin, int head, struct ast_frame *after)
{
struct ast_frame *f;
@@ -1545,18 +1526,19 @@
int res = 0;
struct timeval start;
int ms;
+ AST_LIST_HEAD_NOLOCK(, ast_frame) deferred_frames;
+
+ AST_LIST_HEAD_INIT_NOLOCK(&deferred_frames);
/* If no other generator is present, start silencegen while waiting */
if (ast_opt_transmit_silence && !ast_channel_generatordata(chan)) {
silgen = ast_channel_start_silence_generator(chan);
}
- ast_channel_lock(chan);
- ast_channel_start_defer_frames(chan, 0);
- ast_channel_unlock(chan);
-
start = ast_tvnow();
while ((ms = ast_remaining_ms(start, timeout_ms))) {
+ struct ast_frame *dup_f = NULL;
+
if (cond && ((*cond)(data) == 0)) {
break;
}
@@ -1571,7 +1553,18 @@
res = -1;
break;
}
- ast_frfree(f);
+
+ if (!ast_is_deferrable_frame(f)) {
+ ast_frfree(f);
+ continue;
+ }
+
+ if ((dup_f = ast_frisolate(f))) {
+ if (dup_f != f) {
+ ast_frfree(f);
+ }
+ AST_LIST_INSERT_HEAD(&deferred_frames, dup_f, frame_list);
+ }
}
}
@@ -1580,8 +1573,17 @@
ast_channel_stop_silence_generator(chan, silgen);
}
+ /* We need to free all the deferred frames, but we only need to
+ * queue the deferred frames if there was no error and no
+ * hangup was received
+ */
ast_channel_lock(chan);
- ast_channel_stop_defer_frames(chan);
+ while ((f = AST_LIST_REMOVE_HEAD(&deferred_frames, frame_list))) {
+ if (!res) {
+ ast_queue_frame_head(chan, f);
+ }
+ ast_frfree(f);
+ }
ast_channel_unlock(chan);
return res;
@@ -3882,36 +3884,6 @@
/* Check for pending read queue */
if (!AST_LIST_EMPTY(ast_channel_readq(chan))) {
int skip_dtmf = should_skip_dtmf(chan);
-
- if (ast_test_flag(ast_channel_flags(chan), AST_FLAG_DEFER_FRAMES)) {
- AST_LIST_TRAVERSE_SAFE_BEGIN(ast_channel_readq(chan), f, frame_list) {
- if (ast_is_deferrable_frame(f)) {
- if(f->frametype == AST_FRAME_CONTROL &&
- (f->subclass.integer == AST_CONTROL_HANGUP ||
- f->subclass.integer == AST_CONTROL_END_OF_Q)) {
- /* Hangup is a special case. We want to defer the frame, but we also do not
- * want to remove it from the frame queue. So rather than just moving the frame
- * over, we duplicate it and move the copy to the deferred readq.
- *
- * The reason for this? This way, whoever calls ast_read() will get a NULL return
- * immediately and can tell the channel has hung up and do what it needs to. Also,
- * when frame deferral finishes, then whoever calls ast_read() next will also get
- * the hangup.
- */
- if (ast_test_flag(ast_channel_flags(chan), AST_FLAG_DEFER_HANGUP_FRAMES)) {
- struct ast_frame *dup;
-
- dup = ast_frdup(f);
- AST_LIST_INSERT_HEAD(ast_channel_deferred_readq(chan), dup, frame_list);
- }
- } else {
- AST_LIST_INSERT_HEAD(ast_channel_deferred_readq(chan), f, frame_list);
- AST_LIST_REMOVE_CURRENT(frame_list);
- }
- }
- }
- AST_LIST_TRAVERSE_SAFE_END;
- }
AST_LIST_TRAVERSE_SAFE_BEGIN(ast_channel_readq(chan), f, frame_list) {
/* We have to be picky about which frame we pull off of the readq because
@@ -10361,15 +10333,9 @@
ast_party_connected_line_copy(ast_channel_connected(macro_chan), connected);
}
- ast_channel_start_defer_frames(macro_chan, 0);
ast_channel_unlock(macro_chan);
retval = ast_app_run_macro(autoservice_chan, macro_chan, macro, macro_args);
-
- ast_channel_lock(macro_chan);
- ast_channel_stop_defer_frames(macro_chan);
- ast_channel_unlock(macro_chan);
-
if (!retval) {
struct ast_party_connected_line saved_connected;
@@ -10417,15 +10383,9 @@
ast_party_redirecting_copy(ast_channel_redirecting(macro_chan), redirecting);
}
- ast_channel_start_defer_frames(macro_chan, 0);
ast_channel_unlock(macro_chan);
retval = ast_app_run_macro(autoservice_chan, macro_chan, macro, macro_args);
-
- ast_channel_lock(macro_chan);
- ast_channel_stop_defer_frames(macro_chan);
- ast_channel_unlock(macro_chan);
-
if (!retval) {
struct ast_party_redirecting saved_redirecting;
@@ -10466,15 +10426,9 @@
ast_party_connected_line_copy(ast_channel_connected(sub_chan), connected);
}
- ast_channel_start_defer_frames(sub_chan, 0);
ast_channel_unlock(sub_chan);
retval = ast_app_run_sub(autoservice_chan, sub_chan, sub, sub_args, 0);
-
- ast_channel_lock(sub_chan);
- ast_channel_stop_defer_frames(sub_chan);
- ast_channel_unlock(sub_chan);
-
if (!retval) {
struct ast_party_connected_line saved_connected;
@@ -10515,15 +10469,9 @@
ast_party_redirecting_copy(ast_channel_redirecting(sub_chan), redirecting);
}
- ast_channel_start_defer_frames(sub_chan, 0);
ast_channel_unlock(sub_chan);
retval = ast_app_run_sub(autoservice_chan, sub_chan, sub, sub_args, 0);
-
- ast_channel_lock(sub_chan);
- ast_channel_stop_defer_frames(sub_chan);
- ast_channel_unlock(sub_chan);
-
if (!retval) {
struct ast_party_redirecting saved_redirecting;
diff --git a/main/channel_internal_api.c b/main/channel_internal_api.c
index 691dec0..a0cbe86 100644
--- a/main/channel_internal_api.c
+++ b/main/channel_internal_api.c
@@ -221,7 +221,6 @@
struct stasis_cp_single *topics; /*!< Topic for all channel's events */
struct stasis_forward *endpoint_forward; /*!< Subscription for event forwarding to endpoint's topic */
struct stasis_forward *endpoint_cache_forward; /*!< Subscription for cache updates to endpoint's topic */
- struct ast_readq_list deferred_readq;
};
/*! \brief The monotonically increasing integer counter for channel uniqueids */
@@ -1729,9 +1728,4 @@
}
return *error_code;
-}
-
-struct ast_readq_list *ast_channel_deferred_readq(struct ast_channel *chan)
-{
- return &chan->deferred_readq;
}
--
To view, visit https://gerrit.asterisk.org/4878
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I45069c779aa3a35b6c863f65245a6df2c7865496
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
More information about the asterisk-commits
mailing list