DB.undo() leaks MVCC storages

[jamadden]

I see this in the relstorage tests. That means the underlying database connections leak.

Here's where a storage is allocated by calling new_instance():

  File "//relstorage/src/relstorage/tests/blob/testblob.py", line 146, in testUndoWithoutPreviousVersion
    database.undo(database.undoLog(0, 1)[0]['id'])
  File "//lib/python3.7/site-packages/ZODB/DB.py", line 997, in undo
    self.undoMultiple([id], txn)
  File "//lib/python3.7/site-packages/ZODB/DB.py", line 978, in undoMultiple
    txn.join(TransactionalUndo(self, ids))
  File "//lib/python3.7/site-packages/ZODB/DB.py", line 1060, in __init__
    db._mvcc_storage, 'undo_instance', db._mvcc_storage.new_instance)()

The TransactionalUndo object never closes the new storage.

RelStorage could potentially work around this (e.g., implement a 'undo_instance' method that returns a special storage wrapper that closes itself in tpc_finish and tpc_abort), but it seems like the more general solution would be for TransactionalUndo to close the storage it opened in its implementation of those methods.

[jamadden]

This problem is made worse by the fact that there's a reference cycle involved, so the connections won't get cleaned up until the GC runs at some arbitrary point in the future.

The reference cycle was completely non-intuitive. The active transaction points to the TransactionalUndo instance as a resource manager (Transaction._resources), which points to the storage instance (TransactionalUndo._storage); the storage instance has methods that have __globals__ being the module dictionary of relstorage.storage; in turn (through a chain of module imports) the transaction module is referenced. The transaction module has an instance of ThreadTransactionManager, which has a thread-local dictionary containing the current transaction manager, which in turn points back to the active transaction.

1. active transaction instance ->
2. TransactionUndo instance ->
3. RelStorage instance ->
4. module globals (relstorage.storage->ZODB->ZODB.POSException->transaction) ->
5. transaction module ->
6. ThreadTransactionManager instance ->
7. thread local dict ->
8. _txn is the active transaction instance from step 1

EDIT: As pointed out in zopefoundation/transaction#82, the transaction does drop the IDataManager objects it has when a transaction is committed, so there must be another cycle somewhere.

[d-maurer]

The ZEO tests produce lots of ResourceWarnings of the form unclosed file, unclosed socket. Maybe, the cycles come not from transaction?

[jamadden]

I'll try to manually break this cycle and then go hunting again.

Failure to close the storage was easy to diagnose and correct, though.