Re: Lock-ups with multi-threading - draft patch

On Sun, Sep 27, 2009 at 1:34 AM, Richard Davies
<[email protected]> wrote:
>> OK, I've managed to generate the simple test case, which is a big step
>> forward!
>
> And here's a proposed patch, which works for me.
>
>
> I've duplicated the deadlock inside gdb with debug symbols and I attach a
> backtrace for my test case in the deadlocked state (attached, running with
> Python 2.6.2, Psycopg2 2.0.12 and PostgreSQL 8.1.4).
>
> The cause of the deadlock is now clear, with both threads at various stages
> near the end of pq_execute():
>
> Thread 2 is inside conn_notice_process(), trying to claim the
> curs->conn->lock at line 67. I believe that this thread holds the Python GIL
> (global interpreter lock) so will prevent other threads from running.
>
> Thread 3 is inside pq_fetch() at line 777. It is holding the
> curs->conn->lock since line 731. It is running Py_BLOCK_THREADS; and hence
> is waiting for the other thread release the GIL.
>
>
> The bug is that conn_notice_process() in Thread 2 needs to release the GIL
> so that Thread 3 can complete and release curs->conn->lock before Thread 2
> claims it.
>
>
> I notice that most Psycopg calls to pthread_mutex_lock/unlock() are wrapped
> inside Py_BEGIN/END_ALLOW_THREADS but that conn_notice_process() does not do
> this (probably an oversight?).
>
> I attach a patch which to use Py_BEGIN/END_ALLOW_THREADS inside
> conn_notice_process() in the same way at the reset. I also found two other
> functions which also look like they're missing this and included these in
> the patch.
>
> This needs double-checking, in particular for whether Py_BLOCK_THREADS and
> Py_UNBLOCK_THREADS should be used inside any of these sections to protect
> any non-thread-safe code.
>
> However, it works for me!

Thank you for tracking this down and providing a patch.  I think you
are right about this being an issue with the dependency between
Python's GIL and psycopg2's connection locks, but the patch itself has
problems.

Namely, calling any Python API functions without holding the GIL is an
error and can corrupt the interpreter's internal state.  A simple fix
would be to reaquire the GIL after acquiring the connection lock (I
think this should be okay).  Alternatively, the conn_notice_process()
function could be changed to just retrieve the notice_pending list and
set it to NULL while holding the connection lock, and then process
those notices after reacquiring the GIL.

James.