Merge tim-deprecate-subtxn branch.

Subtransactions are deprecated in 3.5, and will go away
in 3.7.

NEWS.txt

@@ -27,16 +27,16 @@ Savepoints
   marked a savepoint as invalid after its first use.  The implementation has
   been repaired, to match the docs.
 
-Subtransactions
----------------
+Subtransactions are deprecated
+------------------------------
 
-- (3.5a4) Internal uses of subtransactions (transaction ``commit()`` or
-  ``abort()`` passing a true argument) were rewritten to use savepoints
-  instead.  Application code is strongly encouraged to do this too:
-  subtransactions are weaker, will be deprecated soon, and do not mix well
-  with savepoints (when you do a subtransaction commit, all current
-  savepoints are made unusable).  In general, a subtransaction commit
-  done just to free memory can be changed from::
+- (3.5a4) Subtransactions are deprecated, and will be removed in ZODB 3.7.
+  Use savepoints instead.  Savepoints are more powerful, and code using
+  subtransactions does not mix well with code using savepoints (a
+  subtransaction commit forces all current savepoints to become unusable, so
+  code using subtransactions can hurt newer code trying to use savepoints).
+  In general, a subtransaction commit done just to free memory can be changed
+  from::
 
       transaction.commit(1)
 
@@ -62,6 +62,10 @@ Subtransactions
 
       sp.rollback()
 
+- (3.5a4) Internal uses of subtransactions (transaction ``commit()`` or
+  ``abort()`` passing a true argument) were rewritten to use savepoints
+  instead.
+
 Multi-database
 --------------
 

src/ZODB/tests/testConnection.py

@@ -24,6 +24,12 @@ from ZODB.utils import p64, u64
 from ZODB.tests.warnhook import WarningsHook
 from zope.interface.verify import verifyObject
 
+# deprecated37  remove when subtransactions go away
+# Don't complain about subtxns in these tests.
+warnings.filterwarnings("ignore",
+                        ".*\nsubtransactions are deprecated",
+                        DeprecationWarning, __name__)
+
 class ConnectionDotAdd(unittest.TestCase):
 
     def setUp(self):

src/ZODB/tests/testSubTransaction.py

@@ -11,10 +11,16 @@
 # FOR A PARTICULAR PURPOSE.
 #
 ##############################################################################
-r"""
+"""
 ZODB subtransaction tests
 =========================
 
+Subtransactions are deprecated.  First we install a hook, to verify that
+deprecation warnings are generated.
+
+>>> hook = WarningsHook()
+>>> hook.install()
+
 Subtransactions are provided by a generic transaction interface, but
 only supported by ZODB.  These tests verify that some of the important
 cases work as expected.
@@ -61,6 +67,19 @@ We'll make a series of modifications in subtransactions.
 >>> shadow_a.value, shadow_b.value
 ('a0', 'b0')
 
+The subtransaction commit should have generated a deprecation wng:
+
+>>> len(hook.warnings)
+1
+>>> message, category, filename, lineno = hook.warnings[0]
+>>> print message
+This will be removed in ZODB 3.7:
+subtransactions are deprecated; use transaction.savepoint() instead of \
+transaction.commit(1)
+>>> category.__name__
+'DeprecationWarning'
+>>> hook.clear()
+
 >>> a.value = "a2"
 >>> c.value = "c1"
 >>> transaction.commit(1)
@@ -100,6 +119,21 @@ database state as of the last sub-transaction commit.  There is
 >>> a.value, b.value, c.value
 ('a1', 'b1', 'c0')
 
+The subtxn abort should also have generated a deprecation warning:
+
+>>> len(hook.warnings)
+1
+>>> message, category, filename, lineno = hook.warnings[0]
+>>> print message
+This will be removed in ZODB 3.7:
+subtransactions are deprecated; use sp.rollback() instead of \
+transaction.abort(1), where `sp` is the corresponding savepoint \
+captured earlier
+>>> category.__name__
+'DeprecationWarning'
+>>> hook.clear()
+
+
 Multiple aborts have no extra effect.
 
 >>> transaction.abort(1)
@@ -131,8 +165,14 @@ database state as of the last sub-transaction commit.  There is
 >>> a.value, b.value, c.value
 ('a0', 'b0', 'c0')
 
