apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: Thread pool prototype
Date Wed, 03 May 2006 09:32:10 GMT
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".

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?

> +#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).

...
> +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).

> +typedef struct _apr_thread_pool_task
> +{

again, don't use leading underscores.

...
> +struct _apr_thread_pool
> +{
> +    apr_pool_t *parent;

that field doesn't appear to be used?

...
> +
> +static apr_status_t apr_thread_pool_construct(apr_thread_pool_t * me,
> +                                              apr_size_t init_threads,
> +                                              apr_size_t max_threads)
> +{
> +    apr_status_t rv;
> +    int i;
> +
> +    me->cnt_max = max_threads;
> +    me->idle_max = init_threads;
> +    rv = apr_thread_mutex_create(&me->lock, APR_THREAD_MUTEX_NESTED,
> +                                 me->pool);
> +    if (APR_SUCCESS != rv) {

Personally I find the "constant != variable" style very distracting, not 
sure if this is a widely held view...

> +
> +/*
> + * 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.

...
> +static void *APR_THREAD_FUNC apr_thread_pool_func(apr_thread_t * t,
> +                                                  void *param)

Again, don't use an apr_ prefix on private functions.

> +{
> +    apr_thread_pool_t *me = (apr_thread_pool_t *) param;

No need to cast from void *.

> +    apr_thread_pool_task_t *task;
> +    struct _apr_thread_list_elt *elt;
> +    apr_status_t rv;
> +
> +    elt = (struct _apr_thread_list_elt *) apr_pcalloc(me->pool, sizeof(*elt));

There's no need to cast the return value of apr_*alloc.

> +    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.

...
> +
> +static apr_status_t apr_thread_pool_cleanup(void *me)

No apr_ prefix for private functions.

> +{
> +    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.

...
> +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.

> +    *me = (apr_thread_pool_t *) apr_pcalloc(pool, sizeof(**me));
> +    if (!*me) {
> +        return APR_ENOMEM;
> +    }

... or OOM checking again.

...
> +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.


Mime
View raw message