[asterisk-dev] [Code Review] Fix Bridging thread leak

[Mark Michelson]

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

Review request for Asterisk Developers.


Summary
-------

It appears that under normal circumstances, bridging threads are never joined using pthread_join(). The reporter supplied a patch that makes the bridge thread detached rather than joinable. I don't feel this is sufficient because the smart_bridge_operation() joins the thread partway through its method. This implies that it intends for the bridge thread to be completely finished and cleaned up prior to continuing.

In this patch, I have fixed the leak by adding a pthread_join() call to the code in ast_bridge_destroy(). I also have modified bridge_check_dissolve() not to set the stop flag. If bridge_check_dissolve() sets the stop flag for the bridge, then the bridge_thread will set bridge->thread to AST_PTHREADT_NULL. This will make it impossible for ast_bridge_destroy() to be able to tell if there is a thread that needs to be joined.


This addresses bug ASTERISK-19834.
    https://issues.asterisk.org/jira/browse/ASTERISK-19834


Diffs
-----

  /branches/1.8/main/bridging.c 369625 

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


Testing
-------

Ran a SIPp scenario that had calls constantly joining and leaving a conference bridge until a total of 2000 participants had joined and left.

Unfortunately, it's difficult to see if the added pthread_join() calls are doing anything because on my system, neither gdb nor /proc will display zombie threads. So for me anyway, gdb and /proc appear the same before and after applying the patch. More importantly, this patch does not appear to cause any crashes or other problems.


Thanks,

Mark

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