back to context-manager, simplify

src/allmydata/scripts/tahoe_run.py

@@ -30,8 +30,8 @@ from allmydata.util.configutil import UnknownConfigError
 from allmydata.util.deferredutil import HookMixin
 from allmydata.util.pid import (
     check_pid_process,
-    cleanup_pidfile,
     ProcessInTheWay,
+    InvalidPidFile,
 )
 from allmydata.storage.crawler import (
     MigratePickleFileError,
@@ -237,8 +237,13 @@ def run(config, runApp=twistd.runApp):
         print("%s is not a recognizable node directory" % quoted_basedir, file=err)
         return 1
 
-    # we turn off Twisted's pid-file to use our own
+    twistd_args = [
-    twistd_args = ["--pidfile", None, "--nodaemon", "--rundir", basedir]
+        # turn off Twisted's pid-file to use our own
+        "--pidfile", None,
+        # ensure twistd machinery does not daemonize.
+        "--nodaemon",
+        "--rundir", basedir,
+    ]
     twistd_args.extend(config.twistd_args)
     twistd_args.append("DaemonizeTahoeNode") # point at our DaemonizeTahoeNodePlugin
 
@@ -254,9 +259,7 @@ def run(config, runApp=twistd.runApp):
         return 1
     twistd_config.loadedPlugins = {"DaemonizeTahoeNode": DaemonizeTahoeNodePlugin(nodetype, basedir)}
 
-    # before we try to run, check against our pidfile -- this will
+    # our own pid-style file contains PID and process creation time
-    # raise an exception if there appears to be a running process "in
-    # the way"
     pidfile = FilePath(get_pidfile(config['basedir']))
     try:
         check_pid_process(pidfile)

src/allmydata/test/cli/test_run.py

@@ -159,7 +159,7 @@ class RunTests(SyncTestCase):
         """
         basedir = FilePath(self.mktemp()).asTextMode()
         basedir.makedirs()
-        basedir.child(u"twistd.pid").setContent(b"foo")
+        basedir.child(u"running.process").setContent(b"foo")
         basedir.child(u"tahoe-client.tac").setContent(b"")
 
         config = RunOptions()
@@ -168,17 +168,19 @@ class RunTests(SyncTestCase):
         config['basedir'] = basedir.path
         config.twistd_args = []
 
-        runs = []
+        class DummyRunner:
-        result_code = run(config, runApp=runs.append)
+            runs = []
+            _exitSignal = None
+
+            def run(self):
+                self.runs.append(True)
+
+        result_code = run(config, runner=DummyRunner())
         self.assertThat(
             config.stderr.getvalue(),
             Contains("found invalid PID file in"),
         )
         self.assertThat(
-            runs,
+            DummyRunner.runs,
-            HasLength(1),
+            Equals([])
-        )
-        self.assertThat(
-            result_code,
-            Equals(0),
         )

src/allmydata/util/pid.py

@@ -1,5 +1,8 @@
 import os
 import psutil
+from contextlib import (
+    contextmanager,
+)
 
 
 class ProcessInTheWay(Exception):
@@ -8,6 +11,13 @@ class ProcessInTheWay(Exception):
     """
 
 
+class InvalidPidFile(Exception):
+    """
+    our pidfile isn't well-formed
+    """
+
+
+@contextmanager
 def check_pid_process(pidfile, find_process=None):
     """
     If another instance appears to be running already, raise an
@@ -26,9 +36,16 @@ def check_pid_process(pidfile, find_process=None):
     if pidfile.exists():
         with pidfile.open("r") as f:
             content = f.read().decode("utf8").strip()
-        pid, starttime = content.split()
+        try:
-        pid = int(pid)
+            pid, starttime = content.split()
-        starttime = float(starttime)
+            pid = int(pid)
+            starttime = float(starttime)
+        except ValueError:
+            raise InvalidPidFile(
+                "found invalid PID file in {}".format(
+                    pidfile
+                )
+            )
         try:
             # if any other process is running at that PID, let the
             # user decide if this is another magic-older
@@ -55,11 +72,7 @@ def check_pid_process(pidfile, find_process=None):
     with pidfile.open("w") as f:
         f.write("{} {}\n".format(pid, starttime).encode("utf8"))
 
-
+    yield  # setup completed, await cleanup
-def cleanup_pidfile(pidfile):
-    """
-    Safely remove the given pidfile
-    """
 
     try:
         pidfile.remove()