Psycopg2: __exit__ implementations for cursor and connection are reversed

[matthewapeters]

When using the "with" context in python 2.7 and above, we noticed that connections are being left open on the database. The following code illustrates this:

>>>> import psycopg2 as psy
>>>> creds={"host":"localhost", "database":"my_db","user":"my_user","password":"not_secure","port":5432}
...  with psy.connect(**creds) as conn:
...   with conn.cursor() as curs:
...     curs.execute("select now();")
...
>>>> curs.execute("select now();")
Traceback (most recent call last):
  File "", line 1, in 
psycopg2.InterfaceError: cursor already closed
>>> curs=con.cursor()
>>> curs.execute("select current_database();")
>>> for i in curs.fetchall():
...   print(i)
...
('my_db',)
>>> con.rolllback()

The error for cursor is anticipated and correct.
The lack of an error on the generation of a new cursor off of a connection that is now out of context is the bug.

The cause of the bug is visible in the comments for psycopg/connection_type.c and psycopg/cursor_type.c (just search of "__exit__()" in the project)

psycopg/connection_type.c 
Showing the top two matches. Last indexed on Aug 24, 2014.

410    #define psyco_conn_exit_doc \
411    "__exit__ -- commit if no exception, else roll back"
412    
413    static PyObject *
…    
963         METH_NOARGS, psyco_conn_enter_doc},
964        {"__exit__", (PyCFunction)psyco_conn_exit,
965         METH_VARARGS, psyco_conn_exit_doc},

psycopg/cursor_type.c 
Showing the top two matches. Last indexed on Sep 15, 2014.

1208    #define psyco_curs_exit_doc \
1209    "__exit__ -- close the cursor"
1210    
1211    static PyObject *
1212    psyco_curs_exit(cursorObject *self, PyObject *args)
…    
1741        {"__enter__", (PyCFunction)psyco_curs_enter,
1742         METH_NOARGS, psyco_curs_enter_doc},
1743        {"__exit__", (PyCFunction)psyco_curs_exit,
1744         METH_VARARGS, psyco_curs_exit_doc},

The correct behavior is inverted. Connections neither commit nor roll back - these are methods of cursors. Cursors should not simply close: they may be mid transaction and need to evaluate if they should commit or roll back.

I understand this code has been this way for many releases. But in that the __exit__() functions actually violate the proper implementations for the objects going out of context, we technically have a bug on our hands.

The following article fails to identify the behavior described above (http://initd.org/psycopg/articles/2013/04/07/psycopg-25-released/) in the section "Connections and cursors as context managers":

(FIXED: the connection is NOT closed as originally stated)

The "FIX" is a correction to the author's assumption that the connection is closed as one rightly assumes.

The only time __exit__() should do anything other than close a connection is when it is returning a connection to a pool. This is why other database interfaces provide classes like PooledConnection to disambiguate the behaviors.

[matthewapeters]

Correction to some of my own statements: Rollback and commit belong to the connection, and not the cursor.
Creating two cursors on the same connection simply generates two interfaces to the same connection and corresponding transaction block.

>>> conn = ps.connect(**creds)
>>> curs1  = conn.cursor()
>>> curs2 = conn.cursor()
>>> curs1.execute("begin;")
>>> curs2.execute("select * from not_a_table;")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
psycopg2.ProgrammingError: relation "not_a_table" does not exist
LINE 1: select * from not_a_table;
                      ^
>>> curs1.execute("select now();")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
psycopg2.InternalError: current transaction is aborted, commands ignored until end of transaction block
>>> conn.rollback()
>>> curs1

>>> curs2

>>> curs1.execute("select now();")
>>> print([i for i in curs1.fetchall()])
[(datetime.datetime(2015, 2, 24, 16, 18, 24, 656431, tzinfo=psycopg2.tz.FixedOffsetTimezone(offset=-480, name=None)),)]
>>> curs2.close()
>>> curs2
<cursor object at 0x101b20af8; closed: -1>
>>> curs1
<cursor object at 0x101b20a00; closed: 0>
>>> conn.close()
>>> curs1
<cursor object at 0x101b20a00; closed: 0>
>>> curs1.execute("select now();")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
psycopg2.InterfaceError: cursor already closed

I don't know why closing the connection does not render the cursor curs1 to closed:

>>> conn.close()
>>> curs1
<cursor object at 0x101b20a00; closed: 0>

but that is another issue.

The real issue is that the connection object is not closed when __exit__() is invoked.

[dvarrazzo]

Connections are not closed on exit, no: this is by design and is not going to change. There are good reasons for that, but the most compelling is that connections left open in transactions are a much worse problem than connections left open and you may enter a transaction context several times with the same connection. Your wrong assumption is that the context we enter and exit is the connection lifetime; no: the context is a transaction and exiting it closes the transaction.

[matthewapeters]

Thanks for your consideration of my issue. I understand the issue is closed, but I know that I represent several users of the library: I posted the issue after several applications written by several users in my organization stumbled across this behavior. I have seen the documentation that says that under no conditions are the connection closed. Engineers more senior than I missed the comment, because the example directly above the documentation does not display the connection.close() statement that ought to end the example.

[dvarrazzo]

Thank you very much for your observations. The issue is closed but as you point out the documentation is maybe a bit too skimpy: I'll think about extending that to make it more obvious (e.g. including an example of using the same connection in multiple contexts representing separate transactions and better specifying that close() doesn't come with the package.

If you want to provide a documentation patch that would be welcome.

[dvarrazzo]

Docs improved at http://initd.org/psycopg/docs/usage.html#with-statement