[1259] Check the thing is not used after exception

As it would not be safe anyway.

src/lib/python/isc/xfrin/diff.py

@@ -73,12 +73,13 @@ class Diff:
 
     def __check_commited(self):
         """
-        This checks if the diff is already commited. If it is, it raises
-        ValueError. This check is for methods that need to work only on
+        This checks if the diff is already commited or broken. If it is, it
+        raises ValueError. This check is for methods that need to work only on
         yet uncommited diffs.
         """
         if self.__updater is None:
-            raise ValueError("The diff is already commited, you come late")
+            raise ValueError("The diff is already commited or it has raised " +
+                             "an exception, you come late")
 
     def __data_common(self, rr, operation):
         """
@@ -172,15 +173,21 @@ class Diff:
         self.__check_commited()
         # First, compact the data
         self.compact()
-        # Then pass the data inside the data source
-        for (operation, rrset) in self.__buffer:
-            if operation == 'add':
-                self.__updater.add_rrset(rrset)
-            elif operation == 'remove':
-                self.__updater.remove_rrset(rrset)
-            else:
-                raise ValueError('Unknown operation ' + operation)
-        # As everything is already in, drop the buffer
+        try:
+            # Then pass the data inside the data source
+            for (operation, rrset) in self.__buffer:
+                if operation == 'add':
+                    self.__updater.add_rrset(rrset)
+                elif operation == 'remove':
+                    self.__updater.remove_rrset(rrset)
+                else:
+                    raise ValueError('Unknown operation ' + operation)
+            # As everything is already in, drop the buffer
+        except:
+            # If there's a problem, we can't continue.
+            self.__updater = None
+            raise
+
         self.__buffer = []
 
     def commit(self):
@@ -195,10 +202,15 @@ class Diff:
         # Push the data inside the data source
         self.apply()
         # Make sure they are visible.
-        self.__updater.commit()
-        # Remove the updater. That will free some resources for one, but
-        # mark this object as already commited, so we can check
-        self.__updater = None
+        try:
+            self.__updater.commit()
+        finally:
+            # Remove the updater. That will free some resources for one, but
+            # mark this object as already commited, so we can check
+
+            # We remove it even in case the commit failed, as that makes us
+            # unusable.
+            self.__updater = None
 
     def get_buffer(self):
         """

src/lib/python/isc/xfrin/tests/diff_tests.py

@@ -18,6 +18,13 @@ import unittest
 from isc.dns import Name, RRset, RRClass, RRType, RRTTL, Rdata
 from isc.xfrin.diff import Diff, NoSuchZone
 
+class TestError(Exception):
+    """
+    Just to have something to be raised during the tests.
+    Not used outside.
+    """
+    pass
+
 class DiffTest(unittest.TestCase):
     """
     Tests for the isc.xfrin.diff.Diff class.
@@ -37,6 +44,7 @@ class DiffTest(unittest.TestCase):
         self.__data_operations = []
         self.__apply_called = False
         self.__commit_called = False
+        self.__broken_called = False
         # Some common values
         self.__rrclass = RRClass.IN()
         self.__type = RRType.A()
@@ -73,6 +81,15 @@ class DiffTest(unittest.TestCase):
         """
         self.__apply_called = True
 
+    def __broken_operation(self, *args):
+        """
+        This can be used whenever an operation should fail. It raises TestError.
+        It should take whatever amount of parameters needed, so it can be put
+        quite anywhere.
+        """
+        self.__broken_called = True
+        raise TestError("Test error")
+
     def commit(self):
         """
         This is part of pretending to be a zone updater. This notes the commit
@@ -340,6 +357,45 @@ class DiffTest(unittest.TestCase):
         self.assertRaises(ValueError, diff.add_data, rrset)
         self.assertRaises(ValueError, diff.remove_data, rrset)
 
+    def __do_raise_test(self):
+        """
+        Do a raise test. Expects that one of the operations is exchanged for
+        broken version.
+        """
+        diff = Diff(self, Name('example.org.'))
+        diff.add_data(self.__rrset1)
+        diff.remove_data(self.__rrset2)
+        self.assertRaises(TestError, diff.commit)
+        self.assertTrue(self.__broken_called)
+        self.assertRaises(ValueError, diff.add_data, self.__rrset1)
+        self.assertRaises(ValueError, diff.remove_data, self.__rrset2)
+        self.assertRaises(ValueError, diff.commit)
+        self.assertRaises(ValueError, diff.apply)
+
+    def test_raise_add(self):
+        """
+        Test the exception from add_rrset is propagated and the diff can't be
+        used afterwards.
+        """
+        self.add_rrset = self.__broken_operation
+        self.__do_raise_test()
+
+    def test_raise_remove(self):
+        """
+        Test the exception from remove_rrset is propagated and the diff can't be
+        used afterwards.
+        """
+        self.remove_rrset = self.__broken_operation
+        self.__do_raise_test()
+
+    def test_raise_commit(self):
+        """
+        Test the exception from updater's commit gets propagated and it can't be
+        used afterwards.
+        """
+        self.commit = self.__broken_operation
+        self.__do_raise_test()
+
 if __name__ == "__main__":
     isc.log.init("bind10")
     unittest.main()