write node.url and portnum files atomically, to fix race in test_runner

Previously, test_runner sometimes fails because the _node_has_started()
poller fires after the portnum file has been opened, but before it has
actually been filled, allowing the test process to observe an empty file,
which flunks the test.

This adds a new fileutil.write_atomically() function (using the usual
write-to-.tmp-then-rename approach), and uses it for both node.url and
client.port . These files are written a bit before the node is really up and
running, but they're late enough for test_runner's purposes, which is to know
when it's safe to read client.port and use 'tahoe restart' (and therefore
SIGINT) to restart the node.

The current node/client code doesn't offer any better "are you really done
with startup" indicator.. the ideal approach would be to either watch the
logfile, or connect to its flogport, but both are a hassle. Changing the node
to write out a new "all done" file would be intrusive for regular
operations.

diff --git a/src/allmydata/node.py b/src/allmydata/node.py
index 458c57e1c237d6ee0149f6445f4c45985e40dd0d..aedaf8429f3b38b72cab206331cd42e160f94627 100644 (file)
--- a/src/allmydata/node.py
+++ b/src/allmydata/node.py
@@ -360,7 +360,7 @@ class Node(service.MultiService):
         portnum = l.getPortnum()
         # record which port we're listening on, so we can grab the same one
         # next time
-        open(self._portnumfile, "w").write("%d\n" % portnum)
+        fileutil.write_atomically(self._portnumfile, "%d\n" % portnum, mode="")
 
         base_location = ",".join([ "%s:%d" % (addr, portnum)
                                    for addr in local_addresses ])

diff --git a/src/allmydata/test/test_runner.py b/src/allmydata/test/test_runner.py
index 6dd8b6341f9d7d5a29b35ee947e909e4bc97032f..4e9f683d79bc7dbffdf0ba3122236327508b69ba 100644 (file)
--- a/src/allmydata/test/test_runner.py
+++ b/src/allmydata/test/test_runner.py
@@ -588,6 +588,7 @@ class RunNode(common_util.SignalMixin, unittest.TestCase, pollmixin.PollMixin,
         d.addCallback(_cb2)
 
         def _node_has_started():
+            # this depends upon both files being created atomically
             return os.path.exists(NODE_URL_FILE) and os.path.exists(PORTNUM_FILE)
         d.addCallback(lambda res: self.poll(_node_has_started))
 
@@ -627,7 +628,9 @@ class RunNode(common_util.SignalMixin, unittest.TestCase, pollmixin.PollMixin,
         # 'tahoe stop' command takes a while.
         def _stop(res):
             fileutil.write(HOTLINE_FILE, "")
-            self.failUnless(os.path.exists(TWISTD_PID_FILE), (TWISTD_PID_FILE, os.listdir(os.path.dirname(TWISTD_PID_FILE))))
+            self.failUnless(os.path.exists(TWISTD_PID_FILE),
+                            (TWISTD_PID_FILE,
+                             os.listdir(os.path.dirname(TWISTD_PID_FILE))))
             return self.run_bintahoe(["--quiet", "stop", c1])
         d.addCallback(_stop)
 

diff --git a/src/allmydata/test/test_util.py b/src/allmydata/test/test_util.py
index 54ef25518d375ff877e3d41011cfb4d2b7503c65..575a7a493d2e3b19a06f13d3723b7e6ac58337e5 100644 (file)
--- a/src/allmydata/test/test_util.py
+++ b/src/allmydata/test/test_util.py
@@ -420,6 +420,15 @@ class FileUtil(unittest.TestCase):
         fileutil.rm_dir(basedir)
         fileutil.remove_if_possible(fn) # should survive errors
 
+    def test_write_atomically(self):
+        basedir = "util/FileUtil/test_write_atomically"
+        fileutil.make_dirs(basedir)
+        fn = os.path.join(basedir, "here")
+        fileutil.write_atomically(fn, "one")
+        self.failUnlessEqual(fileutil.read(fn), "one")
+        fileutil.write_atomically(fn, "two", mode="") # non-binary
+        self.failUnlessEqual(fileutil.read(fn), "two")
+
     def test_open_or_create(self):
         basedir = "util/FileUtil/test_open_or_create"
         fileutil.make_dirs(basedir)

diff --git a/src/allmydata/util/fileutil.py b/src/allmydata/util/fileutil.py
index bfcffd8f9932df2b665e32f45357b6166245e62b..d6235509848cf53fc3928da9044b57a6175476dd 100644 (file)
--- a/src/allmydata/util/fileutil.py
+++ b/src/allmydata/util/fileutil.py
@@ -247,6 +247,12 @@ def move_into_place(source, dest):
         remove_if_possible(dest)
     os.rename(source, dest)
 
+def write_atomically(target, contents, mode="b"):
+    f = open(target+".tmp", "w"+mode)
+    f.write(contents)
+    f.close()
+    move_into_place(target+".tmp", target)
+
 def write(path, data):
     wf = open(path, "wb")
     try:

diff --git a/src/allmydata/webish.py b/src/allmydata/webish.py
index a8e0bff8e252cc917cc4d82630038c279d46c0eb..813856cb02c53bdb81e06d3953281f20d803aae9 100644 (file)
--- a/src/allmydata/webish.py
+++ b/src/allmydata/webish.py
@@ -163,7 +163,8 @@ class WebishServer(service.MultiService):
         if nodeurl_path:
             def _write_nodeurl_file(ign):
                 # this file will be created with default permissions
-                fileutil.write(nodeurl_path, self.getURL() + "\n")
+                line = self.getURL() + "\n"
+                fileutil.write_atomically(nodeurl_path, line, mode="")
             self._started.addCallback(_write_nodeurl_file)
 
     def getURL(self):