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

[Matt Jordan]

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



/trunk/main/taskprocessor.c
<https://reviewboard.asterisk.org/r/2200/#comment14416>

    I'd go ahead and put an \internal \brief on this



/trunk/main/taskprocessor.c
<https://reviewboard.asterisk.org/r/2200/#comment14417>

    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.



/trunk/main/taskprocessor.c
<https://reviewboard.asterisk.org/r/2200/#comment14418>

    I don't think you need the ao2_unlock here anymore, since you removed the corresponding ao2_lock



/trunk/main/taskprocessor.c
<https://reviewboard.asterisk.org/r/2200/#comment14419>

    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.



/trunk/tests/test_taskprocessor.c
<https://reviewboard.asterisk.org/r/2200/#comment14420>

    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.


- Matt


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/20121226/7efdee35/attachment-0001.htm>