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

[Mark Michelson]

> On Dec. 26, 2012, 9:29 a.m., Matt Jordan wrote:
> > /trunk/tests/test_taskprocessor.c, lines 449-451
> > <https://reviewboard.asterisk.org/r/2200/diff/2/?file=32190#file32190line449>
> >
> >     As a "nice to have", I think a test checking shutdown of a taskprocessor would be useful. This would do something like:
> >     
> >     * Load up the taskprocessor with a large number of tasks
> >     * Spin up a thread to start consuming said tasks
> >     * Request a shutdown
> >     * Ensure after the shutdown call returns that no tasks exist left in the queue.
> >     
> >     You wouldn't care whether or not the consuming thread actually processed them all, but rather that when you were done, the taskprocessor was gone and no tasks weren't destroyed.

This test isn't going to be easy to write. The problem is that you can't just request a shutdown of a taskprocessor. When you wish to shut down a taskprocessor, you call ast_taskprocessor_unreference(). If the refcount after unreferencing is below a certain amount, then it is interpreted to mean to shut down the taskprocessor. This is how taskprocessors were implemented from the get-go. I'd much rather have separate shutdown and unref calls, but I didn't want to have to touch all the places that currently use taskprocessors.

So for the test in question, I could push a bunch of tasks into the taskprocessor, but then I'd have to kill the taskprocessor in order to shut it down, thus meaning I could not test to ensure that all tasks have been properly freed.

This comment did get me looking more closely, and I have noticed now that we do not pop off all tasks in the taskprocessor and free them when the taskprocessor is destroyed, so I need to get that taken care of.


- 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/64d324e2/attachment-0001.htm>