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

Richard Davies <[email protected]> Fri, 2 Oct 2009 13:17:27 +0100

Newsgroups	gmane.comp.python.db.psycopg.devel
Message-ID	<[email protected]>

Hi James and Federico,

I wrote in on Monday with a patch which fixes a multi-threading lockup:
http://lists.initd.org/pipermail/psycopg/2009-September/006631.html

This patch definitely fixes a reproducable deadlock for me. Does it look
good to be committed, or is there anything else that I need to do?

At the time, I wrote:
> 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?

And I've now also produced a second version of the patch (attached), which
does this.

I believe that both this new patch and the version which I sent on Monday
are correct code which works. Today's patch touches more lines, but I think
leave things in a tidier state. In any case, I'd be happy to have either
version incorporated.

Let me know if there's anything more which I can do?

Cheers,

Richard.

_______________________________________________
Psycopg mailing list
Psycopg-IAPFreCvJWPBWskQ1e/[email protected]
http://lists.initd.org/mailman/listinfo/psycopg

patch-2 (text/plain, 4.4 KB)

diff -uNr psycopg2-2.0.12/psycopg/connection_int.c psycopg2-2.0.12.patched/psycopg/connection_int.c
--- psycopg2-2.0.12/psycopg/connection_int.c	2009-08-09 15:09:46.000000000 +0100
+++ psycopg2-2.0.12.patched/psycopg/connection_int.c	2009-10-02 12:48:34.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 */
@@ -112,6 +118,10 @@
 int
 conn_setup(connectionObject *self, PGconn *pgconn)
 {
+    Py_BEGIN_ALLOW_THREADS;
+    pthread_mutex_lock(&self->lock);
+    Py_BLOCK_THREADS;
+
     PGresult *pgres;
     const char *data, *tmp;
     const char *scs;    /* standard-conforming strings */
@@ -160,26 +170,32 @@
     Dprintf("conn_connect: server requires E'' quotes: %s",
         self->equote ? "YES" : "NO");
 
-    Py_BEGIN_ALLOW_THREADS;
+    Py_UNBLOCK_THREADS;
     pgres = PQexec(pgconn, datestyle);
-    Py_END_ALLOW_THREADS;
+    Py_BLOCK_THREADS;
 
     if (pgres == NULL || PQresultStatus(pgres) != PGRES_COMMAND_OK ) {
         PyErr_SetString(OperationalError, "can't set datestyle to ISO");
         PQfinish(pgconn);
         IFCLEARPGRES(pgres);
+        Py_UNBLOCK_THREADS;
+        pthread_mutex_unlock(&self->lock);
+        Py_BLOCK_THREADS;
         return -1;
     }
     CLEARPGRES(pgres);
 
-    Py_BEGIN_ALLOW_THREADS;
+    Py_UNBLOCK_THREADS;
     pgres = PQexec(pgconn, encoding);
-    Py_END_ALLOW_THREADS;
+    Py_BLOCK_THREADS;
 
     if (pgres == NULL || PQresultStatus(pgres) != PGRES_TUPLES_OK) {
         PyErr_SetString(OperationalError, "can't fetch client_encoding");
         PQfinish(pgconn);
         IFCLEARPGRES(pgres);
+        Py_UNBLOCK_THREADS;
+        pthread_mutex_unlock(&self->lock);
+        Py_BLOCK_THREADS;
         return -1;
     }
     tmp = PQgetvalue(pgres, 0, 0);
@@ -188,6 +204,9 @@
         PyErr_NoMemory();
         PQfinish(pgconn);
         IFCLEARPGRES(pgres);
+        Py_UNBLOCK_THREADS;
+        pthread_mutex_unlock(&self->lock);
+        Py_BLOCK_THREADS;
         return -1;
     }
     for (i=0 ; i < strlen(tmp) ; i++)
@@ -195,15 +214,18 @@
     self->encoding[i] = '\0';
     CLEARPGRES(pgres);
 
-    Py_BEGIN_ALLOW_THREADS;
+    Py_UNBLOCK_THREADS;
     pgres = PQexec(pgconn, isolevel);
-    Py_END_ALLOW_THREADS;
+    Py_BLOCK_THREADS;
 
     if (pgres == NULL || PQresultStatus(pgres) != PGRES_TUPLES_OK) {
         PyErr_SetString(OperationalError,
                          "can't fetch default_isolation_level");
         PQfinish(pgconn);
         IFCLEARPGRES(pgres);
+        Py_UNBLOCK_THREADS;
+        pthread_mutex_unlock(&self->lock);
+        Py_BLOCK_THREADS;
         return -1;
     }
     data = PQgetvalue(pgres, 0, 0);
@@ -217,6 +239,10 @@
         self->isolation_level = 2;
     CLEARPGRES(pgres);
 
+    Py_UNBLOCK_THREADS;
+    pthread_mutex_unlock(&self->lock);
+    Py_END_ALLOW_THREADS;
+
     return 0;
 }
 
diff -uNr psycopg2-2.0.12/psycopg/connection_type.c psycopg2-2.0.12.patched/psycopg/connection_type.c
--- psycopg2-2.0.12/psycopg/connection_type.c	2009-08-09 15:08:59.000000000 +0100
+++ psycopg2-2.0.12.patched/psycopg/connection_type.c	2009-10-02 12:44:50.000000000 +0100
@@ -366,9 +366,7 @@
     if (pq_reset(self) < 0)
         return NULL;
 
-    pthread_mutex_lock(&self->lock);
     res = conn_setup(self, self->pgconn);
-    pthread_mutex_unlock(&self->lock);
     if (res < 0)
         return NULL;