Page MenuHomeSoftware Heritage
Differential D1384

Explicitely give Db connections back to the pool.
ClosedPublic
Actions

Authored by vlorentz on Apr 9 2019, 4:56 PM.

Details

Reviewers
olasd
Group Reviewers
Reviewers
Commits
rDCORE1d9c0bf05c32: Explicitely give Db connections back to the pool.
Summary

So they gracefully release the connection on error instead
of relying on reference-counting to call the Db's __del__
(which does not happen in Hypothesis tests) because a ref
to it is kept via the traceback object.

See also: D1381

Diff Detail

Repository
rDCORE Foundations and core functionalities
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vlorentz created this revision.Apr 9 2019, 4:56 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptApr 9 2019, 4:56 PM
swh-public-ci added a comment.Apr 9 2019, 4:58 PM

Build is green
See https://jenkins.softwareheritage.org/job/DCORE/job/tox/201/ for more details.

Harbormaster completed remote builds in B5246: Diff 4438.Apr 9 2019, 4:58 PM
olasd accepted this revision.Apr 9 2019, 5:33 PM
olasd added a subscriber: olasd.
olasd added inline comments.
swh/core/db/common.py
50

Can we guard this with an if hasattr(self, 'put_db') to avoid making other users of this (indexer, archiver) insta-buggy?

83

Same comment

This revision is now accepted and ready to land.Apr 9 2019, 5:33 PM
vlorentz added inline comments.Apr 9 2019, 5:35 PM
swh/core/db/common.py
50

No, because I removed BaseDb.__del__, so it would leak connections in prod.

vlorentz mentioned this in D1385: Add method put_db, required by swh-core >= v0.0.59..Apr 9 2019, 5:39 PM
vlorentz mentioned this in D1386: Add method put_db, required by swh-core >= v0.0.59..
vlorentz mentioned this in D1387: Add method put_db, required by swh-core >= v0.0.59..Apr 9 2019, 5:42 PM
Closed by commit rDCORE1d9c0bf05c32: Explicitely give Db connections back to the pool. (authored by vlorentz). · Explain WhyApr 9 2019, 6:05 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
swh/
core/
db/
3 lines
19 lines

Diff 4478

View Options

swh/core/db/__init__.py

Show First 20 Lines • Show All 107 Lines • ▼ Show 20 Lines def __init__(self, conn, pool=None):
conn: psycopg2 connection to the SWH DB conn: psycopg2 connection to the SWH DB
pool: psycopg2 pool of connections pool: psycopg2 pool of connections
""" """
self.adapt_conn(conn) self.adapt_conn(conn)
self.conn = conn self.conn = conn
self.pool = pool self.pool = pool
def __del__(self): def put_conn(self):
if self.pool: if self.pool:
self.pool.putconn(self.conn) self.pool.putconn(self.conn)
def cursor(self, cur_arg=None): def cursor(self, cur_arg=None):
"""get a cursor: from cur_arg if given, or a fresh one otherwise """get a cursor: from cur_arg if given, or a fresh one otherwise
meant to avoid boilerplate if/then/else in methods that proxy stored meant to avoid boilerplate if/then/else in methods that proxy stored
procedures procedures
▲ Show 20 Lines • Show All 52 Lines • ▼ Show 20 Lines def copy_to(self, items, tblname, columns,
with open(write_file, 'w') as f: with open(write_file, 'w') as f:
for d in items: for d in items:
if item_cb is not None: if item_cb is not None:
item_cb(d) item_cb(d)
line = [escape(d.get(k, default_values.get(k))) line = [escape(d.get(k, default_values.get(k)))
for k in columns] for k in columns]
f.write(','.join(line)) f.write(','.join(line))
f.write('\n') f.write('\n')
finally: finally:
# No problem bubbling up exceptions, but we still need to make sure # No problem bubbling up exceptions, but we still need to make sure
# we finish copying, even though we're probably going to cancel the # we finish copying, even though we're probably going to cancel the
# transaction. # transaction.
write_thread.join() write_thread.join()
def mktemp(self, tblname, cur=None): def mktemp(self, tblname, cur=None):
self.cursor(cur).execute('SELECT swh_mktemp(%s)', (tblname,)) self.cursor(cur).execute('SELECT swh_mktemp(%s)', (tblname,))
View Options

swh/core/db/common.py

Show All 37 Lines def decorator(meth, __client_options=client_options):
if 'cur' in kwargs and kwargs['cur']: if 'cur' in kwargs and kwargs['cur']:
cur = kwargs['cur'] cur = kwargs['cur']
old_options = apply_options(cur, __client_options) old_options = apply_options(cur, __client_options)
ret = meth(self, *args, **kwargs) ret = meth(self, *args, **kwargs)
apply_options(cur, old_options) apply_options(cur, old_options)
return ret return ret
else: else:
db = self.get_db() db = self.get_db()
try:
with db.transaction() as cur: with db.transaction() as cur:
apply_options(cur, __client_options) apply_options(cur, __client_options)
return meth(self, *args, db=db, cur=cur, **kwargs) return meth(self, *args, db=db, cur=cur, **kwargs)
finally:
olasdUnsubmitted
Not Done
Inline Actions

Can we guard this with an if hasattr(self, 'put_db') to avoid making other users of this (indexer, archiver) insta-buggy?

olasd: Can we guard this with an `if hasattr(self, 'put_db')` to avoid making other users of this…
vlorentzAuthorUnsubmitted
Done
Inline Actions

No, because I removed BaseDb.__del__, so it would leak connections in prod.

vlorentz: No, because I removed `BaseDb.__del__`, so it would leak connections in prod.
self.put_db(db)
return _meth return _meth
return decorator return decorator
def db_transaction_generator(**client_options): def db_transaction_generator(**client_options):
"""decorator to execute Backend methods within DB transactions, while """decorator to execute Backend methods within DB transactions, while
returning a generator returning a generator
Show All 11 Lines def decorator(meth, __client_options=client_options):
def _meth(self, *args, **kwargs): def _meth(self, *args, **kwargs):
if 'cur' in kwargs and kwargs['cur']: if 'cur' in kwargs and kwargs['cur']:
cur = kwargs['cur'] cur = kwargs['cur']
old_options = apply_options(cur, __client_options) old_options = apply_options(cur, __client_options)
yield from meth(self, *args, **kwargs) yield from meth(self, *args, **kwargs)
apply_options(cur, old_options) apply_options(cur, old_options)
else: else:
db = self.get_db() db = self.get_db()
try:
with db.transaction() as cur: with db.transaction() as cur:
apply_options(cur, __client_options) apply_options(cur, __client_options)
yield from meth(self, *args, db=db, cur=cur, **kwargs) yield from meth(self, *args, db=db, cur=cur, **kwargs)
finally:
olasdUnsubmitted
Not Done
Inline Actions

Same comment

olasd: Same comment
self.put_db(db)
return _meth return _meth
return decorator return decorator
About · Developer information · Legal