diff --git a/py/api/preview_store.py b/py/api/preview_store.py index f2ebf49c..e3709eac 100644 --- a/py/api/preview_store.py +++ b/py/api/preview_store.py @@ -263,22 +263,40 @@ class _Probe: self._checking = None def check(self, path, expected_id): - """``folder_state`` within IO_BUDGET; raises TimeoutError. A check - already running (the probe's, or a stuck one) is joined, never run - twice at once.""" + """``folder_state`` within IO_BUDGET; raises TimeoutError. + + The answer comes from a check that started after this call: one + already running may have looked before what the caller just did, so + it is waited for (never run twice at once; a stuck one times out here) + and followed by a new one.""" + key = (path, expected_id) with self._lock: - pending = self._checking - if pending is None or pending[0] != (path, expected_id): - pending = self._start_check(path, expected_id) - key, done, result = pending - if not done.wait(IO_BUDGET): - raise TimeoutError("the thumbnail folder did not answer in time") + earlier = self._running(key) + pending = None if earlier else self._start_check(path, expected_id) + if earlier: + self._wait(earlier) + with self._lock: + # One another caller started meanwhile is new enough to join. + pending = self._running(key) or self._start_check(path, expected_id) + self._wait(pending) with self._lock: if self._checking is pending: self._checking = None - if "error" in result: - raise result["error"] - return result["value"] + if "error" in pending[2]: + raise pending[2]["error"] + return pending[2]["value"] + + def _running(self, key): + """The check of *key* still running (lock held), else None.""" + pending = self._checking + if pending is None or pending[0] != key or pending[1].is_set(): + return None + return pending + + @staticmethod + def _wait(pending): + if not pending[1].wait(IO_BUDGET): + raise TimeoutError("the thumbnail folder did not answer in time") def _start_check(self, path, expected_id): """Start one check on its own thread (lock held).""" diff --git a/tests/browser/workspace-thumbnail-location.spec.mjs b/tests/browser/workspace-thumbnail-location.spec.mjs index 9c0099e7..06a94d18 100644 --- a/tests/browser/workspace-thumbnail-location.spec.mjs +++ b/tests/browser/workspace-thumbnail-location.spec.mjs @@ -150,7 +150,8 @@ test('another folder’s marker offers Use this folder', async ({ page, request test('Overview shows the notice and Change location opens the row', async ({ page, request }) => { await expect(await request.post('/__test__/thumbnail-location', { data: { mode: 'folder' } })).toBeOK(); - await request.post('/__test__/thumbnail-folder/remove'); + const removed = await request.post('/__test__/thumbnail-folder/remove'); + expect((await removed.json()).available).toBe(false); await page.goto(`${workspace}#/system/overview`); const notice = page.locator('#unavailable-notice'); await expect(notice.locator('h2')).toHaveText('The thumbnail folder is unavailable'); diff --git a/tests/test_preview_probe.py b/tests/test_preview_probe.py new file mode 100644 index 00000000..496a8165 --- /dev/null +++ b/tests/test_preview_probe.py @@ -0,0 +1,93 @@ +"""The thumbnail folder check answers for the folder as it is now (#320). + +A check that started before the caller asked can miss what just happened: #592 +hit it in the merge queue, where a folder deleted right after +``configure`` woke the probe stayed "available" until the next probe. +""" + +import sys +import threading +import unittest +from pathlib import Path +from unittest.mock import patch + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + +from py.api import preview_location, preview_store # noqa: E402 + +WAIT = 5 +HERE = (True, None, {"id": "x"}) +GONE = (False, "missing", None) +KEY = ("/thumbs", "x") + + +class TestProbeCheck(unittest.TestCase): + def setUp(self): + self.probe = preview_store._Probe() + + def test_a_finished_check_nobody_collected_is_not_the_answer(self): + with patch.object(preview_location, "folder_state", return_value=HERE): + with self.probe._lock: + _key, done, _result = self.probe._start_check(*KEY) + self.assertTrue(done.wait(WAIT)) + with patch.object(preview_location, "folder_state", return_value=GONE): + self.assertEqual(self.probe.check(*KEY), GONE) + + def test_a_check_already_running_is_followed_by_a_new_one(self): + started = threading.Event() + release = threading.Event() + calls = [] + + def state(_path, _expected_id): + calls.append(threading.current_thread().name) + if len(calls) == 1: + started.set() + self.assertTrue(release.wait(WAIT)) + return HERE + return GONE + + answers = [] + with patch.object(preview_location, "folder_state", state): + with self.probe._lock: + self.probe._start_check(*KEY) + self.assertTrue(started.wait(WAIT)) + caller = threading.Thread( + target=lambda: answers.append(self.probe.check(*KEY)) + ) + caller.start() + release.set() + caller.join(WAIT) + self.assertFalse(caller.is_alive()) + self.assertEqual(answers, [GONE]) + self.assertEqual(len(calls), 2) # one after the other, never two at once + + def test_a_stuck_check_is_never_doubled(self): + release = threading.Event() + self.addCleanup(release.set) + calls = [] + + def state(_path, _expected_id): + calls.append(1) + release.wait(WAIT) + return HERE + + with ( + patch.object(preview_location, "folder_state", state), + patch.object(preview_store, "IO_BUDGET", 0.05), + ): + with self.probe._lock: + self.probe._start_check(*KEY) + with self.assertRaises(TimeoutError): + self.probe.check(*KEY) + self.assertEqual(len(calls), 1) + + def test_an_error_from_the_new_check_reaches_the_caller(self): + with patch.object( + preview_location, "folder_state", side_effect=OSError("share gone") + ): + with self.assertRaises(OSError): + self.probe.check(*KEY) + + +if __name__ == "__main__": + unittest.main()