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

[Matt Jordan]

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


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.


- Matt


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/49aec3a0/attachment-0001.htm>