[PATCH] Bug in apr locking assumptions and optimization

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

Attached is a patch to introduce a new locking semantic to our thread_mutex.
Right now we presume the 'default' is unnested.  Well, that saves 30% of the
processing time on Unix, but it would cripple Win32 (which gets a critical 
section
in 10 instructions or so IF there is no contention... and it's always 
nested...)

Sebastian tossed me a really interesting and invalid assumption I made
when apr'izing mod_isapi...

At 09:38 AM 5/25/2002, Sebastian Hantsch wrote to me:
>[...] the current Async emulation won't work as designed:
>
>This is a snippet from isapi_handler:
>comp = cid->completed;
>if (cid->completed && (rv == APR_SUCCESS)) {
>        rv = apr_thread_mutex_lock(comp);
>}
>/* The completion port is now locked.  When we regain the
>* lock, we may destroy the request.
>*/
>if (cid->completed && (rv == APR_SUCCESS)) {
>        rv = apr_thread_mutex_lock(comp);
>}
>break;
>
>apr_thread_mutex functions internally use Critical Sections on win32 platform.
>In the Win32 SDK at
>http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/synchro_3xym.asp
>is the following statement:
> > After a thread has ownership of a critical section, it can make 
> additional calls to
> > EnterCriticalSection or TryEnterCriticalSection without blocking its 
> execution. This
> > prevents a thread from deadlocking itself while waiting for a critical 
> section that it already owns.
>
>So, the method currently used in mod_isapi.c does not block as supposed 
>until a HSE_REQ_DONE_WITH_SESSION message arrives.
>I would suggest to use SetEvent/CreateEvent/WaitForSingleObject as in 
>2.0.36, that synchronization methods work for me.

Today we have no unnested locks on Win32.  That's problem one to address.
The attached patch should make this really trivial and unambiguous, while 
leaving
all platforms to choose the obvious semantic for _DEFAULT locks for 
performance.

Comments?

[PATCH] Revised Win32 thread_mutex options

["William A. Rowe, Jr." <wr...@rowe-clan.net>]

The attached patch introduces an unnested Win32 Event-based thread_mutex
that is compatible with the assumption that a single thread may obtain a lock
for another thread, then wait for the second thread to release it.

It further degrades to Mutex objects for nested Win9x locks, since we couldn't
'try' a thread_mutex based on CriticalSection objects before Windows NT.

I also attached a simple patch to illustrate the fix to mod_isapi with the
change to the API.  Hoping Sebastian could take a look as well.

If I don't hear objections, I'll commit later this afternoon.  Comments 
welcome.

Bill

At 10:19 AM 5/29/2002, William A. Rowe, Jr. wrote:
>Attached is a patch to introduce a new locking semantic to our thread_mutex.
>Right now we presume the 'default' is unnested.  Well, that saves 30% of the
>processing time on Unix, but it would cripple Win32 (which gets a critical 
>section
>in 10 instructions or so IF there is no contention... and it's always 
>nested...)
>
>Sebastian tossed me a really interesting and invalid assumption I made
>when apr'izing mod_isapi...
>
>At 09:38 AM 5/25/2002, Sebastian Hantsch wrote to me:
>>[...] the current Async emulation won't work as designed:
>>
>>This is a snippet from isapi_handler:
>>comp = cid->completed;
>>if (cid->completed && (rv == APR_SUCCESS)) {
>>        rv = apr_thread_mutex_lock(comp);
>>}
>>/* The completion port is now locked.  When we regain the
>>* lock, we may destroy the request.
>>*/
>>if (cid->completed && (rv == APR_SUCCESS)) {
>>        rv = apr_thread_mutex_lock(comp);
>>}
>>break;
>>
>>apr_thread_mutex functions internally use Critical Sections on win32 
>>platform.
>>In the Win32 SDK at
>>http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/synchro_3xym.asp
>>is the following statement:
>> > After a thread has ownership of a critical section, it can make 
>> additional calls to
>> > EnterCriticalSection or TryEnterCriticalSection without blocking its 
>> execution. This
>> > prevents a thread from deadlocking itself while waiting for a critical 
>> section that it already owns.
>>
>>So, the method currently used in mod_isapi.c does not block as supposed 
>>until a HSE_REQ_DONE_WITH_SESSION message arrives.
>>I would suggest to use SetEvent/CreateEvent/WaitForSingleObject as in 
>>2.0.36, that synchronization methods work for me.