[asterisk-bugs] [Asterisk 0015285]: [patch] reworked chan_ooh323

[Asterisk Bug Tracker]

A NOTE has been added to this issue. 
====================================================================== 
https://issues.asterisk.org/view.php?id=15285 
====================================================================== 
Reported By:                may213
Assigned To:                russell
====================================================================== 
Project:                    Asterisk
Issue ID:                   15285
Category:                   Addons/chan_ooh323
Reproducibility:            random
Severity:                   feature
Priority:                   normal
Status:                     assigned
Target Version:             1.6.x Version Tracker
Asterisk Version:           SVN 
Regression:                 No 
SVN Branch (only for SVN checkouts, not tarball releases):  trunk 
SVN Revision (number only!):  
Request Review:              
====================================================================== 
Date Submitted:             2009-06-07 18:51 CDT
Last Modified:              2009-07-30 16:59 CDT
====================================================================== 
Summary:                    [patch] reworked chan_ooh323
Description: 
There is patch to asterisk-addons-1.6.1.0 for reworked version of 
chan_ooh323 channel module.

main subject of changes is performance and scalability improvement.
one processing thread is replaced with many threads (one for cmdChannel
of EndPoint, one for incoming call creation, one for call processing).

chan_ooh323 with this reworking is more stable and have good performance
in compare with chan_h323.
chan_h323 have memory leaks in all my asterisk systems (which have
versions from 1.2 up to 1.6.1) and grow more than 2Gb virtual memory after
proccessing about 200000 calls. After many years of solving this trouble i
cancel this work and begin work on chan_ooh323.

asterisk 1.6.1 with this version of chan_ooh323 have uptime 4 days and
proccess about 400 000 calls and have 98Mb virtual memory with 4 active
channels and no memory leak detected by asterisk malloc debug system.
(all modules include OOH323 stack code are compiled with MALLOC_DEBUG
options)
Before testing 1.6.1 i work with 1.4 version and had 8 days uptime
asterisk with more than 1000000 calls processed without leaks.

Also there is small changes for new options - incoming call limit, 
numbering of calls, t35country code and vendor/version identification
(albania code is not liked for me ;))

More detail list of changes/improvements is in Changes-ooh323.eng.

======================================================================
Relationships       ID      Summary
----------------------------------------------------------------------
related to          0015046 [patch] Data truncated in ooh323_reques...
====================================================================== 

---------------------------------------------------------------------- 
 (0108419) dimas (reporter) - 2009-07-30 16:59
 https://issues.asterisk.org/view.php?id=15285#c108419 
---------------------------------------------------------------------- 
Ok, I re-read it the patch and still do not get where am I wrong.
My point was that every time cond_wait is called you should be providing
the same mutex. This means you should NOT init new mutex each time you are
about to call timedwait on some conditional variable but should init the
mutex once at the same place you are initializing your conditional
variable. In your code it was violated.

Now you switched to semaphores and mutex is not used anymore so there is
no such a problem. However:
1. I'm not sure about guidelines regarding using semaphores in Asterisk -
I see dozen of files using conditional variables and only one usage of
semaphore. You better check with Digium guys regarding that.
2. Note that sem_post is more like signal of conditional variable while
your previous patch used broadcast not signal. I have no idea which one is
correct in your case because I'm looking not at the whole code but at the
patch only. So I'm just trying to attract your attention to the fact that
sem_post/cond_broadcast behavior is very different.
3. Note that if you have any mutex locked before entering sem_timedwait,
you will keem them locked for the whole duration of the call. So for
example if another thread needs to grab call->Lock in order to progress and
signal the semaphone - it won't be able. This is the task conditional
variable solves - when entering cond_wait, the mutex you pass is released
but it is guaranteed to be locked back before cond_wait returns. 

By the way, whatever you use - conditional variables or semaphores, you
better  be checking return value of timedwait... 

Issue History 
Date Modified    Username       Field                    Change               
====================================================================== 
2009-07-30 16:59 dimas          Note Added: 0108419                          
======================================================================