Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (asf.osuosl.org: domain of slowhog@gmail.com designates
 64.233.162.196 as permitted sender)
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
        s=beta; d=gmail.com;
        h=received:message-id:date:from:user-agent:mime-version:to:cc:subject:references:in-reply-to:content-type:content-transfer-encoding:sender;
        b=hvxyPNQiXN5JWgM6S1YWOy1+5gLwGFNynS1vcVCwGf+KgBsfDan+oya9JaNHyElPGUCsWllgDsMkyEAxXC4mZ/ke1spAia2FqKT1+ZBchs9nUde7bRBCiqBk/2pS4sK7wnru7SlfjMBPJPepvM0vx+FdJ1Sf5J7eGbT59AJKXaM=
Message-ID: <447260C4.2030702@ztune.net>
Date: Mon, 22 May 2006 18:09:24 -0700
From: Henry Jen <henryjen@ztune.net>
User-Agent: Mail/News 1.5 (X11/20060315)
MIME-Version: 1.0
To: Davi Arnaut <davi@haxent.com.br>
CC: dev@apr.apache.org
Subject: Re: [Fwd: Re: Ack: CLA for Henry Jen [was Re: thread pool status]]
References: <4472363C.6030807@ztune.net>
 <20060522212252.17d15cf0.davi@haxent.com.br>
In-Reply-To: <20060522212252.17d15cf0.davi@haxent.com.br>
Content-Type: text/plain; charset=US-ASCII; format=flowed
Content-Transfer-Encoding: 7bit
Sender: Henry Jen <slowhog@gmail.com>

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