Mailing-List: contact modules-dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: modules-dev@httpd.apache.org
Received-SPF: pass (athena.apache.org: domain of info@bnoordhuis.nl designates
 209.85.160.45 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <8B2FC4EBE135700A0FE1337D@Ximines.local>
References: <36D882E966A33CA59AFDA4DB@Ximines.local>
	<CAHQurc-1Qu3Nz=1GDYQV791awdTYOat7+=9VQRrU9_TkuuOqOw@mail.gmail.com>
	<8B2FC4EBE135700A0FE1337D@Ximines.local>
Date: Mon, 10 Sep 2012 14:46:15 +0200
Message-ID: 
 <CAHQurc8exixSnRc+Or3-ZG7jjJGEY0=gEskYOh3Gh6qt53fi+Q@mail.gmail.com>
Subject: Re: Bucket brigade & filter thread safety
From: Ben Noordhuis <info@bnoordhuis.nl>
To: modules-dev@httpd.apache.org, Alex Bligh <alex@alex.org.uk>
Content-Type: text/plain; charset=ISO-8859-1

On Mon, Sep 10, 2012 at 10:47 AM, Alex Bligh <alex@alex.org.uk> wrote:
> Ben,
>
> Thanks for your reply.
>
>
>>> I am suffering from very occasional corruption of the bucket brigade
>>> which normally shows up as a corrupted pool pointer or a bogus bucket
>>> entry in thread #1 (for instance a SEGV in apr_brigade_length).
>>> Interestingly this is the relatively quiet input brigade which is only
>>> ever touched by the main apache thread. It's almost as if an allocator
>>> is not thread safe.
>>
>>
>> That's because it isn't unless you explicitly make it so (which no MPM
>> does).
>
>
> What I'm trying to do is to use a separate allocator per thread.
>
>
>>> However, I'm using a separate bucket allocator (see code above) and (at
>>> least in my code) a separate pool.
>>
>>
>> They're not really separate, the sub pool is created off r->pool. You
>> should probably use apr_pool_create_ex() here with parent=NULL and an
>> allocator that you created with apr_allocator_create() +
>> apr_allocator_mutex_set().
>
>
> OK I based this on the docs for apr_pool_create which says at
> http://apr.apache.org/docs/apr/1.4/group__apr__pools.html#ga918adf3026c894efeae254a0446aed3b
>
> : This function is thread-safe, in the sense that multiple threads can
> : safely create subpools of the same parent pool concurrently. Similarly, a
> : subpool can be created by one thread at the same time that another thread
> : accesses the parent pool.
>
> Have I understood that wrong?

No, but the documentation omits some crucial details.
apr_pool_create() is thread-safe only if:

1. libapr is compiled with APR_HAS_THREADS
2. APR_POOL_DEBUG is turned off
3. the parent pool has a thread-safe allocator (which is true for the
global allocator that is used when parent=NULL, provided conditions #1
and #2 are satisfied)

The pools you get from httpd core satisfy #3 but a module may replace
e.g. r->pool with another pool that doesn't. Ergo, don't rely on a
pool being thread safe unless you explicitly make it so.

>> That won't solve all your problems though. Bucket brigades are not
>> thread safe, you will need something to synchronize on.
>
>
> So what I was trying to do was to use
> a) the input bucket brigade in thread #1 (main thread)
> b) the output bucket brigade in thread #2
> in an attempt to avoid synchronization
>
> But what I don't understand is whether thread #2, in writing
> to the output filters (which presumably have a reference
> to r->pool) will need synchronisation.

Yes. It's not just because r->pool may or may not be synchronized, the
internal structure of the bucket brigade is not protected by any locks
either.

> And if I have to synchronize, how do I do that in practice?
> Thread #2 does and ap_fwrite/ap_flush so I can hold a mutex
> there. But what do I do in thread #1, which calls ap_brigade_get
> and blocks? I can't hold a mutex during that. I can make it
> a non-blocking ap_brigade_get (if I understood how to do it)

Non-blocking reads are pretty straightforward:

  apr_thread_mutex_lock(&mutex);
  rv = ap_get_brigade(f->next, bb, AP_MODE_READBYTES, APR_NONBLOCK_READ, len);
  if (APR_STATUS_IS_EAGAIN(rv)) apr_thread_cond_wait(&cond, &mutex);
  rv = ap_get_brigade(f->next, bb, AP_MODE_READBYTES, APR_NONBLOCK_READ, len);
  apr_thread_mutex_unlock(&mutex);

The other thread wakes up this thread with apr_thread_cond_signal(&cond).

> but what I really need is the equivalent of a select() which
> I can do with the mutex not held (or some way to drop the mutex
> during the raw reads). Any ideas?

You could set up a pollset in the main thread and funnel incoming data
into your bucket brigade. Not terribly efficient (lots of context
switches) but the real world impact may very well be negligible and
you can support multi-process setups with zero changes to your code.