[Checkins] SVN: transaction/branches/chrism-fix-attempts/ - Fix "for attempt in transaction.attempts(x)" machinery, which would not

[Chris McDonough]

Log message for revision 124787:
  - Fix "for attempt in transaction.attempts(x)" machinery, which would not
    retry a transaction if its implicit call to ``.commit()`` itself raised a
    transient error.  Symptom: seeing conflict errors even though you thought
    you were retrying some number of times via the "attempts" machinery (the
    first attempt to generate an exception during commit would cause that
    exception to be raised).
  
  

Changed:
  U   transaction/branches/chrism-fix-attempts/CHANGES.txt
  U   transaction/branches/chrism-fix-attempts/transaction/_manager.py
  A   transaction/branches/chrism-fix-attempts/transaction/tests/test_attempt.py

-=-
Modified: transaction/branches/chrism-fix-attempts/CHANGES.txt
===================================================================
--- transaction/branches/chrism-fix-attempts/CHANGES.txt	2012-03-29 00:57:38 UTC (rev 124786)
+++ transaction/branches/chrism-fix-attempts/CHANGES.txt	2012-03-29 06:45:32 UTC (rev 124787)
@@ -6,6 +6,13 @@
 
 - Python 3.3 compatibility.
 
+- Fix "for attempt in transaction.attempts(x)" machinery, which would not
+  retry a transaction if its implicit call to ``.commit()`` itself raised a
+  transient error.  Symptom: seeing conflict errors even though you thought
+  you were retrying some number of times via the "attempts" machinery (the
+  first attempt to generate an exception during commit would cause that
+  exception to be raised).
+
 1.2.0 (2011-12-05)
 ------------------
 

Modified: transaction/branches/chrism-fix-attempts/transaction/_manager.py
===================================================================
--- transaction/branches/chrism-fix-attempts/transaction/_manager.py	2012-03-29 00:57:38 UTC (rev 124786)
+++ transaction/branches/chrism-fix-attempts/transaction/_manager.py	2012-03-29 06:45:32 UTC (rev 124787)
@@ -135,8 +135,15 @@
 
     def __exit__(self, t, v, tb):
         if v is None:
-            self.manager.commit()
+            try:
+                self.manager.commit()
+            except TransientError:
+                self.manager.abort()
+                return True # swallow
+            except:
+                self.manager.abort()
+                return False # don't swallow
         else:
             retry = self.manager._retryable(t, v)
             self.manager.abort()
-            return retry
+            return retry # swallow exception if True, else don't swallow

Added: transaction/branches/chrism-fix-attempts/transaction/tests/test_attempt.py
===================================================================
--- transaction/branches/chrism-fix-attempts/transaction/tests/test_attempt.py	                        (rev 0)
+++ transaction/branches/chrism-fix-attempts/transaction/tests/test_attempt.py	2012-03-29 06:45:32 UTC (rev 124787)
@@ -0,0 +1,85 @@
+import unittest
+
+class TestAttempt(unittest.TestCase):
+    def _makeOne(self, manager):
+        from transaction._manager import Attempt
+        return Attempt(manager)
+
+    def test___enter__(self):
+        manager = DummyManager()
+        inst = self._makeOne(manager)
+        inst.__enter__()
+        self.assertTrue(manager.entered)
+
+    def test___exit__no_exc_no_commit_exception(self):
+        manager = DummyManager()
+        inst = self._makeOne(manager)
+        result = inst.__exit__(None, None, None)
+        self.assertFalse(result)
+        self.assertTrue(manager.committed)
+
+    def test___exit__no_exc_nonretryable_commit_exception(self):
+        manager = DummyManager(raise_on_commit=ValueError)
+        inst = self._makeOne(manager)
+        result = inst.__exit__(None, None, None)
+        self.assertFalse(result)
+
+    def test___exit__no_exc_abort_exception_after_nonretryable_commit_exc(self):
+        manager = DummyManager(raise_on_abort=ValueError, 
+                               raise_on_commit=KeyError)
+        inst = self._makeOne(manager)
+        self.assertRaises(ValueError, inst.__exit__, None, None, None)
+        self.assertTrue(manager.committed)
+        self.assertTrue(manager.aborted)
+        
+    def test___exit__no_exc_retryable_commit_exception(self):
+        from transaction.interfaces import TransientError
+        manager = DummyManager(raise_on_commit=TransientError)
+        inst = self._makeOne(manager)
+        result = inst.__exit__(None, None, None)
+        self.assertTrue(result)
+        self.assertTrue(manager.committed)
+        self.assertTrue(manager.aborted)
+
+    def test___exit__with_exception_value_retryable(self):
+        from transaction.interfaces import TransientError
+        manager = DummyManager()
+        inst = self._makeOne(manager)
+        result = inst.__exit__(TransientError, TransientError(), None)
+        self.assertTrue(result)
+        self.assertFalse(manager.committed)
+        self.assertTrue(manager.aborted)
+
+    def test___exit__with_exception_value_nonretryable(self):
+        manager = DummyManager()
+        inst = self._makeOne(manager)
+        result = inst.__exit__(KeyError, KeyError(), None)
+        self.assertFalse(result)
+        self.assertFalse(manager.committed)
+        self.assertTrue(manager.aborted)
+        
+class DummyManager(object):
+    entered = False
+    committed = False
+    aborted = False
+    
+    def __init__(self, raise_on_commit=None, raise_on_abort=None):
+        self.raise_on_commit = raise_on_commit
+        self.raise_on_abort = raise_on_abort
+
+    def _retryable(self, t, v):
+        from transaction._manager import TransientError
+        return issubclass(t, TransientError)
+        
+    def __enter__(self):
+        self.entered = True
+
+    def abort(self):
+        self.aborted = True
+        if self.raise_on_abort:
+            raise self.raise_on_abort
+        
+    def commit(self):
+        self.committed = True
+        if self.raise_on_commit:
+            raise self.raise_on_commit