apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Henry Jen <henry...@ztune.net>
Subject Re: Thread pool prototype
Date Wed, 03 May 2006 21:39:35 GMT
Joe Orton wrote:
> Some quick review in-line below.  Where do you envisage this code going, 
> apr-util or apr?  I think the former is more appropriate since the code 
> is entirely platform-independent; it needs a build system patch too 
> either way :).  The code style is good apart from the use of the space 
> after * in pointer arguments; should be "apr_pool_t *pool" not 
> "apr_pool_t * pool".
> 

The patch is for apr-util. But I have no preference. The build system 
for autotools works as is. Windows is another story. :-)

That's a result of using GNU indent as suggested at 
http://httpd.apache.org/dev/styleguide.html.

indent -i4 -npsl -di0 -br -nce -d0 -cli0 -npcs -nfc1 -nut

> On Tue, May 02, 2006 at 10:46:19AM -0700, Henry Jen wrote:
>> +#ifdef __cplusplus
>> +extern "C"
>> +{
>> +#if 0
>> +};
>> +#endif
> 
> This is weird, what's it for?  To un-confuse an editor's indenting logic 
> or something?
>

Un-confuse indent, yes.

>> +#endif /* __cplusplus */
>> +
>> +/** Opaque Thread Pool structure. */
>> +typedef struct _apr_thread_pool apr_thread_pool_t;
> 
> Don't begin type or variable names with an underscore (such things are 
> reserved by the C library).
> 

Right, I need to fix this.
I misunderstood it as reserved to be be used by "implementation" of 
"library". ;-)


> ...
>> +APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
>> +                                                 apr_pool_t * pool,
> 
> It is conventional to have the pool as the last argument (or first in 
> rare cases).
> 

Thanks for pointing out. I happen to refer to prototype with 2 
parameters and apr_array_make. :-)

>> +struct _apr_thread_pool
>> +{
>> +    apr_pool_t *parent;
> 
> that field doesn't appear to be used?
>

Removed in latest patch.

>> +
>> +/*
>> + * NOTE: This function is not thread safe by itself. Caller should hold the lock
>> + */
>> +apr_thread_pool_task_t *apr_thread_pool_tasks_pop(apr_thread_pool_t * me)
> 
> is this function supposed to be public or private?  It should be static 
> if the latter, and shouldn't have an apr_ prefix so it's easy for the 
> reader to distinguish.

Will fix.

>> +    APR_RING_ELEM_INIT(elt, link);
>> +    elt->thd = t;
>> +    elt->stop = 0;
>> +    apr_thread_mutex_lock(me->lock);
>> +    ++me->idle_cnt;
>> +    APR_RING_INSERT_TAIL(me->idle_thds, elt, _apr_thread_list_elt, link);
>> +    apr_thread_mutex_unlock(me->lock);
>> +
>> +    apr_thread_mutex_lock(me->cond_lock);
>> +    apr_thread_cond_wait(me->cond, me->cond_lock);
>> +    apr_thread_mutex_unlock(me->cond_lock);
>> +
>> +    apr_thread_mutex_lock(me->lock);
> 
> Lots of missing error checking for mutex/condvar calls throughout this 
> code.
> 

Hmm, what do you suggest to do?

I don't expect error to occurs unless something seriously screwed up. 
Those are blocking call, no timeout is specified.

>> +{
>> +    apr_thread_pool_t *_self = me;
>> +
>> +    _self->terminated = 1;
>> +    apr_thread_pool_idle_max_set(_self, 0);
>> +    while (_self->busy_cnt) {
>> +        apr_sleep(20 * 1000);   /* spin lock with 20 ms */
>> +    }
> 
> I don't think that's strictly safe, it should be using the atomic 
> functions if it must be done without the mutex held.
>

It is not intent to. When set the terminated to 1, the count shouldn't 
increase, only decrease. A quick snapshot works well.


> ...
>> +APR_DECLARE(apr_status_t) apr_thread_pool_create(apr_thread_pool_t ** me,
>> +                                                 apr_pool_t * pool,
>> +                                                 apr_size_t init_threads,
>> +                                                 apr_size_t max_threads)
>> +{
>> +    apr_thread_t *t;
>> +    apr_status_t rv = APR_SUCCESS;
>> +    apr_pool_t *p;
>> +
>> +    if (!me) {
>> +        return APR_BADARG;
>> +    }
> 
> Don't do argument validation, segfaulting if the caller screws up is 
> standard practice.
>

In this case, agree. :-)

>> +    *me = (apr_thread_pool_t *) apr_pcalloc(pool, sizeof(**me));
>> +    if (!*me) {
>> +        return APR_ENOMEM;
>> +    }
> 
> ... or OOM checking again.
>

I don't understand what do you suggest to do.

> ...
>> +static apr_thread_pool_task_t *task_new(apr_thread_pool_t * me,
>> +                                        apr_thread_start_t func,
>> +                                        void *param, apr_byte_t priority)
>> +{
>> +    apr_thread_pool_task_t *t;
>> +
>> +    if (APR_RING_EMPTY(me->recycled_tasks, _apr_thread_pool_task, link)) {
>> +        t = apr_pcalloc(me->pool, sizeof(*t));
>> +        if (NULL == t) {
>> +            return NULL;
>> +        }
> 
> No need to check for NULL from apr_p*alloc again.
> 
> 

So you are suggesting just let the app segfault when out of memory?

Cheers,
Henry

Mime
View raw message