<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">"this could potentially have unintended consequences"</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">The change to the scheduler itself (the pthread_self) check is safe enough: before it would deadlock, now it ERRORs and allows you to continue (with a possibly degraded asterisk).</p><p style="white-space: pre-wrap; word-wrap: break-word;">So the only scary change is the now-not-mandatory unref call in the macro. However, doing it like this changeset, makes it more aligned with how its friend, the AST_SCHED_REPLACE_VARIABLE_UNREF call, already works: I would not be surprised if most of the callers of AST_SCHED_DEL_UNREF already assumed behaviour would be similar.</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (!_res && _data) // <-- only if id > -1 would we do the unref<br> unrefcall; /* should ref _data! */</pre><p style="white-space: pre-wrap; word-wrap: break-word;">(I think the caller in res/res_sorcery_memory_cache.c assumes so, when unreffing cache after getting -1. Correct me if I'm wrong.)</p><p style="white-space: pre-wrap; word-wrap: break-word;"><br>> Tons of things use the scheduler and and written with the old behavior in mind.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Are you sure? I'm only touching the AST_SCHED_DEL_UNREF macro, and the users are only:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">$ find . '!' -path './.pc/*' '(' -name '*.c' -o -name '*.cpp' -o -name '*.h' ')' | xargs grep AST_SCHED_DEL_UNREF | sed -e 's/:.*//' | uniq -c<br> 2 ./res/res_sorcery_memory_cache.c<br> 2 ./res/res_rtp_asterisk.c<br> 1 ./res/res_pjsip_pubsub.c<br> 34 ./channels/chan_sip.c<br> 1 ./include/asterisk/sched.h</pre><p style="white-space: pre-wrap; word-wrap: break-word;">That is:</p><ul><li>no macro's using the AST_SCHED_DEL_UNREF macro</li><li>5 occurrences outside of chan_sip; they can be manually reviewed</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">That leaves us with the chan_sip case:</p><ul><li>I have one example where this *fixes* a real deadlock problem (I'm fairly confident this used to be a double-free in Asterisk 11, as I mentioned in the ticket)</li><li>the other occurrences; I cannot vouch for all of them, but generally the trend seems to be: an item is expected to be scheduled (return non -1) and the item is then unreffed (see [2], but see also [3])</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">And chan_sip is the place where this *fixes* things.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Making the unref optional, generally only causes a memory leak. Whereas calling it too often (like what happens without the patch) causes double-frees.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I know which one I prefer.</p><p style="white-space: pre-wrap; word-wrap: break-word;"><br>> Have you tried running the Asterisk testsuite against this change?</p><p style="white-space: pre-wrap; word-wrap: break-word;">No. I thought the friendly-automation and its verified+1 did that. I cannot find the particular run in Jenkins though, I believe it may have been pruned by now.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Let me counter that: have you tried running reproducing_sched_unref_self_deadlock.patch[1] ?</p><p style="white-space: pre-wrap; word-wrap: break-word;">[1] https://issues.asterisk.org/jira/secure/attachment/58136/ASTERISK-28282_reproducing_sched_unref_self_deadlock.patch</p><p style="white-space: pre-wrap; word-wrap: break-word;">If you load that into a an Asterisk and do some moderate traffic on it, the deadlock occurs. If you patch Asterisk with this changeset, the deadlock is replaced by a benign ERROR message. (Possibly with a ref-/memleak in the rarest of circumstances.)</p><p style="white-space: pre-wrap; word-wrap: break-word;"><br>Lastly, I have some live behaviour that can at least partially attest to some success [4].</p><p style="white-space: pre-wrap; word-wrap: break-word;">----</p><p style="white-space: pre-wrap; word-wrap: break-word;">[2] Good behaviour, it cleans up immediately (this is the common case I see in chan_sip):</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> pvt->autokillid = ast_sched_add(sched, ms, __sip_autodestruct, pvt);<br> if (pvt->autokillid < 0) {<br> /* Uh Oh. Expect bad behavior. */<br> dialog_unref(pvt, "Failed to schedule autokillid");</pre><p style="white-space: pre-wrap; word-wrap: break-word;">and:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (-1 < pvt->autokillid) {<br> append_history(pvt, "CancelDestroy", "");<br> AST_SCHED_DEL_UNREF(sched, pvt->autokillid,</pre><p style="white-space: pre-wrap; word-wrap: break-word;">(dialog->reinviteid, behaves like this too, as does dialog->autokillid and dialog->provisional_keepalive_sched_id)<br>(dialog->request_queue_sched_id does nothing at all: that's scary actually, does it clean up after someone elses ref?)</p><p style="white-space: pre-wrap; word-wrap: break-word;">(This is *not* an exhaustive list.)</p><p style="white-space: pre-wrap; word-wrap: break-word;">----</p><p style="white-space: pre-wrap; word-wrap: break-word;">[3] Unexpected/bad behaviour, it does an extra ref:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> p->waitid = ast_sched_add(sched, wait, sip_reinvite_retry, p);<br> if (p->waitid < 0) {<br> /* Uh Oh. Expect bad behavior. */<br> dialog_ref(p, "Failed to schedule waitid"); // <-- this is not supposed to happen though, and see [5]</pre><p style="white-space: pre-wrap; word-wrap: break-word;">And no checks for -1 this time:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> AST_SCHED_DEL_UNREF(sched, p->initid, dialog_unref(p, "when you delete the initid sched, you should dec the refcount for the stored dialog ptr"));</pre><p style="white-space: pre-wrap; word-wrap: break-word;">But note that if waitid is altered (set to -1) along the road, it is also unreffed. Only if ast_sched_add fails, do we appear to get a refleak after this changeset.</p><p style="white-space: pre-wrap; word-wrap: break-word;">And (as I hinted at above) REPLACE_UNREF already uses the "I expect this to run only if it succeeds"-semantics:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> AST_SCHED_REPLACE_UNREF(p->initid, sched, p->timer_b, auto_congest, p,<br> dialog_unref(_data, "dialog ptr dec when SCHED_REPLACE del op succeeded"),<br> dialog_unref(p, "dialog ptr dec when SCHED_REPLACE add failed"),<br> dialog_ref(p, "dialog ptr inc when SCHED_REPLACE add succeeded") );</pre><p style="white-space: pre-wrap; word-wrap: break-word;">(Wait what!? Using "_data" which is declared inside the AST_SCHED_REPLACE_UNREF macro? Aw.. now I have to bleach my eyes. Someone even did that in res/res_pjsip_pubsub.c.)</p><p style="white-space: pre-wrap; word-wrap: break-word;">In any case: _data (which would be the previous "p") would only get unreffed if waitid > -1 in that replace case. The AST_SCHED_DEL_UNREF semantics should be the same, IMHO.</p><p style="white-space: pre-wrap; word-wrap: break-word;">----</p><p style="white-space: pre-wrap; word-wrap: break-word;">[4] On a random machine with this changeset, I see this:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> # asterisk -nrx 'core show uptime'<br> System uptime: 1 week, 4 days, 14 hours, 13 minutes, 23 seconds<br> Last reload: 5 days, 12 hours, 32 minutes, 1 second</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> # asterisk -nrx 'sip show sched'<br> Highwater = 593<br> schedcnt = 276<br> ...</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> # asterisk -nrx 'core show channels count'; \<br> asterisk -nrx 'sip show objects' | awk '/^refcount/{n+=1;s+=$2}END{print n " " s}'<br> 274 active channels<br> 139 active calls<br> 804000 calls processed<br> 303 1150</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> ^-- 303 objects, 1150 refs; sounds pretty legit</pre><p style="white-space: pre-wrap; word-wrap: break-word;">----</p><p style="white-space: pre-wrap; word-wrap: break-word;">[5] Surely that is a bug?</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">$ grep -A1 'Expect bad behavior' channels/chan_sip.c | grep '_ref\|_unref'<br> ao2_t_ref(pkt, -1, "Failed to schedule stop packet retransmission action");<br> dialog_unref(pvt, "Failed to schedule cancel destroy action");<br> dialog_unref(pvt, "Failed to schedule autokillid");<br> dialog_unref(p, "Failed to schedule destroy action");<br> dialog_unref(pvt, "Failed to schedule stop provisional keepalive action");<br> dialog_unref(pvt, "Failed to schedule stop reinviteid action");<br> dialog_unref(p, "Failed to schedule reinviteid");<br> ao2_t_ref(mwi, -1, "Failed to schedule shutdown MWI subscription action");<br> ao2_t_ref(mwi, -1, "Failed to schedule mwi resub");<br> ao2_t_ref(mwi, -1, "Failed to schedule start MWI subscription action");<br> ao2_t_ref(reg, -1, "Failed to schedule reregister timeout");<br> ao2_t_ref(reg, -1, "Failed to schedule start reregister timeout action");<br> ao2_t_ref(reg, -1, "Failed to schedule stop register timeout action");<br> ao2_t_ref(reg, -1, "Failed to schedule register timeout");<br> ao2_t_ref(reg, -1, "Failed to schedule start register timeout action");<br> dialog_unref(pvt, "Failed to schedule check pending actions action");<br> dialog_unref(pvt, "Failed to schedule stop reinvite retry action");<br> dialog_ref(p, "Failed to schedule waitid");<br> dialog_unref(pvt, "Failed to schedule stop t38id action");<br> dialog_unref(pvt, "Failed to schedule t38id");<br> dialog_unref(pvt, "Failed to schedule start t38id action");<br> dialog_unref(pvt, "Failed to schedule stop session timer action");<br> dialog_unref(pvt, "Failed to schedule start session timer action");<br> ao2_t_ref(reg, -1, "Failed to schedule cleanup_registration action");</pre><p style="white-space: pre-wrap; word-wrap: break-word;">^-- there's only 1 doing a ref instead of an unref; looks like a typo to me</p><p style="white-space: pre-wrap; word-wrap: break-word;">----</p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/10991">View Change</a></p><ul style="list-style: none; padding: 0;"></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: 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: Tue, 12 Mar 2019 09:25:00 +0000 </div>
<div style="display:none"> Gerrit-HasComments: No </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>