Re: Lock-ups with multi-threading - draft 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.
..
> 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).

OK, I'm attaching a second patch, in which I take this approach, using
Py_BLOCK_THREADS. I think this is now good for conn_notice_process() and
conn_notice_clean() in connection_int.c


I'm less sure about the change to psyco_conn_reset() in connection_type.c -
there is a lot of code inside the sub-call to conn_setup(), including nested
uses of Py_BEGIN_ALLOW_THREADS (which I think is prohibited in Python?).

What do you think about moving the locking of the connection from
psyco_conn_reset() to inside conn_setup(), where it could be done inside on
of the existing Py_BEGIN_ALLOW_THREADS blocks?

Cheers,

Richard.

==================================
diff -uNr psycopg2-2.0.12/psycopg/connection_int.c psycopg2-2.0.12.patch/psycopg/connection_int.c
--- psycopg2-2.0.12/psycopg/connection_int.c	2009-08-09 15:09:46.000000000 +0100
+++ psycopg2-2.0.12.patch/psycopg/connection_int.c	2009-09-28 17:37:41.000000000 +0100
@@ -64,11 +64,13 @@
 void
 conn_notice_process(connectionObject *self)
 {
+    Py_BEGIN_ALLOW_THREADS;
     pthread_mutex_lock(&self->lock);
 
     struct connectionObject_notice *notice =  self->notice_pending;
     
     while (notice != NULL) {
+        Py_BLOCK_THREADS;
         PyObject *msg = PyString_FromString(notice->message);
 
         Dprintf("conn_notice_process: %s", notice->message);
@@ -80,10 +82,12 @@
         if (PyList_GET_SIZE(self->notice_list) > CONN_NOTICES_LIMIT)
             PySequence_DelItem(self->notice_list, 0);
         
+        Py_UNBLOCK_THREADS;
         notice = notice->next;
     }
     
     pthread_mutex_unlock(&self->lock);
+    Py_END_ALLOW_THREADS;
     
     conn_notice_clean(self);  
 }
@@ -91,6 +95,7 @@
 void
 conn_notice_clean(connectionObject *self)
 {
+    Py_BEGIN_ALLOW_THREADS;
     pthread_mutex_lock(&self->lock);
     
     struct connectionObject_notice *tmp, *notice = self->notice_pending;
@@ -105,6 +110,7 @@
     self->notice_pending = NULL;
     
     pthread_mutex_unlock(&self->lock);    
+    Py_END_ALLOW_THREADS;
 }
 
 /* conn_setup - setup and read basic information about the connection */
diff -uNr psycopg2-2.0.12/psycopg/connection_type.c psycopg2-2.0.12.patch/psycopg/connection_type.c
--- psycopg2-2.0.12/psycopg/connection_type.c	2009-08-09 15:08:59.000000000 +0100
+++ psycopg2-2.0.12.patch/psycopg/connection_type.c	2009-09-28 17:32:22.000000000 +0100
@@ -366,9 +366,16 @@
     if (pq_reset(self) < 0)
         return NULL;
 
+    Py_BEGIN_ALLOW_THREADS;
     pthread_mutex_lock(&self->lock);
+    Py_BLOCK_THREADS;
+
     res = conn_setup(self, self->pgconn);
+
+    Py_UNBLOCK_THREADS;
     pthread_mutex_unlock(&self->lock);
+    Py_END_ALLOW_THREADS;
+
     if (res < 0)
         return NULL;