[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 3:

(5 comments)

https://gerrit.asterisk.org/#/c/10991/2/include/asterisk/sched.h 
File include/asterisk/sched.h:

https://gerrit.asterisk.org/#/c/10991/2/include/asterisk/sched.h@84 
PS2, Line 84: 			usleep(1); \
> I do think it is bad if id changes while we are trying to delete the scheduled item.

Well indeed. If it changes, then all bets are off anyway. So I preferred my version for consistent behaviour.

But I can certainly live with your version :)


https://gerrit.asterisk.org/#/c/10991/2/main/sched.c 
File main/sched.c:

https://gerrit.asterisk.org/#/c/10991/2/main/sched.c@121 
PS2, Line 121: 	pthread_t currently_executing_on_thread_id;
> A better member name with doxygen: […]
Done


https://gerrit.asterisk.org/#/c/10991/2/main/sched.c@630 
PS2, Line 630: 	} else if (con->currently_executing_on_thread_id == pthread_self()) {
             : 		/* We might trample on deleted memory at this point. Not good,
             : 		 * but it's better than a deadlock.
             : 		 * Thou shalt not reschedule things from a scheduled callback!
             : 		 */
             : 		ast_log(LOG_ERROR,
             : 			"BUG! Trying to delete sched %d from the same callback %p (sched %d). "
             : 			"Ignoring so we don't deadlock\n",
             : 			id, con->currently_executing->callback, con->currently_executing->sched_id->id);
             : 		ast_log_backtrace();
             : 		/* We'll return -1 below, because s is NULL. The caller
             : 		 * will rightly assume that the unscheduling failed. */
> This is the wrong place for this check.  You are restricting a valid scenario. […]
Right. Fixing!


https://gerrit.asterisk.org/#/c/10991/2/main/sched.c@642 
PS2, Line 642: 	} else if (con->currently_executing && (id == con->currently_executing->sched_id->id)) {
> Within this if clause we have determined that we are trying to delete the scheduled item while it is […]
That is better indeed.


https://gerrit.asterisk.org/#/c/10991/2/main/sched.c@796 
PS2, Line 796: 		con->currently_executing_on_thread_id = 0;
> This is not needed when the ast_sched_del() check is fixed.
Done



-- 
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: 3
Gerrit-Owner: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Walter Doekes <walter+asterisk at wjd.nu>
Gerrit-Comment-Date: Wed, 17 Jul 2019 13:08:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Richard Mudgett <rmudgett at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190717/155d1cd0/attachment-0001.html>