apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Henry Jen <henry...@ztune.net>
Subject Re: [Fwd: Re: Ack: CLA for Henry Jen [was Re: thread pool status]]
Date Tue, 23 May 2006 01:09:24 GMT
Davi Arnaut wrote:
> On Mon, 22 May 2006 15:07:56 -0700
> Henry Jen <henryjen@ztune.net> wrote:
> 
>> William A. Rowe, Jr. wrote:
>>> +notinavail:Henry Jen:Henry Jen:henryjen@ztune.net:Signed CLA
>>>
>>> Is received - for the benefit of anyone with cycles to commit his thread
>>> pool design or any other code contributions.
>>>
>> Attached is the latest patch for the thread pool implementation, your 
>> help on review/commit it is greatly appreciated.
>>
>> Cheers,
>> Henry
>>
>>
> 
> Glimpse:
> 
> +static apr_status_t thread_pool_cleanup(void *me)
> +{
> +    apr_thread_pool_t *_self = me;
> +
> +    _self->terminated = 1;
> +    apr_thread_pool_idle_max_set(_self, 0);
> +    while (_self->thd_cnt) {
> +        apr_sleep(20 * 1000);   /* spin lock with 20 ms */
> +    }
> 
> What happens to the busy threads ?

Those will stop after done current task at hand with the terminated 
flag. Therefore a spin lock. :-)

> 
> +
> +    *me = apr_pcalloc(pool, sizeof(**me));
> +    if (!*me) {
> +        return APR_ENOMEM;
> +    }
> 
> Be concise, either check all apr_pcalloc failures or none.
> 

I intent to check, I spot one place I did not in thread_pool_func, 
anywhere else you noticed?

It was brought up earlier that apache has a convention not to check, but 
some application needs to be able to stop gracefully even with error 
condition happens. So, is checking acceptable to apr?


> +
> +    while (init_threads) {
> +        ++(*me)->thd_cnt;
> +        rv = apr_thread_create(&t, NULL, thread_pool_func, *me, (*me)->pool);
> +        if (APR_SUCCESS == rv) {
> +            --init_threads;
> +        }
> +    }
> 
> What if apr_thread_create fails ? thd_cnt will hold a wrong value. Also, a small
> apr_sleep() here wouldn't hurt.
> 
> +    if (0 > next) {
> 
> Excuse me, but this is just too much :)
>

Okay, okay. I will change it to 'if (next < 0)'. :-)


> +        ++me->thd_cnt;
> +        apr_thread_create(&thd, NULL, thread_pool_func, me, me->pool);
> +    }
> 
> apr_thread_create may fail..
>

Thanks, I will fix it.

Cheers,
Henry

Mime
View raw message