Theme server: stopping it while its first renderer starts leaves noth…

PortMaster/pylibs/theme_server/serve.py

@@ -369,6 +369,8 @@ def __init__(self, themes, ports_config, work_dir, max_workers=MAX_WORKERS, use_
         self.counter = 0
         ## Screen sizes whose renderer is starting, for /api/status.
         self.starting = set()
+        ## Set by stop(): no renderer starts after it.
+        self.stopping = False
 
     def status(self):
         """What the renderers are doing, so the page can say: {'starting': [resolution],
@@ -401,6 +403,9 @@ def _worker(self, resolution, theme_dir, scheme, lang, loaded):
 
         ## One start at a time: the page asks for every screen at once.
         with self.start_lock:
+            if self.stopping:
+                raise WorkerError("the server is stopping")
+
             with self.lock:
                 worker = self.workers.get(resolution)
                 if worker is not None and worker.alive():
@@ -470,14 +475,24 @@ def warm(self, theme_id="default_theme", scheme=None, resolution="640x480", lang
         theme_dir, scheme, loaded = self._wanted(theme_id, scheme, lang)
         self._worker(resolution, theme_dir, scheme, lang, loaded)
 
-    def stop(self):
-        with self.lock:
-            workers, self.workers = list(self.workers.values()), {}
-        for worker in workers:
-            worker.stop()
-        if self.zygote:
-            self.zygote.stop()
-            self.zygote = None
+    def stop(self, start_timeout=10):
+        """Stops the renderers and the zygote: also one still starting (the first one, warm(),
+        can still be starting when PortMaster stops the server), which would otherwise go on
+        into the work folder after it's removed. Waits up to start_timeout seconds for that
+        start: PortMaster kills the server after 15."""
+        self.stopping = True
+        started = self.start_lock.acquire(timeout=start_timeout)
+        try:
+            with self.lock:
+                workers, self.workers = list(self.workers.values()), {}
+            for worker in workers:
+                worker.stop()
+            if self.zygote:
+                self.zygote.stop()
+                self.zygote = None
+        finally:
+            if started:
+                self.start_lock.release()
 
 
 ################################################################################

PortMaster/pylibs/theme_server/worker.py

@@ -102,7 +102,11 @@ def prepare_tools_dir(work, ports_config, theme_dir, scheme):
     if tools.exists():
         shutil.rmtree(str(tools))
 
-    config.mkdir(parents=True)
+    ## One level at a time, never parents=True: if serve.py has already removed its
+    ## folder (stopped while this one was starting), this fails instead of making it again.
+    tools.mkdir()
+    (tools / "PortMaster").mkdir()
+    config.mkdir()
     (tools / "PortMaster" / "themes").mkdir()
     (tools / "PortMaster" / "libs").mkdir()
     (work / "ports").mkdir(exist_ok=True)
@@ -293,7 +297,8 @@ def zygote(options):
 
     control = socket.socket(fileno=options.control_fd)
     shared = Path(options.work)
-    shared.mkdir(parents=True, exist_ok=True)
+    ## In serve.py's folder, which is gone if it stopped while this was starting.
+    shared.mkdir(exist_ok=True)
     ## The themes (linked as renderers load them) and the config pugwash reads at start;
     ## each renderer's HarbourMaster has its own copy of the ports.
     prepare_tools_dir(shared, Path(options.ports_config), PM_DIR / "pylibs" / "default_theme", None)
@@ -341,7 +346,7 @@ def zygote(options):
                 control.close()
                 signal.signal(signal.SIGCHLD, signal.SIG_DFL)
                 work = Path(spawn['work'])
-                work.mkdir(parents=True, exist_ok=True)
+                work.mkdir(exist_ok=True)
                 log = open(str(work.with_suffix(".log")), "w")
                 os.dup2(log.fileno(), 2)
                 os.dup2(log.fileno(), 1)
@@ -391,7 +396,7 @@ def main(argv=None):
 
     width, height = parse_resolution(options.resolution)
     work = Path(options.work)
-    work.mkdir(parents=True, exist_ok=True)
+    work.mkdir(exist_ok=True)
     prepare_tools_dir(work, Path(options.ports_config), PM_DIR / "pylibs" / "default_theme", None)
     renderer_env(width, height, options.lang)
     use_tools_dir(work)

tests/test_theme_server.py

@@ -43,7 +43,9 @@ def test_prepare_tools_dir(tmp_path):
     theme.mkdir()
     (theme / "theme.json").write_text("{}")
 
+    ## (Its callers make the work folder: it never makes the folders above tools.)
     work = tmp_path / "work"
+    work.mkdir()
     assert worker.prepare_tools_dir(work, cache_config, theme, "Dark") == "My Theme"
     config = work / "tools" / "PortMaster" / "config"
     ## The screenshot folders are linked, the files copied: on a device this is PortMaster's
@@ -553,6 +555,32 @@ def test_managed_server(tmp_path):
             proc.kill()
 
 
+@pytest.mark.sdl
+@pytest.mark.own_display
+@pytest.mark.parametrize("delay", [0, 0.05, 0.2, 0.5, 1.0])
+def test_managed_server_stopped_while_starting(tmp_path, delay):
+    ## Stopped (B, or PortMaster quitting) while its first renderer is still starting:
+    ## that start mustn't carry on into the folder the server has already removed.
+    import time
+
+    proc = start_managed(tmp_path)
+    try:
+        ready = json.loads(proc.stdout.readline() or "{}")
+        if not ready.get('ok'):
+            pytest.skip(f"the renderer can't start here: {ready.get('error')}")
+
+        time.sleep(delay)
+        proc.stdin.close()
+        assert proc.wait(timeout=60) == 0
+        ## A renderer left behind would still be writing there.
+        time.sleep(0.5)
+        assert [path for path in (tmp_path / "tmp").iterdir() if path.name.startswith("pm-theme-server-")] == []
+
+    finally:
+        if proc.poll() is None:
+            proc.kill()
+
+
 @pytest.mark.sdl
 def test_theme_editor_scene(gui, hm_dirs, monkeypatch):
     import time

tests/test_theme_server_http.py

@@ -574,3 +574,88 @@ def test_copy_theme_with_changes(server, themes_dir):
     status, answer = post(server[0], "/api/theme_copy", {'theme': "default_theme", 'name': "Mine", 'text': '{"#info": {"name": "x"}, "main_menu": {}}'})
     assert status == 200
     assert json.loads((themes_dir / "Mine" / "theme.json").read_text()) == {"#info": {"name": "Mine"}, "main_menu": {}}
+
+
+################################################################################
+## Stopping while a renderer starts
+class SlowZygote:
+    """A zygote that takes `delay` seconds to start, and says when it was stopped."""
+    delay = 0.5
+    made = []
+
+    def __init__(self, ports_config, work):
+        time.sleep(self.delay)
+        self.stopped = False
+        SlowZygote.made.append(self)
+
+    def alive(self):
+        return not self.stopped
+
+    def stop(self):
+        self.stopped = True
+
+
+class FakeWorker:
+    made = []
+
+    def __init__(self, *args, **kwargs):
+        self.stopped = False
+        FakeWorker.made.append(self)
+
+    def alive(self):
+        return not self.stopped
+
+    def stop(self):
+        self.stopped = True
+
+
+@pytest.fixture
+def slow_pool(tmp_path, themes_dir, monkeypatch):
+    SlowZygote.made, FakeWorker.made = [], []
+    monkeypatch.setattr(serve, "Zygote", SlowZygote)
+    monkeypatch.setattr(serve, "Worker", FakeWorker)
+    return serve.RenderPool(serve.Themes(themes_dir), tmp_path / "config", tmp_path / "work")
+
+
+def test_stop_waits_for_a_start(slow_pool):
+    ## warm() is still starting the first renderer when PortMaster stops the server: stop()
+    ## waits for it and stops what it started, or it would carry on in the removed folder.
+    warm = threading.Thread(target=slow_pool.warm)
+    warm.start()
+    time.sleep(0.1)
+
+    slow_pool.stop()
+    warm.join()
+
+    assert len(SlowZygote.made) == 1 and SlowZygote.made[0].stopped
+    assert len(FakeWorker.made) == 1 and FakeWorker.made[0].stopped
+    assert slow_pool.workers == {} and slow_pool.zygote is None
+
+
+def test_nothing_starts_after_stop(slow_pool):
+    slow_pool.stop()
+    with pytest.raises(serve.WorkerError):
+        slow_pool.warm()
+    assert SlowZygote.made == [] and FakeWorker.made == []
+
+
+def test_stop_gives_up_waiting(slow_pool, monkeypatch):
+    ## A start that never ends: stop() still returns (PortMaster kills the server at 15 s).
+    monkeypatch.setattr(SlowZygote, "delay", 3)
+    warm = threading.Thread(target=slow_pool.warm, daemon=True)
+    warm.start()
+    time.sleep(0.1)
+
+    began = time.monotonic()
+    slow_pool.stop(start_timeout=0.3)
+    assert time.monotonic() - began < 2
+
+
+def test_renderer_folders_need_the_servers(tmp_path):
+    ## A renderer starting after serve.py removed its folder fails, rather than making it again.
+    from theme_server import worker
+
+    gone = tmp_path / "pm-theme-server-x" / "zygote"
+    with pytest.raises(FileNotFoundError):
+        worker.prepare_tools_dir(gone, tmp_path / "config", serve.DEFAULT_THEME_DIR, None)
+    assert not (tmp_path / "pm-theme-server-x").exists()