<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2253/">https://reviewboard.asterisk.org/r/2253/</a>
</td>
</tr>
</table>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2253/diff/1/?file=32490#file32490line245" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/taskprocessor.c</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static char *cli_tps_ping(struct ast_cli_entry *e, int cmd, struct ast_cli_args *a)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">239</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">ao2_ref</span><span class="p">(</span><span class="n">tps</span><span class="p">,</span> <span class="o">-</span><span class="mi">1</span><span class="p">);</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Won't removing this ao2_ref from here and below cause a reference leak?</pre>
</div>
<br />
<p>- jcolp</p>
<br />
<p>On December 24th, 2012, 2:37 p.m., Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated Dec. 24, 2012, 2:37 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Taskprocessors in Asterisk have the potential to leak memory. Taskprocessor tasks take a data pointer as a parameter. Since this task is executed in a separate thread from where the task was pushed, the task data is heap-allocated. When a task runs, the execution callback for the task frees the task data when it completes.
This is fine for typical operation, but in the case where a taskprocessor is shut down while there are still tasks in queue, we certainly don't want to execute the remaining tasks, we simply wish to remove them from the queue and free them. However, given the current model, there is no way for the task data to be freed properly. This patch introduces a change to ast_taskprocessor_push() to take a new destroy() callback. Rather than having the task execution callback be responsible for freeing task data, the freeing of task data is now handled automatically by the taskprocessor itself.
Some points of discussion:
* chan_iax2's usage of taskprocessors is a bit unpredictable. Its task execution callback may or may not end up needing to re-use the task data at a later time. Thus we cannot pass a destroy() callback when the taskprocessor task is pushed. In my view, this may mean that there is still the potential to leak frames in chan_iax2 when its taskprocessor is shut down. Is there a better alternative that may be used here?
* The set of changes presented here is made against 1.8 because the problem exists there. However, this introduces an API change and so it could be deemed an improper change for 1.8. While it is fixing potential memory leaks, the chances of the leaks to occur are low since the lifetime of taskprocessors is tied either to the lifetime of Asterisk or of the module where the taskprocessor was allocated. This means that the majority of the time, the leak would only occur during Asterisk shutdown. Is this set of changes appropriate for 1.8?
Note: this work uncovered a memory leak that occurs during a particular failure case in app_voicemail.c. The fix for this leak should go into all branches whether the taskprocessor API change is approved or not.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Started Asterisk and placed calls through. This exercises the taskprocessors in app_voicemail.c, pbx,c, event.c, and app_queue.c. I also did taskprocessor pings from the CLI. The one aspect that is untested is call completion.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-20786">ASTERISK-20786</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/main/event.c <span style="color: grey">(378204)</span></li>
<li>/branches/1.8/main/pbx.c <span style="color: grey">(378204)</span></li>
<li>/branches/1.8/main/taskprocessor.c <span style="color: grey">(378204)</span></li>
<li>/branches/1.8/apps/app_queue.c <span style="color: grey">(378204)</span></li>
<li>/branches/1.8/apps/app_voicemail.c <span style="color: grey">(378204)</span></li>
<li>/branches/1.8/channels/chan_iax2.c <span style="color: grey">(378204)</span></li>
<li>/branches/1.8/include/asterisk/astobj2.h <span style="color: grey">(378204)</span></li>
<li>/branches/1.8/include/asterisk/taskprocessor.h <span style="color: grey">(378204)</span></li>
<li>/branches/1.8/main/astobj2.c <span style="color: grey">(378204)</span></li>
<li>/branches/1.8/main/ccss.c <span style="color: grey">(378204)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2253/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>