<p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/10991">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10991/2/include/asterisk/sched.h">File include/asterisk/sched.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10991/2/include/asterisk/sched.h@84">Patch Set #2, Line 84:</a> <code style="font-family:monospace,monospace"> usleep(1); \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Since id is a variable that could theoretically change on us while we are attempting to delete the scheduled item, we need to refresh the _id value in the loop after the usleep().</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">while (...) {<br> usleep(1);<br> _id = id;<br>}</pre><p style="white-space: pre-wrap; word-wrap: break-word;">I do think it is bad if id changes while we are trying to delete the scheduled item. But paranoia. :)</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.asterisk.org/#/c/10991/2/main/sched.c">File main/sched.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10991/2/main/sched.c@121">Patch Set #2, Line 121:</a> <code style="font-family:monospace,monospace"> pthread_t currently_executing_on_thread_id;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">A better member name with doxygen:<br>/*! Valid while currently_executing is not NULL */<br>pthread_t executing_thread_id;</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10991/2/main/sched.c@630">Patch Set #2, Line 630:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> } else if (con->currently_executing_on_thread_id == pthread_self()) {<br> /* We might trample on deleted memory at this point. Not good,<br> * but it's better than a deadlock.<br> * Thou shalt not reschedule things from a scheduled callback!<br> */<br> ast_log(LOG_ERROR,<br> "BUG! Trying to delete sched %d from the same callback %p (sched %d). "<br> "Ignoring so we don't deadlock\n",<br> id, con->currently_executing->callback, con->currently_executing->sched_id->id);<br> ast_log_backtrace();<br> /* We'll return -1 below, because s is NULL. The caller<br> * will rightly assume that the unscheduling failed. */<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is the wrong place for this check. You are restricting a valid scenario. It is permissible for a callback to delete other scheduled items. It cannot delete itself because that is a deadlock and there is a defined way for a scheduled callback to delete itself.</p><p style="white-space: pre-wrap; word-wrap: break-word;">At this point we could be trying to delete a scheduled item that simply doesn't exist or the scheduled callback has already deleted itself because it returned zero.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10991/2/main/sched.c@642">Patch Set #2, Line 642:</a> <code style="font-family:monospace,monospace"> } else if (con->currently_executing && (id == con->currently_executing->sched_id->id)) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Within this if clause we have determined that we are trying to delete the scheduled item while it is running. If the scheduled item is trying to delete itself using this function then that is when you should trigger your error message with backtrace and ignore the deletion to prevent the deadlock.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">} else if (con->currently_executing && (id == ...)) {<br> if (con->currently_executing_on_thread_id == pthread_self()) {<br> /* The scheduled callback is trying to delete itself. Not good as that is a deadlock. */<br> ast_log(LOG_ERROR,<br> "BUG! Trying to delete sched %d from within the callback %p. Ignoring so we don't deadlock\n",<br> id, con->currently_executing->callback);<br> ast_log_backtrace();<br> /* We'll return -1 below because s is NULL. The caller will rightly assume that the unscheduling failed. */<br> } else {<br> ... what was previously in the if clause ...<br> }<br>}</pre></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/10991/2/main/sched.c@796">Patch Set #2, Line 796:</a> <code style="font-family:monospace,monospace"> con->currently_executing_on_thread_id = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This is not needed when the ast_sched_del() check is fixed.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/10991">change 10991</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/10991"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-Change-Id: Ic26777fa0732725e6ca7010df17af77a012aa856 </div>
<div style="display:none"> Gerrit-Change-Number: 10991 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Walter Doekes <walter+asterisk@wjd.nu> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Joshua Colp <jcolp@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Walter Doekes <walter+asterisk@wjd.nu> </div>
<div style="display:none"> Gerrit-Reviewer: mattf <creslin@digium.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 29 May 2019 22:07:10 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>