[asterisk-dev] [Code Review]: Revamp app_confbridge into something resembling a state machine

[Matt Jordan]

> On Sept. 24, 2012, 8:55 p.m., Matt Jordan wrote:
> > This is really for the test review, but I wanted to post it on this review since it really is dependent on the behavior in this patch.  The following are the 'high-level' scenarios that need to be tested.  In each scenario, multiple permutations are possible based on the presence of user profile flags - how in depth we go with this depends on how much we feel these need to be exercised.
> > 
> > Marked users:
> >   Conference should automatically start for Marked users and those users should
> >   never be in an inactive conference state.
> > 
> > Marked/Unmarked users:
> >   Conference should automatically start with two or more Unmarked users, or if a
> >   Marked user enters.  An Unmarked user exiting should not send the conference
> >   to an inactive state if the only user left is Marked; otherwise the
> >   conference should go inactive and the user's profile options honored.
> > 
> > Marked/WaitMarked Users:
> >   WaitMarked users should not trigger conference starting.  Marked users should
> >   automatically trigger a conference starting.  When marked users leave a
> >   conference, WaitMarked users should be put back into an inactive conference.
> > 
> > Marked/Unmarked Users:
> >   Unmarked users should not trigger a conference start unless there are two or
> >   more of them.  A Marked user should automatically trigger a conference start.
> >   If an Unmarked user leaves and a Marked user is present and the last person,
> >   the conference should continue.  If a Marked user leaves and an Unmarked user
> >   is the last person, the conference should be put into an inactive state.
> > 
> > Unmarked Users:
> >   Unmarked users should not trigger a conference start unless there are two or
> >   more of them.  If a conference is down to its last person and the last person
> >   is an Unmarked user, the conference should be put into an inactive state.
> > 
> > Unmarked/WaitMarked Users:
> >   Unmarked and WaitMarked users should never interact.  If a WaitMarked user
> >   enters the conference at any point in time, and no Marked user exists in
> >   the conference, the conference should appear as if it were inactive.  At
> >   the same time, Unmarked users should not notice the presence of WaitMarked
> >   users who are in an inactive state.  Regardless of the number of WaitMarked
> >   users waiting, a single Unmarked user should be put in an inactive state if
> >   they are the only Unmarked user in a conference.  If two or more Unmarked
> >   users are in the conference then the conference should be active; if the
> >   count drops to a single Unmarked user then the conference should be in an
> >   inactive state.
> > 
> > WaitMarked users:
> >   Regardless of the number of WaitMarked users who attempt to join the
> >   conference, the conference remains in an inactive state.  The conference
> >   remains in the inactive state when WaitMarked users leave.
> > 
> > Unmarked/WaitMarked/Marked users:
> >   As a permutation to the Unmarked/WaitMarked scenario, it should be verified
> >   that when multiple Unmarked users are in a conference with multiple WaitMarked
> >   users waiting to join the conference, the presence of a Marked user causes the
> >   WaitMarked users to join, and the leaving of the last Marked user causes the
> >   WaitMarked users to re-enter an inactive state while the Unmarked users remain
> >   in an active conference.
> >

Just realized I had Marked/Unmarked in there twice.  Whoops!  Description differences aside, at least the behavior described was the same between the two.


- Matt


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


On Sept. 17, 2012, 11:51 a.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2072/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2012, 11:51 a.m.)
> 
> 
> Review request for Asterisk Developers and Neil Tallim.
> 
> 
> Summary
> -------
> 
> Thank's to flan's tireless testing, issue reporting, and patches it became clear that app_confbridge had some complex logic and was missing some corner cases. In particular, it didn't handle mixing unmarked and waitmarked users very well and was inconsistent in how MOH and muting was applied.  flan's patches seem to fix the issue, but they highlight how hard the code could be to maintain. In an attempt to make things easier to maintain and to more fully enumerate the various cases that exist, I have attempted to break up the logic into a state machine-like setup.
> 
> This should fix all of the issues in the linked bugs, as well as a couple of other issues. I still need to add some documentation, but I thought I'd give people a chance to look at it test it out, and comment.
> 
> One thing to note is that this solution involves treating waitmarked users without a marked user as essentially outside of the conference. For now, I have the announce_user_count stuff taking into account both the counts for waiting and active users which can lead to things like having a single waiting user followed by an unmarked user and the announcement that "there is one other user in the conference" (because we count the waiting user there) followed by "you are the only user in the conference" (because that prompt plays when you are the only active user) being played to the unmarked user. This is easy to fix, we just need to decide which way to go. Count users who are essentially outside the conference (they can't speak or hear) or not?
> 
> 
> This addresses bugs ASTERISK-19562, ASTERISK-19726 and ASTERISK-20181.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19562
>     https://issues.asterisk.org/jira/browse/ASTERISK-19726
>     https://issues.asterisk.org/jira/browse/ASTERISK-20181
> 
> 
> Diffs
> -----
> 
>   /branches/10/apps/app_confbridge.c 373118 
>   /branches/10/apps/confbridge/conf_state.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_empty.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_inactive.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_marked.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_multi.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_single.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_single_marked.c PRE-CREATION 
>   /branches/10/apps/confbridge/include/conf_state.h PRE-CREATION 
>   /branches/10/apps/confbridge/include/confbridge.h 373118 
> 
> Diff: https://reviewboard.asterisk.org/r/2072/diff
> 
> 
> Testing
> -------
> 
> Lots of calls. Need to do more formalized testing.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120925/a169373f/attachment.htm>