+We have to uninstall the hook so that other warnings don't get lost.
+
+>>> len(hook.warnings)  # we don't expect we captured other warnings
+0
+>>> hook.uninstall()
 """
 
+from ZODB.tests.warnhook import WarningsHook
 from zope.testing import doctest
 
 def test_suite():

src/ZODB/tests/testZODB.py

@@ -12,6 +12,7 @@
 #
 ##############################################################################
 import unittest
+import warnings
 
 import ZODB
 import ZODB.FileStorage
@@ -23,6 +24,12 @@ from persistent import Persistent
 from persistent.mapping import PersistentMapping
 import transaction
 
+# deprecated37  remove when subtransactions go away
+# Don't complain about subtxns in these tests.
+warnings.filterwarnings("ignore",
+                        ".*\nsubtransactions are deprecated",
+                        DeprecationWarning, __name__)
+
 class P(Persistent):
     pass
 
@@ -430,6 +437,72 @@ class ZODBTests(unittest.TestCase):
         finally:
             tm1.abort()
 
+    def checkSavepointDoesntGetInvalidations(self):
+        # Prior to ZODB 3.2.9 and 3.4, Connection.tpc_finish() processed
+        # invalidations even for a subtxn commit.  This could make
+        # inconsistent state visible after a subtxn commit.  There was a
+        # suspicion that POSKeyError was possible as a result, but I wasn't
+        # able to construct a case where that happened.
+        # Subtxns are deprecated now, but it's good to check that the
+        # same kind of thing doesn't happen when making savepoints either.
+
+        # Set up the database, to hold
+        # root --> "p" -> value = 1
+        #      --> "q" -> value = 2
+        tm1 = transaction.TransactionManager()
+        conn = self._db.open(transaction_manager=tm1)
+        r1 = conn.root()
+        p = P()
+        p.value = 1
+        r1["p"] = p
+        q = P()
+        q.value = 2
+        r1["q"] = q
+        tm1.commit()
+
+        # Now txn T1 changes p.value to 3 locally (subtxn commit).
+        p.value = 3
+        tm1.savepoint()
+
+        # Start new txn T2 with a new connection.
+        tm2 = transaction.TransactionManager()
+        cn2 = self._db.open(transaction_manager=tm2)
+        r2 = cn2.root()
+        p2 = r2["p"]
+        self.assertEqual(p._p_oid, p2._p_oid)
+        # T2 shouldn't see T1's change of p.value to 3, because T1 didn't
+        # commit yet.
+        self.assertEqual(p2.value, 1)
+        # Change p.value to 4, and q.value to 5.  Neither should be visible
+        # to T1, because T1 is still in progress.
+        p2.value = 4
+        q2 = r2["q"]
+        self.assertEqual(q._p_oid, q2._p_oid)
+        self.assertEqual(q2.value, 2)
+        q2.value = 5
+        tm2.commit()
+
+        # Back to T1.  p and q still have the expected values.
+        rt = conn.root()
+        self.assertEqual(rt["p"].value, 3)
+        self.assertEqual(rt["q"].value, 2)
+
+        # Now make another savepoint in T1.  This shouldn't change what
+        # T1 sees for p and q.
+        rt["r"] = P()
+        tm1.savepoint()
+
+        # Making that savepoint in T1 should not process invalidations
+        # from T2's commit.  p.value should still be 3 here (because that's
+        # what T1 savepointed earlier), and q.value should still be 2.
+        # Prior to ZODB 3.2.9 and 3.4, q.value was 5 here.
+        rt = conn.root()
+        try:
+            self.assertEqual(rt["p"].value, 3)
+            self.assertEqual(rt["q"].value, 2)
+        finally:
+            tm1.abort()
+
     def checkReadConflictErrorClearedDuringAbort(self):
         # When a transaction is aborted, the "memory" of which
         # objects were the cause of a ReadConflictError during
@@ -565,6 +638,7 @@ class ZODBTests(unittest.TestCase):
 
     def checkFailingCommitSticks(self):
         # See also checkFailingSubtransactionCommitSticks.
+        # See also checkFailingSavepointSticks.
         cn = self._db.open()
         rt = cn.root()
         rt['a'] = 1
@@ -575,9 +649,9 @@ class ZODBTests(unittest.TestCase):
 
         self.assertRaises(PoisonedError, transaction.get().commit)
         # Trying to commit again fails too.
-        self.assertRaises(TransactionFailedError, transaction.get().commit)
-        self.assertRaises(TransactionFailedError, transaction.get().commit)
-        self.assertRaises(TransactionFailedError, transaction.get().commit)
+        self.assertRaises(TransactionFailedError, transaction.commit)
+        self.assertRaises(TransactionFailedError, transaction.commit)
+        self.assertRaises(TransactionFailedError, transaction.commit)
 
         # The change to rt['a'] is lost.
         self.assertRaises(KeyError, rt.__getitem__, 'a')
@@ -587,16 +661,16 @@ class ZODBTests(unittest.TestCase):
         self.assertRaises(TransactionFailedError, rt.__setitem__, 'b', 2)
 
         # Clean up via abort(), and try again.
-        transaction.get().abort()
+        transaction.abort()
         rt['a'] = 1
-        transaction.get().commit()
+        transaction.commit()
         self.assertEqual(rt['a'], 1)
 
         # Cleaning up via begin() should also work.
         rt['a'] = 2
         transaction.get().register(poisoned)
-        self.assertRaises(PoisonedError, transaction.get().commit)
-        self.assertRaises(TransactionFailedError, transaction.get().commit)
+        self.assertRaises(PoisonedError, transaction.commit)
+        self.assertRaises(TransactionFailedError, transaction.commit)
         # The change to rt['a'] is lost.
         self.assertEqual(rt['a'], 1)
         # Trying to modify an object also fails.
@@ -604,7 +678,7 @@ class ZODBTests(unittest.TestCase):
         # Clean up via begin(), and try again.
         transaction.begin()
         rt['a'] = 2
-        transaction.get().commit()
+        transaction.commit()
         self.assertEqual(rt['a'], 2)
 
         cn.close()
@@ -613,23 +687,38 @@ class ZODBTests(unittest.TestCase):
         cn = self._db.open()
         rt = cn.root()
         rt['a'] = 1
-        transaction.get().commit(True)
+        transaction.commit(True)
         self.assertEqual(rt['a'], 1)
 
         rt['b'] = 2
 
-        # Subtransactions don't do tpc_vote, so we poison tpc_begin.
-        poisoned = PoisonedJar()
+        # Make a jar that raises PoisonedError when a subtxn commit is done.
+        poisoned = PoisonedJar(break_savepoint=True)
         transaction.get().join(poisoned)
-        poisoned.break_savepoint = True
-        self.assertRaises(PoisonedError, transaction.get().commit, True)
+        # We're using try/except here instead of assertRaises so that this
+        # module's attempt to suppress subtransaction deprecation wngs
+        # works.
+        try:
+            transaction.commit(True)
+        except PoisonedError:
+            pass
+        else:
+            self.fail("expected PoisonedError")
         # Trying to subtxn-commit again fails too.
-        self.assertRaises(TransactionFailedError,
-                          transaction.get().commit, True)
-        self.assertRaises(TransactionFailedError,
-                          transaction.get().commit, True)
+        try:
+            transaction.commit(True)
+        except TransactionFailedError:
+            pass
+        else:
+            self.fail("expected TransactionFailedError")
+        try:
+            transaction.commit(True)
+        except TransactionFailedError:
+            pass
+        else:
+            self.fail("expected TransactionFailedError")
         # Top-level commit also fails.
-        self.assertRaises(TransactionFailedError, transaction.get().commit)
+        self.assertRaises(TransactionFailedError, transaction.commit)
 
         # The changes to rt['a'] and rt['b'] are lost.
         self.assertRaises(KeyError, rt.__getitem__, 'a')
@@ -639,21 +728,28 @@ class ZODBTests(unittest.TestCase):
         # also raises TransactionFailedError.
         self.assertRaises(TransactionFailedError, rt.__setitem__, 'b', 2)
 
-
         # Clean up via abort(), and try again.
-        transaction.get().abort()
+        transaction.abort()
         rt['a'] = 1
-        transaction.get().commit()
+        transaction.commit()
         self.assertEqual(rt['a'], 1)
 
         # Cleaning up via begin() should also work.
         rt['a'] = 2
-        poisoned = PoisonedJar()
         transaction.get().join(poisoned)
-        poisoned.break_savepoint = True
-        self.assertRaises(PoisonedError, transaction.get().commit, True)
-        self.assertRaises(TransactionFailedError,
-                          transaction.get().commit, True)
+        try:
+            transaction.commit(True)
+        except PoisonedError:
+            pass
+        else:
+            self.fail("expected PoisonedError")
+        # Trying to subtxn-commit again fails too.
+        try:
+            transaction.commit(True)
+        except TransactionFailedError:
+            pass
+        else:
+            self.fail("expected TransactionFailedError")
 
         # The change to rt['a'] is lost.
         self.assertEqual(rt['a'], 1)
@@ -663,7 +759,7 @@ class ZODBTests(unittest.TestCase):
         # Clean up via begin(), and try again.
         transaction.begin()
         rt['a'] = 2
-        transaction.get().commit(True)
+        transaction.commit(True)
         self.assertEqual(rt['a'], 2)
         transaction.get().commit()
 
@@ -674,11 +770,69 @@ class ZODBTests(unittest.TestCase):
         cn.close()
         cn2.close()
 
+    def checkFailingSavepointSticks(self):
+        cn = self._db.open()
+        rt = cn.root()
+        rt['a'] = 1
+        transaction.savepoint()
+        self.assertEqual(rt['a'], 1)
+
+        rt['b'] = 2
+
+        # Make a jar that raises PoisonedError when making a savepoint.
+        poisoned = PoisonedJar(break_savepoint=True)
+        transaction.get().join(poisoned)
+        self.assertRaises(PoisonedError, transaction.savepoint)
+        # Trying to make a savepoint again fails too.
+        self.assertRaises(TransactionFailedError, transaction.savepoint)
+        self.assertRaises(TransactionFailedError, transaction.savepoint)
+        # Top-level commit also fails.
+        self.assertRaises(TransactionFailedError, transaction.commit)
+
+        # The changes to rt['a'] and rt['b'] are lost.
+        self.assertRaises(KeyError, rt.__getitem__, 'a')
+        self.assertRaises(KeyError, rt.__getitem__, 'b')
+
+        # Trying to modify an object also fails, because Transaction.join()
+        # also raises TransactionFailedError.
+        self.assertRaises(TransactionFailedError, rt.__setitem__, 'b', 2)
+
+        # Clean up via abort(), and try again.
+        transaction.abort()
+        rt['a'] = 1
+        transaction.commit()
+        self.assertEqual(rt['a'], 1)
+
+        # Cleaning up via begin() should also work.
+        rt['a'] = 2
+        transaction.get().join(poisoned)
+        self.assertRaises(PoisonedError, transaction.savepoint)
+        # Trying to make a savepoint again fails too.
+        self.assertRaises(TransactionFailedError, transaction.savepoint)
+
+        # The change to rt['a'] is lost.
+        self.assertEqual(rt['a'], 1)
+        # Trying to modify an object also fails.
+        self.assertRaises(TransactionFailedError, rt.__setitem__, 'b', 2)
+
+        # Clean up via begin(), and try again.
+        transaction.begin()
+        rt['a'] = 2
+        transaction.savepoint()
+        self.assertEqual(rt['a'], 2)
+        transaction.commit()
+
+        cn2 = self._db.open()
+        rt = cn.root()
+        self.assertEqual(rt['a'], 2)
+
+        cn.close()
+        cn2.close()
+
 class PoisonedError(Exception):
     pass
 
-# PoisonedJar arranges to raise exceptions from interesting places.
-# For whatever reason, subtransaction commits don't call tpc_vote.
+# PoisonedJar arranges to raise PoisonedError from interesting places.
 class PoisonedJar:
     def __init__(self, break_tpc_begin=False, break_tpc_vote=False,
                  break_savepoint=False):
@@ -689,7 +843,8 @@ class PoisonedJar:
     def sortKey(self):
         return str(id(self))
 
-    # A way to poison a subtransaction commit.
+    # A way that used to poison a subtransaction commit.  With the current
+    # implementation of subtxns, pass break_savepoint=True instead.
     def tpc_begin(self, *args):
         if self.break_tpc_begin:
             raise PoisonedError("tpc_begin fails")
@@ -699,6 +854,7 @@ class PoisonedJar:
         if self.break_tpc_vote:
             raise PoisonedError("tpc_vote fails")
 
+    # A way to poison a savepoint -- also a way to poison a subtxn commit.
     def savepoint(self):
         if self.break_savepoint:
             raise PoisonedError("savepoint fails")

src/ZODB/utils.py

@@ -38,6 +38,7 @@ __all__ = ['z64',
            'WeakSet',
            'DEPRECATED_ARGUMENT',
            'deprecated36',
+           'deprecated37',
            'get_pickle_metadata',
           ]
 
@@ -57,6 +58,13 @@ def deprecated36(msg):
     warnings.warn("This will be removed in ZODB 3.6:\n%s" % msg,
                   DeprecationWarning, stacklevel=3)
 
+# Raise DeprecationWarning, noting that the deprecated thing will go
+# away in ZODB 3.7.  Point to the caller of our caller (i.e., at the
+# code using the deprecated thing).
+def deprecated37(msg):
+    warnings.warn("This will be removed in ZODB 3.7:\n%s" % msg,
+                  DeprecationWarning, stacklevel=3)
+
 z64 = '\0'*8
 
 assert sys.hexversion >= 0x02030000

src/transaction/_manager.py

@@ -21,6 +21,11 @@ import thread
 
 from transaction._transaction import Transaction
 
+# Used for deprecated arguments.  ZODB.utils.DEPRECATED_ARGUMENT is
+# too hard to use here, due to the convoluted import dance across
+# __init__.py files.
+_marker = object()
+
 # We have to remember sets of synch objects, especially Connections.
 # But we don't want mere registration with a transaction manager to
 # keep a synch object alive forever; in particular, it's common
@@ -80,11 +85,26 @@ class TransactionManager(object):
     def unregisterSynch(self, synch):
         self._synchs.remove(synch)
 
-    def commit(self, sub=False):
-        self.get().commit(sub)
-
-    def abort(self, sub=False):
-        self.get().abort(sub)
+    def commit(self, sub=_marker):
+        if sub is _marker:
+            sub = None
+        else:
+            from ZODB.utils import deprecated37
+            deprecated37("subtransactions are deprecated; use "
+                         "transaction.savepoint() instead of "
+                         "transaction.commit(1)")
+        return self.get().commit(sub, deprecation_wng=False)
+
+    def abort(self, sub=_marker):
+        if sub is _marker:
+            sub = None
+        else:
+            from ZODB.utils import deprecated37
+            deprecated37("subtransactions are deprecated; use "
+                         "sp.rollback() instead of "
+                         "transaction.abort(1), where `sp` is the "
+                         "corresponding savepoint captured earlier")
+        return self.get().abort(sub, deprecation_wng=False)
 
     def savepoint(self, optimistic=False):
         return self.get().savepoint(optimistic)

src/transaction/_transaction.py

@@ -358,7 +358,15 @@ class Transaction(object):
         # in which case it would do nothing besides uselessly free() this
         # transaction.
 
-    def commit(self, subtransaction=False):
+    def commit(self, subtransaction=_marker, deprecation_wng=True):
+        if subtransaction is _marker:
+            subtransaction = 0
+        elif deprecation_wng:
+            from ZODB.utils import deprecated37
+            deprecated37("subtransactions are deprecated; use "
+                         "transaction.savepoint() instead of "
+                         "transaction.commit(1)")
+
         if self._savepoint2index:
             self._invalidate_all_savepoints()
 
@@ -464,7 +472,16 @@ class Transaction(object):
                 self.log.error("Error in tpc_abort() on manager %s",
                                rm, exc_info=sys.exc_info())
 
-    def abort(self, subtransaction=False):
+    def abort(self, subtransaction=_marker, deprecation_wng=True):
+        if subtransaction is _marker:
+            subtransaction = 0
+        elif deprecation_wng:
+            from ZODB.utils import deprecated37
+            deprecated37("subtransactions are deprecated; use "
+                         "sp.rollback() instead of "
+                         "transaction.abort(1), where `sp` is the "
+                         "corresponding savepoint captured earlier")
+
         if subtransaction:
             # TODO deprecate subtransactions.
             if not self._subtransaction_savepoint:

src/transaction/tests/test_transaction.py

@@ -40,9 +40,17 @@ $Id$
 """
 
 import unittest
+import warnings
+
 import transaction
 from ZODB.utils import positive_id
 
+# deprecated37  remove when subtransactions go away
+# Don't complain about subtxns in these tests.
+warnings.filterwarnings("ignore",
+                        ".*\nsubtransactions are deprecated",
+                        DeprecationWarning, __name__)
+
 class TransactionTests(unittest.TestCase):
 
     def setUp(self):
@@ -212,7 +220,8 @@ class TransactionTests(unittest.TestCase):
 
         try:
             self.transaction_manager.commit()
-        except TestTxnException: pass
+        except TestTxnException:
+            pass
 
         assert self.nosub1._p_jar.ctpc_abort == 1
         assert self.sub1._p_jar.ctpc_abort == 1
@@ -444,10 +453,14 @@ def test_beforeCommitHook():
       >>> log
       []
 
-    The hook is only called for a full commit, not for subtransactions.
+    The hook is only called for a full commit, not for a savepoint or
+    subtransaction.
 
       >>> t = transaction.begin()
       >>> t.beforeCommitHook(hook, 'A', kw1='B')
+      >>> dummy = t.savepoint()
+      >>> log
+      []
       >>> t.commit(subtransaction=True)
       >>> log
       []