Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Make sure that we close cursors before returning from a query #6408

Merged
merged 2 commits into from
Nov 25, 2019
Merged

Make sure that we close cursors before returning from a query #6408

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8079cc4
Make sure that we close cursors before returning from a query
richvdh Nov 25, 2019
f29f821
address review comments
richvdh Nov 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/6408.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix an intermittent exception when handling read-receipts.
45 changes: 36 additions & 9 deletions synapse/storage/_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,16 +409,16 @@ def _new_transaction(
i = 0
N = 5
while True:
cursor = conn.cursor()
cursor = LoggingTransaction(
cursor,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we call conn.cursor() in here instead of redefining a variable? this will bite us when mypy gets to it eventually :)

name,
self.database_engine,
after_callbacks,
exception_callbacks,
)
try:
txn = conn.cursor()
txn = LoggingTransaction(
txn,
name,
self.database_engine,
after_callbacks,
exception_callbacks,
)
r = func(txn, *args, **kwargs)
r = func(cursor, *args, **kwargs)
conn.commit()
return r
except self.database_engine.module.OperationalError as e:
Expand Down Expand Up @@ -456,6 +456,33 @@ def _new_transaction(
)
continue
raise
finally:
# we're either about to retry with a new cursor, or we're about to
# release the connection. Once we release the connection, it could
# get used for another query, which might do a conn.rollback().
#
# In the latter case, even though that probably wouldn't affect the
# results of this transaction, python's sqlite will reset all
# statements on the connection [1], which will make our cursor
# invalid [2].
#
# While the above probably doesn't apply to postgres, we still need
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't actually use cursors in postgres, since we don't name them, therefore we're just fetching all the data on a query. psycopg2's cursor when not named is just an abstraction.

fwiw postgres says that any cursor can see any changes from any other cursor on the same connection, named or not

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also calling .close() on a postgres cursor isn't required as it's implicitly closed at the end of a transaction anyway, as after that it loses semantic meaning. for "client-side cursors" that we use, there's nothing to actually 'close', since no portal was created -- in this case, psycopg2 just sets it as closed and yells if you try and reuse the same cursor.

# to make sure that we have done with the cursor before we release
# the connection, for compatibility with sqlite.
#
# In any case, continuing to read rows after commit()ing seems
# dubious from the PoV of ACID transactional semantics
# (sqlite explicitly says that once you commit, you may see rows
# from subsequent updates.)
#
# In short, if we haven't finished with the cursor yet, that's a
# problem waiting to bite us.
#
# TL;DR: we're done with the cursor, so we can close it.
#
# [1]: https://github.com/python/cpython/blob/v3.8.0/Modules/_sqlite/connection.c#L465
# [2]: https://github.com/python/cpython/blob/v3.8.0/Modules/_sqlite/cursor.c#L236
cursor.close()
except Exception as e:
logger.debug("[TXN FAIL] {%s} %s", name, e)
raise
Expand Down
2 changes: 1 addition & 1 deletion synapse/storage/data_stores/main/receipts.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ def get_all_updated_receipts_txn(txn):
args.append(limit)
txn.execute(sql, args)

return (r[0:5] + (json.loads(r[5]),) for r in txn)
return [r[0:5] + (json.loads(r[5]),) for r in txn]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we not wrap this in an explicit list(), to force it from being a generator?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't that the same as using [] ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, yes, the ()s you replaced keep it being a generator (because it doesn't cast it to a tuple, while [] does cast to a list, because python is an intuitive language)

I'd personally do list(r[0:5] + (json.loads(r[5]),) for r in txn) to make the casting more obvious, but as-written is fine


return self.runInteraction(
"get_all_updated_receipts", get_all_updated_receipts_txn
Expand Down