[asterisk-dev] [Code Review]: Threadpool support: Taskprocessor changes and tests

[Mark Michelson]

> On Dec. 26, 2012, 9:29 a.m., Matt Jordan wrote:
> > /trunk/main/taskprocessor.c, lines 164-166
> > <https://reviewboard.asterisk.org/r/2200/diff/2/?file=32189#file32189line164>
> >
> >     The current behavior is that a shutdown won't immediately kill the taskprocessor - it instead changes the state of the pvt so that when the taskprocessor next goes into the idle state, it will detect the shutdown request and shut itself down.
> >     
> >     There isn't anything inherently wrong with this, but it does have two limitations that I can see.
> >     
> >     1) When the system gets shut down, a taskprocessor can't be immediately cancelled. If a large number of items are queued up, the taskprocessor will continue working them until it finally goes idle. Consumers of the taskprocessor have to know that shutting down a taskprocessor does not stop queued tasks, and that they as consumers have to continue to safely handle said tasks even when the system is going down. This means they have to be written in such a fashion that they don't attempt to use any resources that may have already been reclaimed, i.e., lots of defensive programming.
> >     
> >     This may warrant a comment somewhere.
> >     
> >     2) If you had a condition where a producer continued to put tasks on a taskprocessor, there would be no way for a consumer to cancel the taskprocessor. Not sure that's really a problem, but it is a limitation of the design.

Your analysis isn't quite correct. We don't have to actually wait until the taskprocessor thread goes idle. The reason is that the taskprocessor will go idle the next time it tries to grab a task to process. This is because tps_taskprocessor_pop() returns NULL if the taskprocessor is shutting down, thus causing ast_taskprocessor_execute() to return 0. Now, there could still be a delay if a long-running process is executing, but there's no way around that at all.


> On Dec. 26, 2012, 9:29 a.m., Matt Jordan wrote:
> > /trunk/main/taskprocessor.c, lines 669-679
> > <https://reviewboard.asterisk.org/r/2200/diff/2/?file=32189#file32189line669>
> >
> >     If the taskprocessor own's the lifetime of the listener, then why does the listener bump the ref count of the taskprocessor? It'd be nice if the lifetimes of these two objects weren't circular.

Unfortunately, the taskprocessor, as designed, does not own the lifetime of the listener. This is because the listener gets created, and then the taskprocessor is created after. There is nothing to prevent the original creator of the listener from keeping its reference, thus resulting in the listener outliving the taskprocessor. I can strongly recommend that anyone creating a taskprocessor should lose their listener references after creating the taskprocessor, but there'd be no way, through code, to enforce such a guideline.


- Mark


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2200/#review7578
-----------------------------------------------------------


On Nov. 19, 2012, 4:39 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2200/
> -----------------------------------------------------------
> 
> (Updated Nov. 19, 2012, 4:39 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> For the new SIP channel driver, we would really like to use a thread pool to handle the tasks. In order to do this correctly, I am designing a general-purpose thread pool that can be used for SIP or for anything else in Asterisk if desired. I am basing the design of the thread pool off of the thread pool used in Asterisk SCF.
> 
> The first set of changes is to essentially gut main/taskprocessor.c. Taskprocessors in Asterisk are currently limited to managing a single thread of execution. The set of changes provided here introduces a listener model. When a taskprocessor's state changes (e.g. a task is added, the queue becomes empty) the listener is notified. The listener may then take whatever action it desires. If the listener wants to execute all tasks in a single thread, it can. If it would rather allow for multiple threads to execute threads, it can facilitate that instead.
> 
> To prove the concept, a default taskprocessor listener was created that behaves exactly like taskprocessors always have. Unit tests are included in this patch set that show that the default taskprocessor listener behaves as expected. A unit test has also been added to ensure that listener callbacks are called when expected and that the data provided in the callbacks is accurate.
> 
> In an effort to not cause far-reaching changes, I have kept the API for taskprocessors backwards-compatible. Honestly, I'm not a big fan of current method of shutting down a taskprocessor, but it works nonetheless.
> 
> 
> This addresses bug ASTERISK-20691.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20691
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/taskprocessor.h 376499 
>   /trunk/main/astobj2.c 376499 
>   /trunk/main/taskprocessor.c 376499 
>   /trunk/tests/test_taskprocessor.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/2200/diff
> 
> 
> Testing
> -------
> 
> In addition to the unit tests provided, I also ran calls to be sure that mechanisms in Asterisk that use taskprocessors still functioned as expected. I verified that the event system and that the PBX function as expected.
> 
> 
> Thanks,
> 
> Mark
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130107/0a5ed0f4/attachment.htm>