Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve database connection API to actually close opened connections #108

Open
jvperrin opened this issue Nov 26, 2017 · 3 comments
Open

Comments

@jvperrin
Copy link
Member

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
Copy link
Member Author

This appears to have been officially deprecated now in pymysql (PyMySQL/PyMySQL@1ef6c58) due to PyMySQL/PyMySQL#735, so we should get to this fix in the next year, preferably sooner.

@dkess
Copy link
Member

dkess commented Jan 21, 2019

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

@jvperrin
Copy link
Member Author

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants