jamadden: Any interested reviewers?
jamadden: I realized we should really be `release()` the new storage, not closing it. For the MVCC adapters, this doesn't make a difference (they're both no-ops), but on RelStorage there is a difference.

It would be nice to get this merged and the rest of the changes in master released.
jmuchemb: Are you sure relstorage needs things to be done when allocating/freeing instances for undos ? I mean: relstorage does not implement undo_instance and mvccstorage does not nothing for undo instances.

If relstorage could implement undo_instance() in such a way calling release would not be needed, the ZODB code would be simpler.
jamadden: > Are you sure relstorage needs things to be done when allocating/freeing instances for undos ? I mean: relstorage does not implement undo_instance and mvccstorage does not nothing for undo instances.

Yes. RelStorage implements `new_instance`, which this code calls (RelStorage directly implements `IMVCCStorage` so it's not ever wrapped in an mvccadapter). That allocates external resources, and those need to be closed in a timely fashion without relying on the garbage collector. Without this change, database connections leak.

> If relstorage could implement undo_instance() in such a way calling release would not be needed, the ZODB code would be simpler.

I suppose that's true, but I'm not particularly a fan of that for a few different reasons:

1) It seems like it's just the Right Thing To Do to `release()` the storage you allocated by calling `new_instance()`. The responsible party is the one that opened it, they should be the one to close it.
2) Implementing `undo_instance` in that way would require assuming things about how it is used (e.g., always closing everything at the end of `tpc_finish` and `tpc_abort`...just like the code here does, but the code here *knows* how it's using the storage).
3) `undo_instance` is not a documented part of any interface, and requiring storages to implement that would need for it to be documented, and then it would push the complexity of knowing/assuming how the storage is used out to all the storage implementations instead of keeping it centralized here.
diff --git a/CHANGES.rst b/CHANGES.rst
index a8968eb9..3331176d 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -13,12 +13,17 @@
 
 - Drop support for Python 3.4.
 
+- Fix ``DB.undo()`` and ``DB.undoMultiple()`` to close the storage
+  they open behind the scenes when the transaction is committed or
+  rolled back. See `issue 268
+  <https://github.com/zopefoundation/ZODB/issues/268>`_.
+
 5.5.1 (2018-10-25)
 ==================
 
 - Fix KeyError on releasing resources of a Connection when closing the DB.
-  This requires at least version 2.4 of the `transaction` package.
-  See `issue 208 <https://github.com/zopefoundation/ZODB/issues/208>`.
+  This requires at least version 2.4 of the ``transaction`` package.
+  See `issue 208 <https://github.com/zopefoundation/ZODB/issues/208>`_.
 
 5.5.0 (2018-10-13)
 ==================
diff --git a/src/ZODB/DB.py b/src/ZODB/DB.py
index 6a5af7c1..38a9edb6 100644
--- a/src/ZODB/DB.py
+++ b/src/ZODB/DB.py
@@ -14,32 +14,29 @@
 """Database objects
 """
 from __future__ import print_function
+
 import sys
 import logging
 import datetime
 import time
 import warnings
 
-from . import utils
-
-from ZODB.broken import find_global
-from ZODB.utils import z64
-from ZODB.Connection import Connection, TransactionMetaData, noop
-from ZODB._compat import Pickler, _protocol, BytesIO
-import ZODB.serialize
+from persistent.TimeStamp import TimeStamp
+import six
 
+import transaction
 import transaction.weakset
 
 from zope.interface import implementer
+
+from ZODB import utils
 from ZODB.interfaces import IDatabase
 from ZODB.interfaces import IMVCCStorage
-
-import transaction
-
-from persistent.TimeStamp import TimeStamp
-import six
-
-from . import POSException, valuedoc
+from ZODB.broken import find_global
+from ZODB.utils import z64
+from ZODB.Connection import Connection, TransactionMetaData, noop
+import ZODB.serialize
+from ZODB import valuedoc
 
 logger = logging.getLogger('ZODB.DB')
 
@@ -1056,19 +1053,43 @@ class TransactionalUndo(object):
 
     def __init__(self, db, tids):
         self._db = db
-        self._storage = getattr(
-            db._mvcc_storage, 'undo_instance', db._mvcc_storage.new_instance)()
         self._tids = tids
+        self._storage = None
 
     def abort(self, transaction):
         pass
 
+    def close(self):
+        if self._storage is not None:
+            # We actually want to release the storage we've created,
+            # not close it. releasing it frees external resources
+            # dedicated to this instance, closing might make permanent
+            # changes that affect other instances.
+            self._storage.release()
+            self._storage = None
+
     def tpc_begin(self, transaction):
+        assert self._storage is None, "Already in an active transaction"
+
         tdata = TransactionMetaData(
             transaction.user,
             transaction.description,
             transaction.extension)
         transaction.set_data(self, tdata)
+        # `undo_instance` is not part of any IStorage interface;
+        # it is defined in our MVCCAdapter. Regardless, we're opening
+        # a new storage instance, and so we must close it to be sure
+        # to reclaim resources in a timely manner.
+        #
+        # Once the tpc_begin method has been called, the transaction manager will
+        # guarantee to call either `tpc_finish` or `tpc_abort`, so those are the only
+        # methods we need to be concerned about calling close() from.
+        db_mvcc_storage = self._db._mvcc_storage
+        self._storage = getattr(
+            db_mvcc_storage,
+            'undo_instance',
+            db_mvcc_storage.new_instance)()
+
         self._storage.tpc_begin(tdata)
 
     def commit(self, transaction):
@@ -1081,15 +1102,27 @@ class TransactionalUndo(object):
         self._storage.tpc_vote(transaction)
 
     def tpc_finish(self, transaction):
-        transaction = transaction.data(self)
-        self._storage.tpc_finish(transaction)
+        try:
+            transaction = transaction.data(self)
+            self._storage.tpc_finish(transaction)
+        finally:
+            self.close()
 
     def tpc_abort(self, transaction):
-        transaction = transaction.data(self)
-        self._storage.tpc_abort(transaction)
+        try:
+            transaction = transaction.data(self)
+            self._storage.tpc_abort(transaction)
+        finally:
+            self.close()
 
     def sortKey(self):
-        return "%s:%s" % (self._storage.sortKey(), id(self))
+        # The transaction sorts data managers first before it calls
+        # `tpc_begin`, so we can't use our own storage because it's
+        # not open yet. Fortunately new_instances of a storage are
+        # supposed to return the same sort key as the original storage
+        # did.
+        return "%s:%s" % (self._db._mvcc_storage.sortKey(), id(self))
+
 
 def connection(*args, **kw):
     """Create a database :class:`connection <ZODB.Connection.Connection>`.

DB.undo() leaks MVCC storages
I see this [in the relstorage tests](https://github.com/zodb/relstorage/issues/225). That means the underlying database connections leak.

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

```Python traceback
  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.
diff --git a/src/ZODB/tests/testDB.py b/src/ZODB/tests/testDB.py
index f19a6fca..7d24d864 100644
--- a/src/ZODB/tests/testDB.py
+++ b/src/ZODB/tests/testDB.py
@@ -110,6 +110,117 @@ class DBTests(ZODB.tests.util.TestCase):
         check(db.undoLog(0, 3)  , True)
         check(db.undoInfo(0, 3) , True)
 
+
+class TransactionalUndoTests(unittest.TestCase):
+
+    def _makeOne(self):
+        from ZODB.DB import TransactionalUndo
+
+        class MockStorage(object):
+            instance_count = 0
+            close_count = 0
+            release_count = 0
+            begin_count = 0
+            abort_count = 0
+
+            def new_instance(self):
+                self.instance_count += 1
+                return self
+
+            def tpc_begin(self, tx):
+                self.begin_count += 1
+
+            def close(self):
+                self.close_count += 1
+
+            def release(self):
+                self.release_count += 1
+
+            def sortKey(self):
+                return 'MockStorage'
+
+        class MockDB(object):
+
+            def __init__(self):
+                self.storage = self._mvcc_storage = MockStorage()
+
+        return TransactionalUndo(MockDB(), [1])
+
+    def test_only_new_instance_on_begin(self):
+        undo = self._makeOne()
+        self.assertIsNone(undo._storage)
+        self.assertEqual(0, undo._db.storage.instance_count)
+
+        undo.tpc_begin(transaction.get())
+        self.assertIsNotNone(undo._storage)
+        self.assertEqual(1, undo._db.storage.instance_count)
+        self.assertEqual(1, undo._db.storage.begin_count)
+        self.assertIsNotNone(undo._storage)
+
+        # And we can't begin again
+        with self.assertRaises(AssertionError):
+            undo.tpc_begin(transaction.get())
+
+    def test_close_many(self):
+        undo = self._makeOne()
+        self.assertIsNone(undo._storage)
+        self.assertEqual(0, undo._db.storage.instance_count)
+
+        undo.close()
+        # Not open, didn't go through
+        self.assertEqual(0, undo._db.storage.close_count)
+        self.assertEqual(0, undo._db.storage.release_count)
+
+        undo.tpc_begin(transaction.get())
+        undo.close()
+        undo.close()
+        self.assertEqual(0, undo._db.storage.close_count)
+        self.assertEqual(1, undo._db.storage.release_count)
+        self.assertIsNone(undo._storage)
+
+    def test_sortKey(self):
+        # We get the same key whether or not we're open
+        undo = self._makeOne()
+        key = undo.sortKey()
+        self.assertIn('MockStorage', key)
+
+        undo.tpc_begin(transaction.get())
+        key2 = undo.sortKey()
+        self.assertEqual(key, key2)
+
+    def test_tpc_abort_closes(self):
+        undo = self._makeOne()
+        undo.tpc_begin(transaction.get())
+        undo._db.storage.tpc_abort = lambda tx: None
+        undo.tpc_abort(transaction.get())
+        self.assertEqual(0, undo._db.storage.close_count)
+        self.assertEqual(1, undo._db.storage.release_count)
+
+    def test_tpc_abort_closes_on_exception(self):
+        undo = self._makeOne()
+        undo.tpc_begin(transaction.get())
+        with self.assertRaises(AttributeError):
+            undo.tpc_abort(transaction.get())
+        self.assertEqual(0, undo._db.storage.close_count)
+        self.assertEqual(1, undo._db.storage.release_count)
+
+    def test_tpc_finish_closes(self):
+        undo = self._makeOne()
+        undo.tpc_begin(transaction.get())
+        undo._db.storage.tpc_finish = lambda tx: None
+        undo.tpc_finish(transaction.get())
+        self.assertEqual(0, undo._db.storage.close_count)
+        self.assertEqual(1, undo._db.storage.release_count)
+
+    def test_tpc_finish_closes_on_exception(self):
+        undo = self._makeOne()
+        undo.tpc_begin(transaction.get())
+        with self.assertRaises(AttributeError):
+            undo.tpc_finish(transaction.get())
+        self.assertEqual(0, undo._db.storage.close_count)
+        self.assertEqual(1, undo._db.storage.release_count)
+
+
 def test_invalidateCache():
     """The invalidateCache method invalidates a connection caches for all of
     the connections attached to a database::
@@ -423,7 +534,7 @@ def cleanup_on_close():
 """
 
 def test_suite():
-    s = unittest.makeSuite(DBTests)
+    s = unittest.defaultTestLoader.loadTestsFromName(__name__)
     s.addTest(doctest.DocTestSuite(
         setUp=ZODB.tests.util.setUp, tearDown=ZODB.tests.util.tearDown,
         checker=checker

pip install -e .[test]
parse_log_pytest
pytest
pytest
3.7
pytest --no-header -rA --tb=line --color=no -p no:cacheprovider -W ignore::DeprecationWarning
attrs @ file:///croot/attrs_1668696182826/work
BTrees==5.2
certifi @ file:///croot/certifi_1671487769961/work/certifi
cffi==1.15.1
flit_core @ file:///opt/conda/conda-bld/flit-core_1644941570762/work/source/flit_core
importlib-metadata @ file:///tmp/build/80754af9/importlib-metadata_1648562407465/work
iniconfig @ file:///home/linux1/recipes/ci/iniconfig_1610983019677/work
manuel==1.13.0
packaging @ file:///croot/packaging_1671697413597/work
persistent==5.2
pluggy @ file:///tmp/build/80754af9/pluggy_1648042572264/work
py @ file:///opt/conda/conda-bld/py_1644396412707/work
pycparser==2.21
pytest==7.1.2
six==1.17.0
tomli @ file:///opt/conda/conda-bld/tomli_1657175507142/work
transaction==4.0
typing_extensions @ file:///croot/typing_extensions_1669924550328/work
zc.lockfile==3.0.post1
ZConfig==4.1
zipp @ file:///croot/zipp_1672387121353/work
-e git+https://github.com/zopefoundation/ZODB.git@79fe4737e3df5916bd4ce48d49b65caf26000cd4#egg=ZODB
zodbpickle==3.3
zope.exceptions==5.1
zope.interface==6.4.post2
zope.testing==5.0.1
zope.testrunner==6.5

name: ZODB
channels:
  - defaults
  - https://repo.anaconda.com/pkgs/main
  - https://repo.anaconda.com/pkgs/r
  - conda-forge
dependencies:
  - _libgcc_mutex=0.1=main
  - _openmp_mutex=5.1=1_gnu
  - attrs=22.1.0=py37h06a4308_0
  - ca-certificates=2025.2.25=h06a4308_0
  - certifi=2022.12.7=py37h06a4308_0
  - flit-core=3.6.0=pyhd3eb1b0_0
  - importlib-metadata=4.11.3=py37h06a4308_0
  - importlib_metadata=4.11.3=hd3eb1b0_0
  - iniconfig=1.1.1=pyhd3eb1b0_0
  - ld_impl_linux-64=2.40=h12ee557_0
  - libffi=3.4.4=h6a678d5_1
  - libgcc-ng=11.2.0=h1234567_1
  - libgomp=11.2.0=h1234567_1
  - libstdcxx-ng=11.2.0=h1234567_1
  - ncurses=6.4=h6a678d5_0
  - openssl=1.1.1w=h7f8727e_0
  - packaging=22.0=py37h06a4308_0
  - pip=22.3.1=py37h06a4308_0
  - pluggy=1.0.0=py37h06a4308_1
  - py=1.11.0=pyhd3eb1b0_0
  - pytest=7.1.2=py37h06a4308_0
  - python=3.7.16=h7a1cb2a_0
  - readline=8.2=h5eee18b_0
  - setuptools=65.6.3=py37h06a4308_0
  - sqlite=3.45.3=h5eee18b_0
  - tk=8.6.14=h39e8969_0
  - tomli=2.0.1=py37h06a4308_0
  - typing_extensions=4.4.0=py37h06a4308_0
  - wheel=0.38.4=py37h06a4308_0
  - xz=5.6.4=h5eee18b_1
  - zipp=3.11.0=py37h06a4308_0
  - zlib=1.2.13=h5eee18b_1
  - pip:
      - btrees==5.2
      - cffi==1.15.1
      - manuel==1.13.0
      - persistent==5.2
      - pycparser==2.21
      - six==1.17.0
      - transaction==4.0
      - zc-lockfile==3.0.post1
      - zconfig==4.1
      - zodbpickle==3.3
      - zope-exceptions==5.1
      - zope-interface==6.4.post2
      - zope-testing==5.0.1
      - zope-testrunner==6.5
prefix: /opt/conda/envs/ZODB

src/ZODB/tests/testDB.py::TransactionalUndoTests::test_close_many src/ZODB/tests/testDB.py::TransactionalUndoTests::test_only_new_instance_on_begin src/ZODB/tests/testDB.py::TransactionalUndoTests::test_tpc_abort_closes src/ZODB/tests/testDB.py::TransactionalUndoTests::test_tpc_abort_closes_on_exception src/ZODB/tests/testDB.py::TransactionalUndoTests::test_tpc_finish_closes src/ZODB/tests/testDB.py::TransactionalUndoTests::test_tpc_finish_closes_on_exception
src/ZODB/tests/testDB.py::DBTests::testSets src/ZODB/tests/testDB.py::DBTests::test_history_and_undo_meta_data_text_handlinf src/ZODB/tests/testDB.py::DBTests::test_references src/ZODB/tests/testDB.py::TransactionalUndoTests::test_sortKey src/ZODB/tests/testDB.py::test_invalidateCache src/ZODB/tests/testDB.py::test_suite
src/ZODB/DB.py CHANGES.rst
src/ZODB/DB.py CHANGES.rst