diff --git a/PortMaster/pylibs/theme_server/serve.py b/PortMaster/pylibs/theme_server/serve.py index 3e12c71a..6ac81a70 100644 --- a/PortMaster/pylibs/theme_server/serve.py +++ b/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() ################################################################################ diff --git a/PortMaster/pylibs/theme_server/worker.py b/PortMaster/pylibs/theme_server/worker.py index 58b1cb34..bbe0e659 100644 --- a/PortMaster/pylibs/theme_server/worker.py +++ b/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) diff --git a/tests/test_theme_server.py b/tests/test_theme_server.py index 16097284..538265ac 100644 --- a/tests/test_theme_server.py +++ b/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 diff --git a/tests/test_theme_server_http.py b/tests/test_theme_server_http.py index ad8cc6dd..9d549f58 100644 --- a/tests/test_theme_server_http.py +++ b/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()