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

[Jean-Paul Calderone]

On Tue, 17 Mar 2009 19:57:09 +0100, "M.-A. Lemburg" <mal at egenix.com> wrote:
>On 2009-03-17 15:59, Jean-Paul Calderone wrote:
> [snip]
>
>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.
>"""

Oh, boy.  Yes, that could be it.  I assumed it was more like setting a key
in a Python dictionary.

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

So only the first MY_BEGIN_ALLOW_THREADS per thread does anything.  That
could break things, I guess. :)

>
>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.

So the macros should do error checking as well (assuming it makes sense
to continue to use TLS 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.

Can you give some examples?  The only thing I'm aware of is that
performance may differ wildly from platform to platform, but I thought
it was pretty good on all mainstream platforms these days.

>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, ...).

That sounds essentially like the idea I mentioned in my earlier reply
(the one I compared to SQLite).  I see how all of that logic could be
wrapped up in the MY_BEGIN/END_ALLOW_THREADS macro now, so I'm even
more inclined to go that route now.

Tests or scripts that produce failures would still be very helpful.
Clearly the test suite doesn't exercise the buggy codepaths currently,
and it seems neither does thread-crash.py (probably because it chokes
so quickly due to its misuse of Connections in multiple threads).

If I can reproduce the problem others are seeing, I'll be able to tell
if a particular change actually fixes it.  Otherwise I'm just guessing. :)

Jean-Paul