Merge 3c3605b into b2c8a2b

.gitignore

@@ -8,6 +8,7 @@
 .coverage
 .coverage.*
 htmlcov/
+coverage.xml
 bin/
 develop-eggs/
 dist/

CHANGES.rst

@@ -7,7 +7,10 @@
 ==================
 
 - Add missing dependency on zope.event.
-
+- Fix raising ``AlreadyInTransaction`` error on the second and
+  subsequent calls to a loop when a transaction synchronizer raises an
+  error on the first call. See `issue 49
+  <https://github.com/NextThought/nti.transactions/issues/49>`_.
 
 4.0.0 (2019-12-13)
 ==================

setup.py

@@ -16,6 +16,7 @@
     'pyramid',
     'zope.component',
     'zope.testrunner',
+    'ZODB',
 ]
 
 def _read(fname):

src/nti/transactions/_loglevels.py

@@ -10,7 +10,7 @@
 
 try:
     from ZODB.loglevels import TRACE
-except ImportError:
+except ImportError: # pragma: no cover
     TRACE = 5
 
 __all__ = [

src/nti/transactions/interfaces.py

@@ -15,6 +15,8 @@
 from transaction.interfaces import ITransaction
 
 # pylint:disable=no-method-argument,inherit-non-class
+# Sigh. The Python 2 version of pylint raises this.
+# pylint:disable=too-many-ancestors
 
 class IExtendedTransaction(ITransaction):
     """Extensions to the transaction api."""

src/nti/transactions/loop.py

@@ -467,6 +467,19 @@ def __call__(self, *args, **kwargs):
             self.tearDown()
             stats.flush()
 
+    def on_begin_failed(self, exc_info, txm, args, kwargs):
+        """
+        Called when ``begin()`` raised an error other than
+        ``AlreadyInTransaction``, probably due to a bad synchronizer.
+        Subclasses may override this method.
+
+        After this is called, the tranasction manager *txm* will be aborted.
+
+        This method must not raise an exception.
+
+        .. versionadded:: 4.0.1
+        """
+
     def __loop(self, txm, note, stats, args, kwargs):
         # pylint:disable=too-many-branches,too-many-statements,too-many-locals
         attempts_remaining = self.attempts
@@ -482,7 +495,27 @@ def __loop(self, txm, note, stats, args, kwargs):
             # Throw away any previous exceptions our loop raised.
             # The TB could be taking lots of memory
             exc_clear()
-            tx = begin()
+            # Bad synchronizers could cause this to raise.
+            # That's not good.
+            try:
+                tx = begin()
+            except AlreadyInTransaction: # pragma: no cover
+                raise
+            except:
+                self.on_begin_failed(sys.exc_info(), txm, args, kwargs)
+
+                try:
+                    txm.abort()
+                except Exception: # pylint:disable=broad-except
+                    logger.exception(
+                        "Failure when aborting transaction, after failure to begin transaction. "
+                        "This exception will be suppressed in favor of the beginning exception "
+                        "on Python 3; on Python 2, the beginning exception will be suppressed "
+                        "in favor of this one."
+                    )
+
+                raise
+
             if note and note is not unused_descr:
                 tx.note(note)
             notify(AfterTransactionBegan(invocation, tx))

src/nti/transactions/tests/test_loop.py

@@ -49,6 +49,25 @@
 from perfmetrics.testing.matchers import is_counter
 from perfmetrics import statsd_client_stack
 
+from ZODB import DB
+from ZODB.DemoStorage import DemoStorage
+from ZODB.POSException import StorageError
+
+
+if str is bytes:
+    # The Python 2 version of hamcrest has a bug
+    # where it assumes the mismatch_description is
+    # not None in one branch.
+    from hamcrest.core.core.allof import AllOf
+    from hamcrest.core.string_description import StringDescription
+    old_func = AllOf.matches.__func__
+    def matches(self, item, mismatch_description=None):
+        if mismatch_description is None:
+            mismatch_description = StringDescription()
+        return old_func(self, item, mismatch_description)
+
+    AllOf.matches = matches
+
 
 class TestCommit(unittest.TestCase):
     class Transaction(object):
@@ -161,6 +180,7 @@ def setUp(self):
             transaction.abort()
         except NoTransaction:
             pass
+        transaction.manager.clearSynchs()
         self.statsd_client = TrueStatsDClient()
         self.statsd_client.random = lambda: 0 # Ignore rate, capture all packets
         statsd_client_stack.push(self.statsd_client)
@@ -170,6 +190,7 @@ def setUp(self):
     def tearDown(self):
         statsd_client_stack.pop()
         zope.event.subscribers.remove(self.events.append)
+        transaction.manager.clearSynchs()
 
     @fudge.patch('nti.transactions.loop._do_commit')
     def test_trivial(self, fake_commit):
@@ -205,6 +226,75 @@ def handler():
         result = TransactionLoop(handler)()
         assert_that(result, is_(42))
 
+    def test_synchronizer_raises_error_on_begin(self):
+        class SynchError(Exception):
+            pass
+
+        class Synch(object):
+            count = 0
+            def newTransaction(self, _txn):
+                self.count += 1
+                if self.count == 1:
+                    raise SynchError
+
+
+            def afterCompletion(self, _txm):
+                pass
+
+            beforeCompletion = afterCompletion
+
+
+        synch = Synch()
+        transaction.manager.registerSynch(synch)
+
+        class HandlerError(Exception):
+            pass
+
+        def handler():
+            raise HandlerError
+
+        # Doing it the first time fails
+        loop = TransactionLoop(handler)
+        with self.assertRaises(SynchError):
+            loop()
+
+        # Our synch doesn't raise the second time,
+        # and we don't get AlreadyInTransaction.
+        with self.assertRaises(HandlerError):
+            loop()
+
+    def test_zodb_synchronizer_raises_error_on_begin(self):
+        # Closely mimic what we see in
+        # https://github.com/NextThought/nti.transactions/issues/49,
+        # where the storage's ``pollInvalidations`` method
+        # raises errors.
+        db = DB(DemoStorage())
+        # The connection has to be open to register a synch
+        conn = db.open()
+
+        def bad_poll_invalidations():
+            raise StorageError
+
+        conn._storage.poll_invalidations = bad_poll_invalidations
+
+        # For the fun of it, lets assume that afterCompletion is also broken
+        class CompletionError(Exception):
+            pass
+        def bad_afterCompletion():
+            raise CompletionError
+        conn._storage.afterCompletion = bad_afterCompletion
+
+        def handler():
+            self.fail("Never get here")
+
+        loop = TransactionLoop(handler)
+
+        # Python 2 and Python 3 raise different things
+        expected = StorageError if str is not bytes else CompletionError
+        for _ in range(2):
+            with self.assertRaises(expected):
+                loop()
+
     def test_explicit_begin(self):
         def handler():
             transaction.begin()

tox.ini

@@ -1,21 +1,34 @@
 [tox]
-envlist = py27,py35,py36,py37,py38,pypy,pypy3,coverage
+envlist = pypy,py27,py35,py36,py37,py38,pypy3,coverage,docs
 
 [testenv]
+# JAM: The comment and setting are cargo-culted from zope.interface.
+# ``usedevelop`` is required otherwise unittest complains that it
+# discovers a file in src/... but imports it from .tox/.../
+# ``skip_install`` also basically works, but that causes the ``extras``
+# not to be installed (though ``deps`` still are), and doesn't
+# rebuild C extensions.
 usedevelop = true
-commands =
-    coverage run -p -m zope.testrunner --test-path=src
-    pylint nti.transactions
 extras = test
-
+commands =
+    coverage run -p -m zope.testrunner --test-path=src  --auto-color --auto-progress [] # substitute with tox positional args
+setenv =
+    PYTHONHASHSEED=1042466059
+    ZOPE_INTERFACE_STRICT_IRO=1
 
 [testenv:coverage]
-basepython =
-    python3.8
+# The -i/--ignore arg may be necessary, I'm not sure.
+# It was cargo-culted over from zope.interface.
 commands =
-    coverage run -p -m zope.testrunner --test-path=src
     coverage combine
-    coverage report --fail-under=100
-deps =
-    coverage
-    pyramid
+    coverage report -i --fail-under=100
+    coverage html -i
+    coverage xml -i
+depends = py27,py36,py37,py38,pypy,pypy3,docs
+parallel_show_output = true
+
+[testenv:docs]
+extras = docs
+basepython = python3
+commands =
+    sphinx-build -b html -d docs/_build/doctrees docs docs/_build/html