mutable.modify(): after UCWE, publish even if the second invocation of the modifier...

diff --git a/src/allmydata/mutable/node.py b/src/allmydata/mutable/node.py
index 3e6338e6e821b4c3094a996b090df5f3224a7f9e..cfa823d86e12e69d6994da22075a37327b0c8ddf 100644 (file)
--- a/src/allmydata/mutable/node.py
+++ b/src/allmydata/mutable/node.py
@@ -387,7 +387,13 @@ class MutableFileNode:
             new_contents = modifier(old_contents, servermap, first_time)
             if new_contents is None or new_contents == old_contents:
                 # no changes need to be made
-                return
+                if first_time:
+                    return
+                # However, since Publish is not automatically doing a
+                # recovery when it observes UCWE, we need to do a second
+                # publish. See #551 for details. We'll basically loop until
+                # we managed an uncontested publish.
+                new_contents = old_contents
             precondition(isinstance(new_contents, str),
                          "Modifier function must return a string or None")
             return self._upload(new_contents, servermap)

diff --git a/src/allmydata/test/test_dirnode.py b/src/allmydata/test/test_dirnode.py
index 0447e9c1b1eaf7a6a594514cf509b168e851ef10..657a8b58def9e81de4beded7d5c4d7a4181cca2b 100644 (file)
--- a/src/allmydata/test/test_dirnode.py
+++ b/src/allmydata/test/test_dirnode.py
@@ -769,7 +769,10 @@ class UCWEingMutableFileNode(MutableFileNode):
     def _upload(self, new_contents, servermap):
         d = MutableFileNode._upload(self, new_contents, servermap)
         def _ucwe(res):
-            raise UncoordinatedWriteError()
+            if self.please_ucwe_after_next_upload:
+                self.please_ucwe_after_next_upload = False
+                raise UncoordinatedWriteError()
+            return res
         d.addCallback(_ucwe)
         return d
 class UCWEingNewDirectoryNode(dirnode.NewDirectoryNode):
@@ -799,6 +802,8 @@ class Deleter(SystemTestMixin, unittest.TestCase):
         d.addCallback(_created_dir)
         def _do_delete(ignored):
             n = UCWEingNewDirectoryNode(self.clients[0]).init_from_uri(self.root_uri)
+            assert n._node.please_ucwe_after_next_upload == False
+            n._node.please_ucwe_after_next_upload = True
             # This should succeed, not raise an exception
             return n.delete(u"file")
         d.addCallback(_do_delete)

diff --git a/src/allmydata/test/test_mutable.py b/src/allmydata/test/test_mutable.py
index ad368280d8fe737fb76dbe1247d46ed6db6318bb..055b464997ba9dc1e7d6a6ca68617ccf736c14c3 100644 (file)
--- a/src/allmydata/test/test_mutable.py
+++ b/src/allmydata/test/test_mutable.py
@@ -374,11 +374,11 @@ class Filenode(unittest.TestCase, testutil.ShouldFailMixin):
         d.addCallback(_created)
         return d
 
-    def failUnlessCurrentSeqnumIs(self, n, expected_seqnum):
+    def failUnlessCurrentSeqnumIs(self, n, expected_seqnum, which):
         d = n.get_servermap(MODE_READ)
         d.addCallback(lambda servermap: servermap.best_recoverable_version())
         d.addCallback(lambda verinfo:
-                      self.failUnlessEqual(verinfo[0], expected_seqnum))
+                      self.failUnlessEqual(verinfo[0], expected_seqnum, which))
         return d
 
     def test_modify(self):
@@ -399,30 +399,37 @@ class Filenode(unittest.TestCase, testutil.ShouldFailMixin):
             if len(calls) <= 1:
                 raise UncoordinatedWriteError("simulated")
             return old_contents + "line3"
+        def _ucw_error_non_modifier(old_contents, servermap, first_time):
+            # simulate an UncoordinatedWriteError once, and don't actually
+            # modify the contents on subsequent invocations
+            calls.append(1)
+            if len(calls) <= 1:
+                raise UncoordinatedWriteError("simulated")
+            return old_contents
 
         d = self.client.create_mutable_file("line1")
         def _created(n):
             d = n.modify(_modifier)
             d.addCallback(lambda res: n.download_best_version())
             d.addCallback(lambda res: self.failUnlessEqual(res, "line1line2"))
