[Asterisk-code-review] sched: Don't allow ast_sched_del to deadlock ast_sched_runq from same... (...asterisk[13])

[Walter Doekes]

Walter Doekes has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/10991 )

Change subject: sched: Don't allow ast_sched_del to deadlock ast_sched_runq from same thread
......................................................................


Patch Set 2:

> So the only scary change is the now-not-mandatory unref call in the macro.

Note that this is still a lot less scary than it sounded.

While writing the reply, I mistakenly started believing that the refcall
was always called -- even for schedules that weren't set. It is not.

Previously it did:

    if (id > -1) \    <-- this if already existed
        refcall; \

Now it still does:

    } else if (_id > -1) { \
        refcall; \

That means:

* if ID was -1 => neither will call `refcall`
* if ID was not -1 AND sched_del succeeded => both will call `refcall`
* only if ID was not -1 AND sched_del failed 10 times => now there's a
  difference in behaviour

ast_sched_del should not fail 10 times. And every time it does fail,
we're in a "Uh oh this shouldn't happen" state anyway.


>> Have you tried running the Asterisk testsuite against this change?

> No. I thought the friendly-automation and its verified+1 did that.

Was my assumption wrong? Please confirm.


> dialog->request_queue_sched_id does nothing at all: that's scary
> actually, does it clean up after someone elses ref?

I take it back. It's not scary, because dialog->request_queue_sched_id
stays -1, so the unref is not called in either code.


-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/10991
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: Ic26777fa0732725e6ca7010df17af77a012aa856
Gerrit-Change-Number: 10991
Gerrit-PatchSet: 2
Gerrit-Owner: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-Comment-Date: Tue, 09 Apr 2019 09:10:36 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190409/507e0dd1/attachment.html>