Re: [pyOpenSSL] Python core dumps when using pyOpenSSL 0.8.0 and threads

[M.-A. L. <ma...@eg...>]

On 2009-03-17 15:59, Jean-Paul Calderone wrote:
> On Tue, 17 Mar 2009 12:36:48 +0100, "M.-A. Lemburg" <ma...@eg...> wrote:
>> I think you should not try to unroll the
>>
>> Py_BEGIN_ALLOW_THREADS
>> ...Do some blocking I/O operation...
>> Py_END_ALLOW_THREADS
>>
>> blocks and just use those macros in the code.
> 
> pyOpenSSL has always done this.  It's necessary if Python-defined
> callbacks are going to be supported, because the GIL must be re-acquired
> in the callback.  This means thread state structured needs to be available
> in the callback, which in turn means Py_BEGIN_ALLOW_THREADS isn't usable.
> 
> It's possible that PyGILState_Ensure and PyGILState_Release would be
> useful here, but I'm not sure.  I need to investigate further.

Ah, callbacks... evil stuff :-)

You're right. Callbacks don't work with the above approach.

>> Storing the thread state in some application data object is not a
>> good approach, since there's nothing preventing some other thread
>>from restoring the thread state saved by the thread that just
>> released the lock.
>>
>> By contrast, the above macros will add a local stack variable and
>> store the thread state there, so it's not possible for another
>> thread to accidentally restore a thread state that it doesn't
>> own.
> 
> Right.  That's why I changed pyOpenSSL /from/ doing that (prior to 0.8)
> to using thread local storage instead (0.8).  However, it seems there's
> some bug in the thread local storage version of the locking scheme.

Perhaps it's related to this notice of caution in thread.c:

"""
Use PyThread_set_key_value(thekey, value) to associate void* value with
thekey in the current thread.  Each thread has a distinct mapping of thekey
to a void* value.  Caution:  if the current thread already has a mapping
for thekey, value is ignored.
"""

If value is ignored, MY_BEGIN_ALLOW_THREADS() won't save the current
thread state, but leave the old one in place.

It's also possible for this API to fail in case of a memory issue,
but the macro doesn't check for this, so the application may end
up not storing anything at all.

MY_END_ALLOW_THREADS() needs to remove the stored TLS value in order
to make room for the next MY_BEGIN_ALLOW_THREADS() call.

Note that I'm not sure using TLS is a good idea: the implementations
are highly platform dependent and may have undocumented/unwanted
side-effects.

IMHO, it's safer to store the thread id together with the tstate
itself in the context and then check whether the thread id matches
before restoring the state.

This has the additional benefit of making some form of error
raising possible in order to inform the programmer of the obvious
mistake in using the connections from multiple threads,
even if it's just a plain fprintf(stderr, ...).

-- 
Marc-Andre Lemburg
eGenix.com

Professional Python Services directly from the Source  (#1, Mar 17 2009)
>>> Python/Zope Consulting and Support ...        http://www.egenix.com/
>>> mxODBC.Zope.Database.Adapter ...             http://zope.egenix.com/
>>> mxODBC, mxDateTime, mxTextTools ...        http://python.egenix.com/
________________________________________________________________________

::: Try our new mxODBC.Connect Python Database Interface for free ! ::::


   eGenix.com Software, Skills and Services GmbH  Pastor-Loeh-Str.48
    D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg
           Registered at Amtsgericht Duesseldorf: HRB 46611
               http://www.egenix.com/company/contact/