-            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2))
+            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2, "m"))
 
             d.addCallback(lambda res: n.modify(_non_modifier))
             d.addCallback(lambda res: n.download_best_version())
             d.addCallback(lambda res: self.failUnlessEqual(res, "line1line2"))
-            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2))
+            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2, "non"))
 
             d.addCallback(lambda res: n.modify(_none_modifier))
             d.addCallback(lambda res: n.download_best_version())
             d.addCallback(lambda res: self.failUnlessEqual(res, "line1line2"))
-            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2))
+            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2, "none"))
 
             d.addCallback(lambda res:
                           self.shouldFail(ValueError, "error_modifier", None,
                                           n.modify, _error_modifier))
             d.addCallback(lambda res: n.download_best_version())
             d.addCallback(lambda res: self.failUnlessEqual(res, "line1line2"))
-            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2))
+            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2, "err"))
 
             d.addCallback(lambda res:
                           self.shouldFail(FileTooLargeError, "toobig_modifier",
@@ -430,14 +437,32 @@ class Filenode(unittest.TestCase, testutil.ShouldFailMixin):
                                           n.modify, _toobig_modifier))
             d.addCallback(lambda res: n.download_best_version())
             d.addCallback(lambda res: self.failUnlessEqual(res, "line1line2"))
-            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2))
+            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2, "big"))
 
             d.addCallback(lambda res: n.modify(_ucw_error_modifier))
             d.addCallback(lambda res: self.failUnlessEqual(len(calls), 2))
             d.addCallback(lambda res: n.download_best_version())
             d.addCallback(lambda res: self.failUnlessEqual(res,
                                                            "line1line2line3"))
-            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 3))
+            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 3, "ucw"))
+
+            def _reset_ucw_error_modifier(res):
+                calls[:] = []
+                return res
+            d.addCallback(_reset_ucw_error_modifier)
+
+            # in practice, this n.modify call should publish twice: the first
+            # one gets a UCWE, the second does not. But our test jig (in
+            # which the modifier raises the UCWE) skips over the first one,
+            # so in this test there will be only one publish, and the seqnum
+            # will only be one larger than the previous test, not two (i.e. 4
+            # instead of 5).
+            d.addCallback(lambda res: n.modify(_ucw_error_non_modifier))
+            d.addCallback(lambda res: self.failUnlessEqual(len(calls), 2))
+            d.addCallback(lambda res: n.download_best_version())
+            d.addCallback(lambda res: self.failUnlessEqual(res,
+                                                           "line1line2line3"))
+            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 4, "ucw"))
 
             return d
         d.addCallback(_created)
@@ -472,7 +497,7 @@ class Filenode(unittest.TestCase, testutil.ShouldFailMixin):
             d = n.modify(_modifier)
             d.addCallback(lambda res: n.download_best_version())
             d.addCallback(lambda res: self.failUnlessEqual(res, "line1line2"))
-            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2))
+            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2, "m"))
 
             d.addCallback(lambda res:
                           self.shouldFail(UncoordinatedWriteError,
@@ -481,7 +506,7 @@ class Filenode(unittest.TestCase, testutil.ShouldFailMixin):
                                           _backoff_stopper))
             d.addCallback(lambda res: n.download_best_version())
             d.addCallback(lambda res: self.failUnlessEqual(res, "line1line2"))
-            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2))
+            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 2, "stop"))
 
             def _reset_ucw_error_modifier(res):
                 calls[:] = []
@@ -492,7 +517,7 @@ class Filenode(unittest.TestCase, testutil.ShouldFailMixin):
             d.addCallback(lambda res: n.download_best_version())
             d.addCallback(lambda res: self.failUnlessEqual(res,
                                                            "line1line2line3"))
-            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 3))
+            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 3, "pause"))
 
             d.addCallback(lambda res:
                           self.shouldFail(UncoordinatedWriteError,
@@ -502,7 +527,7 @@ class Filenode(unittest.TestCase, testutil.ShouldFailMixin):
             d.addCallback(lambda res: n.download_best_version())
             d.addCallback(lambda res: self.failUnlessEqual(res,
                                                            "line1line2line3"))
-            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 3))
+            d.addCallback(lambda res: self.failUnlessCurrentSeqnumIs(n, 3, "giveup"))
 
             return d
         d.addCallback(_created)