[asterisk-dev] [Code Review] Allow ast_safe_sleep to defer frames similarly to the way autoservice does it.

[Mark Michelson]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/674/
-----------------------------------------------------------

Review request for Asterisk Developers, Russell Bryant and Kevin Fleming.


Summary
-------

Background:
A Digium customer discovered a somewhat odd bug. The setup is that parties A
and B are bridged, and party A places party B on hold. While party B is 
listening to hold music, he mashes a bunch of DTMF. Party A takes party
B off hold while this is happening, but party B continues to hear hold
music. I could reproduce this about 1 in 5 times.

The issue:
When DTMF features are enabled and a user presses keys, the channel that
the DTMF is streamed to is placed in an ast_safe_sleep for 100 ms, the
duration of the emulated tone. If an AST_CONTROL_UNHOLD frame is read
from the channel during the sleep, the frame is dropped. Thus the
unhold indication is never made to the channel that was originally placed
on hold.

The fix:
Originally, I discussed with Kevin possible ways of fixing the specific
problem reported. However, we determined that the same type of problem
could happen in other situations where ast_safe_sleep() is used. Using
autoservice as a model, I modified ast_safe_sleep_conditional() to
defer specific frame types so they can be re-queued once the sleep has
finished. I made a common function for determining if a frame should
be deferred so that there are not two identical switch blocks to
maintain.

I have included Kevin on this review since he and I discussed the 
original fix together. I also have included Russell since this is
touching a pretty core part of Asterisk and I figure he would want
to have a say in this.

The version attached here is for the 1.4 branch since it is the oldest
branch affected by this issue. The 1.6.2 branch and trunk are also
affected.


This addresses bug ABE-2115.
    https://issues.asterisk.org/view.php?id=ABE-2115


Diffs
-----

  /branches/1.4/include/asterisk/channel.h 264541 
  /branches/1.4/main/autoservice.c 264452 
  /branches/1.4/main/channel.c 264541 

Diff: https://reviewboard.asterisk.org/r/674/diff


Testing
-------

I ran the situation from the Digium customer probably about 30 times and was
unable to cause the error to occur. In addition, I also ran the test under
valgrind's memcheck tool and it reported no errors.


Thanks,

Mark