From 3c124a0b87a7bde51519cd0eb98980f04eed1595 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Tue, 14 Mar 2017 12:06:46 +0000 Subject: [PATCH 1/6] Flake8 complaints --- tests/test_cursor.py | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/tests/test_cursor.py b/tests/test_cursor.py index 42e12c49..7f3bf21c 100755 --- a/tests/test_cursor.py +++ b/tests/test_cursor.py @@ -518,29 +518,25 @@ class CursorTests(ConnectingTestCase): RETURNS INT AS 'SELECT $1 * $1' LANGUAGE SQL - ''' % (procname, escaped_paramname)); + ''' % (procname, escaped_paramname)) # Make sure callproc works right - cur.callproc(procname, { paramname: 2 }) + cur.callproc(procname, {paramname: 2}) self.assertEquals(cur.fetchone()[0], 4) # Make sure callproc fails right failing_cases = [ - ({ paramname: 2, 'foo': 'bar' }, psycopg2.ProgrammingError), - ({ paramname: '2' }, psycopg2.ProgrammingError), - ({ paramname: 'two' }, psycopg2.ProgrammingError), - ({ u'bj\xc3rn': 2 }, psycopg2.ProgrammingError), - ({ 3: 2 }, TypeError), - ({ self: 2 }, TypeError), + ({paramname: 2, 'foo': 'bar'}, psycopg2.ProgrammingError), + ({paramname: '2'}, psycopg2.ProgrammingError), + ({paramname: 'two'}, psycopg2.ProgrammingError), + ({u'bj\xc3rn': 2}, psycopg2.ProgrammingError), + ({3: 2}, TypeError), + ({self: 2}, TypeError), ] for parameter_sequence, exception in failing_cases: self.assertRaises(exception, cur.callproc, procname, parameter_sequence) self.conn.rollback() - def test_callproc_badparam(self): - cur = self.conn.cursor() - self.assertRaises(TypeError, cur.callproc, 'lower', 42) - def test_suite(): return unittest.TestLoader().loadTestsFromName(__name__) From b203a7c775c5f81f59c6b4205c3c33455ceeec3d Mon Sep 17 00:00:00 2001 From: Greg Ward Date: Tue, 28 Jun 2016 17:46:21 -0400 Subject: [PATCH 2/6] Always detect when a connection is closed behind psycopg2's back. There's a race condition that only seems to happen over Unix-domain sockets. Sometimes, the closed socket is reported by the kernel to libpq like this (captured with strace): sendto(3, "Q\0\0\0\34select pg_backend_pid()\0", 29, MSG_NOSIGNAL, NULL, 0) = 29 recvfrom(3, "E\0\0\0mSFATAL\0C57P01\0Mterminating "..., 16384, 0, NULL, NULL) = 110 recvfrom(3, 0x12d0330, 16384, 0, 0, 0) = -1 ECONNRESET (Connection reset by peer) That is, psycopg2/libpq sees no error when sending the first query after the connection is closed, but gets an error reading the result. In that case, everything worked fine. But sometimes, the error manifests like this: sendto(3, "Q\0\0\0\34select pg_backend_pid()\0", 29, MSG_NOSIGNAL, NULL, 0) = -1 EPIPE (Broken pipe) recvfrom(3, "E\0\0\0mSFATAL\0C57P01\0Mterminating "..., 16384, 0, NULL, NULL) = 110 recvfrom(3, "", 16274, 0, NULL, NULL) = 0 recvfrom(3, "", 16274, 0, NULL, NULL) = 0 i.e. libpq received an error when sending the query. This manifests as a slightly different exception from a slightly different place. More importantly, in this case connection.closed is left at 0 rather than being set to 2, and that is the bug I'm fixing here. Note that we see almost identical behaviour for sync and async connections, and the fixes are the same. So I added extremely similar test cases. Finally, there is still a bug here: for async connections, we sometimes raise DatabaseError (incorrect) and sometimes raise OperationalError (correct). Will fix that next. --- psycopg/pqpath.c | 6 ++++++ tests/test_cursor.py | 45 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/psycopg/pqpath.c b/psycopg/pqpath.c index 59cbb5e3..222696be 100644 --- a/psycopg/pqpath.c +++ b/psycopg/pqpath.c @@ -986,6 +986,9 @@ pq_execute(cursorObject *curs, const char *query, int async, int no_result, int /* don't let pgres = NULL go to pq_fetch() */ if (curs->pgres == NULL) { + if (CONNECTION_BAD == PQstatus(curs->conn->pgconn)) { + curs->conn->closed = 2; + } pthread_mutex_unlock(&(curs->conn->lock)); Py_BLOCK_THREADS; if (!PyErr_Occurred()) { @@ -1013,6 +1016,9 @@ pq_execute(cursorObject *curs, const char *query, int async, int no_result, int CLEARPGRES(curs->pgres); if (PQsendQuery(curs->conn->pgconn, query) == 0) { + if (CONNECTION_BAD == PQstatus(curs->conn->pgconn)) { + curs->conn->closed = 2; + } pthread_mutex_unlock(&(curs->conn->lock)); Py_BLOCK_THREADS; PyErr_SetString(OperationalError, diff --git a/tests/test_cursor.py b/tests/test_cursor.py index 7f3bf21c..f9fc66ca 100755 --- a/tests/test_cursor.py +++ b/tests/test_cursor.py @@ -29,6 +29,8 @@ import psycopg2.extensions from testutils import (unittest, ConnectingTestCase, skip_before_postgres, skip_if_no_namedtuple, skip_if_no_getrefcount, slow) +import psycopg2.extras + class CursorTests(ConnectingTestCase): @@ -537,6 +539,49 @@ class CursorTests(ConnectingTestCase): self.assertRaises(exception, cur.callproc, procname, parameter_sequence) self.conn.rollback() + def test_external_close_sync(self): + # If a "victim" connection is closed by a "control" connection + # behind psycopg2's back, psycopg2 always handles it correctly: + # raise OperationalError, set conn.closed to 2. This reproduces + # issue #443, a race between control_conn closing victim_conn and + # psycopg2 noticing. + control_conn = self.conn + connect_func = self.connect + wait_func = lambda conn: None + self._test_external_close(control_conn, connect_func, wait_func) + + def test_external_close_async(self): + # Issue #443 is in the async code too. Since the fix is duplicated, + # so is the test. + control_conn = self.conn + connect_func = lambda: self.connect(async=True) + wait_func = psycopg2.extras.wait_select + self._test_external_close(control_conn, connect_func, wait_func) + + def _test_external_close(self, control_conn, connect_func, wait_func): + # The short sleep before using victim_conn the second time makes it + # much more likely to lose the race and see the bug. Repeating the + # test several times makes it even more likely. + for i in range(10): + victim_conn = connect_func() + wait_func(victim_conn) + + with victim_conn.cursor() as cur: + cur.execute('select pg_backend_pid()') + wait_func(victim_conn) + pid1 = cur.fetchall()[0][0] + + with control_conn.cursor() as cur: + cur.execute('select pg_terminate_backend(%s)', (pid1,)) + + time.sleep(0.001) + with self.assertRaises(psycopg2.DatabaseError): + with victim_conn.cursor() as cur: + cur.execute('select 1') + wait_func(victim_conn) + + self.assertEqual(victim_conn.closed, 2) + def test_suite(): return unittest.TestLoader().loadTestsFromName(__name__) From 12317557da20d84baf12054d8dfce11ba8976cb6 Mon Sep 17 00:00:00 2001 From: Greg Ward Date: Tue, 28 Jun 2016 18:03:05 -0400 Subject: [PATCH 3/6] Always raise OperationalError when connection was closed externally. From the DB-API (https://www.python.org/dev/peps/pep-0249/): OperationalError Exception raised for errors that are related to the database's operation and not necessarily under the control of the programmer, e.g. an unexpected disconnect occurs, [...] Additionally, psycopg2 was inconsistent, at least in the async case: depending on how the "connection closed" error was reported from the kernel to libpq, it would sometimes raise OperationalError and sometimes DatabaseError. Now it always raises OperationalError. --- psycopg/pqpath.c | 10 ++++++---- tests/test_cursor.py | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/psycopg/pqpath.c b/psycopg/pqpath.c index 222696be..f270ce8f 100644 --- a/psycopg/pqpath.c +++ b/psycopg/pqpath.c @@ -179,8 +179,10 @@ pq_raise(connectionObject *conn, cursorObject *curs, PGresult **pgres) /* if the connection has somehow been broken, we mark the connection object as closed but requiring cleanup */ - if (conn->pgconn != NULL && PQstatus(conn->pgconn) == CONNECTION_BAD) + if (conn->pgconn != NULL && PQstatus(conn->pgconn) == CONNECTION_BAD) { conn->closed = 2; + exc = OperationalError; + } if (pgres == NULL && curs != NULL) pgres = &curs->pgres; @@ -214,9 +216,9 @@ pq_raise(connectionObject *conn, cursorObject *curs, PGresult **pgres) if (code != NULL) { exc = exception_from_sqlstate(code); } - else { - /* Fallback if there is no exception code (reported happening e.g. - * when the connection is closed). */ + else if (exc == NULL) { + /* Fallback if there is no exception code (unless we already + determined that the connection was closed). */ exc = DatabaseError; } diff --git a/tests/test_cursor.py b/tests/test_cursor.py index f9fc66ca..98891ea7 100755 --- a/tests/test_cursor.py +++ b/tests/test_cursor.py @@ -575,7 +575,7 @@ class CursorTests(ConnectingTestCase): cur.execute('select pg_terminate_backend(%s)', (pid1,)) time.sleep(0.001) - with self.assertRaises(psycopg2.DatabaseError): + with self.assertRaises(psycopg2.OperationalError): with victim_conn.cursor() as cur: cur.execute('select 1') wait_func(victim_conn) From 7c2333dd81c5c4ee6abd6f0fad54e02ce8d83357 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Mon, 4 Jul 2016 20:49:21 +0100 Subject: [PATCH 4/6] Connection state fixed noted in the news --- NEWS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS b/NEWS index 88689f67..f745b9cc 100644 --- a/NEWS +++ b/NEWS @@ -82,6 +82,8 @@ What's new in psycopg 2.6.3 What's new in psycopg 2.6.2 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Fixed inconsistent state in externally closed connections + (:tickets:`#263, #311, #443`). - Report the server response status on errors (such as :ticket:`#281`). - Raise `!NotSupportedError` on unhandled server response status (:ticket:`#352`). From 8e28444897de74b9137f00c60a41f676058cc751 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Mon, 4 Jul 2016 21:49:45 +0100 Subject: [PATCH 5/6] Bunch of test tweaks to make the test grid green --- tests/test_connection.py | 12 ++---------- tests/test_cursor.py | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/tests/test_connection.py b/tests/test_connection.py index dae0560f..385d979c 100755 --- a/tests/test_connection.py +++ b/tests/test_connection.py @@ -72,16 +72,8 @@ class ConnectionTests(ConnectingTestCase): # ticket #148 conn = self.conn cur = conn.cursor() - try: - cur.execute("select pg_terminate_backend(pg_backend_pid())") - except psycopg2.OperationalError, e: - if e.pgcode != psycopg2.errorcodes.ADMIN_SHUTDOWN: - raise - except psycopg2.DatabaseError, e: - # curiously when disconnected in green mode we get a DatabaseError - # without pgcode. - if e.pgcode is not None: - raise + self.assertRaises(psycopg2.OperationalError, + cur.execute, "select pg_terminate_backend(pg_backend_pid())") self.assertEqual(conn.closed, 2) conn.close() diff --git a/tests/test_cursor.py b/tests/test_cursor.py index 98891ea7..10b8d71e 100755 --- a/tests/test_cursor.py +++ b/tests/test_cursor.py @@ -27,7 +27,8 @@ import pickle import psycopg2 import psycopg2.extensions from testutils import (unittest, ConnectingTestCase, skip_before_postgres, - skip_if_no_namedtuple, skip_if_no_getrefcount, slow) + skip_if_no_namedtuple, skip_if_no_getrefcount, slow, skip_if_no_superuser, + skip_if_windows) import psycopg2.extras @@ -539,6 +540,9 @@ class CursorTests(ConnectingTestCase): self.assertRaises(exception, cur.callproc, procname, parameter_sequence) self.conn.rollback() + @skip_if_no_superuser + @skip_if_windows + @skip_before_postgres(8, 4) def test_external_close_sync(self): # If a "victim" connection is closed by a "control" connection # behind psycopg2's back, psycopg2 always handles it correctly: @@ -550,6 +554,9 @@ class CursorTests(ConnectingTestCase): wait_func = lambda conn: None self._test_external_close(control_conn, connect_func, wait_func) + @skip_if_no_superuser + @skip_if_windows + @skip_before_postgres(8, 4) def test_external_close_async(self): # Issue #443 is in the async code too. Since the fix is duplicated, # so is the test. @@ -575,11 +582,14 @@ class CursorTests(ConnectingTestCase): cur.execute('select pg_terminate_backend(%s)', (pid1,)) time.sleep(0.001) - with self.assertRaises(psycopg2.OperationalError): + + def f(): with victim_conn.cursor() as cur: cur.execute('select 1') wait_func(victim_conn) + self.assertRaises(psycopg2.OperationalError, f) + self.assertEqual(victim_conn.closed, 2) From 3626e961f852e1103ca72650f16f7fd427d48987 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Tue, 14 Mar 2017 12:26:07 +0000 Subject: [PATCH 6/6] Reported bug #443 fixed *again* Also see ticket #527. --- NEWS | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index f745b9cc..e28202c6 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,14 @@ Current release --------------- +What's new in psycopg 2.7.2 +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +- Fixed inconsistent state in externally closed connections + (:tickets:`#263, #311, #443`). Was fixed in 2.6.2 but not included in + 2.7 by mistake. + + What's new in psycopg 2.7.1 ^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -93,7 +101,8 @@ What's new in psycopg 2.6.2 (:ticket:`#333`). - Fixed `!PersistentConnectionPool` on Python 3 (:ticket:`#348`). - Fixed segfault on `repr()` of an unitialized connection (:ticket:`#361`). -- Allow adapting bytes using QuotedString on Python 3 too (:ticket:`#365`). +- Allow adapting bytes using `~psycopg2.extensions.QuotedString` on Python 3 + (:ticket:`#365`). - Added support for setuptools/wheel (:ticket:`#370`). - Fix build on Windows with Python 3.5, VS 2015 (:ticket:`#380`). - Fixed `!errorcodes.lookup` initialization thread-safety (:ticket:`#382`).