[asterisk-dev] [Code Review]: Add a serializer interface to the threadpool

[David Lee]

> On Feb. 8, 2013, 4:23 p.m., Mark Michelson wrote:
> > /trunk/main/taskprocessor.c, lines 677-687
> > <https://reviewboard.asterisk.org/r/2323/diff/1/?file=33339#file33339line677>
> >
> >     This change is a bit troublesome. One of the reasons that tps_taskprocessor_pop() can return NULL is if the taskprocessor is shutting down. A common idiom in use is to loop calling ast_taskprocessor_execute() until it returns 0. Now if someone decides to shut down a taskprocessor that still has tasks in it, then the code that is looping over ast_taskprocessor_execute will not break out of its loop since ast_taskprocessor_execute will continually return non-zero.
> >     
> >     I think that if tps_taskprocessor_pop() returns NULL, we should unconditionally return 0 because either
> >     1) There were no tasks to pop off
> >     2) The taskprocessor has shut down and so we shouldn't be attempting to execute any more.
> 
> David Lee wrote:
>     The problem there is the mentioned race condition. The listener never gets the 'was_empty' param set, but the processing thread stops processing.
>     
>     The shutdown case is a problem, though. I'll create a unit test for it and take care of it.

Actually; shutting_down is never set by anything, so tps_taskprocessor_pop() only returns NULL when the queue is actually empty.

I could fix the glitch, but I have misgivings about not executing tasks that have been enqueued. That would leak any resources that would have been unref'ed by the dropped tasks. I'd be more inclined to keep the current behavior: shutdown completes once all tasks are finished executing. Objections?


- David


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


On Feb. 8, 2013, 3:24 p.m., David Lee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2323/
> -----------------------------------------------------------
> 
> (Updated Feb. 8, 2013, 3:24 p.m.)
> 
> 
> Review request for Asterisk Developers and Mark Michelson.
> 
> 
> Summary
> -------
> 
> This patch adds the ability to create a serializer from a thread pool. A
> serializer is a ast_taskprocessor with the same contract as a default
> taskprocessor (tasks execute serially) except instead of executing out
> of a dedicated thread, execution occurs in a thread from a
> ast_threadpool. Think of it as a lightweight thread.
> 
> While it guarantees that each task will complete before executing the
> next, there is no guarantee as to which thread from the pool individual
> tasks will execute. This normally only matters if your code relys on
> thread specific information, such as thread locals.
> 
> This patch also fixes a bug in how the 'was_empty' parameter is computed
> for the push callback.
> 
> 
> Diffs
> -----
> 
>   /trunk/include/asterisk/threadpool.h 381085 
>   /trunk/main/taskprocessor.c 381085 
>   /trunk/main/threadpool.c 381085 
>   /trunk/tests/test_threadpool.c 381085 
> 
> Diff: https://reviewboard.asterisk.org/r/2323/diff
> 
> 
> Testing
> -------
> 
> Unit testing.
> 
> 
> Thanks,
> 
> David
> 
>

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