ocflib icon indicating copy to clipboard operation
ocflib copied to clipboard

Improve database connection API to actually close opened connections

Open jvperrin opened this issue 7 years ago • 3 comments

Taken from @chriskuehl's comment on #103:

This is fine, but eventually we should take the opportunity to fix this API. Currently it's really hard to write correct code which closes connections as you'd expect; a better API would be something like this:

@contextlib.contextmanager
def get_connection(...):
    with contextlib.closing(pymysql.connect(...)) as conn:
        yield conn

Then usage is like this:

with get_connection() as conn:
    with conn as cursor:
        cursor.execute('...')

Currently we never call close on the connections we open.

jvperrin avatar Nov 26 '17 11:11 jvperrin

This appears to have been officially deprecated now in pymysql (https://github.com/PyMySQL/PyMySQL/commit/1ef6c587337bd6ff3272c2c4771948676fd2a9e6) due to https://github.com/PyMySQL/PyMySQL/issues/735, so we should get to this fix in the next year, preferably sooner.

jvperrin avatar Jan 21 '19 11:01 jvperrin

I'm confused on this. Was the with syntax deprecated? If so, if we never implemented it, what do we need to do?

dkess avatar Jan 21 '19 19:01 dkess

We're getting deprecation notices about this in test run outputs of ocflib now:

 .tox/py35/lib/python3.5/site-packages/pymysql/connections.py:497
[repeats many more times]
 .tox/py35/lib/python3.5/site-packages/pymysql/connections.py:497
   /opt/jenkins/slave/workspace/ocflib_PR-201/.tox/py35/lib/python3.5/site-packages/pymysql/connections.py:497: DeprecationWarning: Context manager API of Connection object is deprecated; Use conn.begin()

As for the most recent question on this, I realize I didn't actually respond to it. I think https://stackoverflow.com/a/31215864 does a much better job answering the core issue, so I'll link to that one. While we didn't implement our own version of it or anything, we do use connection objects with with in quite a few places: https://sourcegraph.ocf.berkeley.edu/search?q=with+.*+conn (this probably has a few false positives, but close enough)

jvperrin avatar Jan 30 '20 09:01 jvperrin