From 7fd69a9c3febefb40f5675d3cc62dee671616cda Mon Sep 17 00:00:00 2001 From: Martin Tranberg Date: Sun, 12 Apr 2026 10:36:29 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20task=2010=20+=20review=20fix=20#1=20?= =?UTF-8?q?=E2=80=94=20thread=20safety=20and=20sleep=20bug?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix #1: Don't sleep after the final exhausted retry attempt in _graph_request(). The sleep on the last iteration was pure overhead — the loop exits immediately after, so the caller just waited an extra backoff period for no reason. Guard with attempt < _MAX_RETRIES - 1. Task 10: Add threading.Lock (_edits_lock) for all compound operations on active_edits, which is accessed from both the UI thread and background edit threads: - __init__: declare self._edits_lock = threading.Lock() - open_file: snapshot already_editing and at_limit under the lock, then release before showing blocking UI dialogs - process_file: wrap initial dict assignment, waiting=True, the in-check+waiting=False, and in-check+del under the lock - on_done_editing_clicked: lock the items() snapshot used to build waiting_files, preventing iteration over a dict being mutated Add 8 new unit tests (1 for Fix #1 sleep count, 7 for Task 10 lock). Co-Authored-By: Claude Sonnet 4.6 --- sharepoint_browser.py | 41 +++++++++++++-------- tests/test_review_fixes.py | 75 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 15 deletions(-) diff --git a/sharepoint_browser.py b/sharepoint_browser.py index fa63367..831bff1 100644 --- a/sharepoint_browser.py +++ b/sharepoint_browser.py @@ -92,8 +92,9 @@ def _graph_request(method, url, **kwargs): res = requests.request(method, url, **kwargs) if res.status_code not in _RETRY_STATUSES: return res - retry_after = int(res.headers.get("Retry-After", 2 ** attempt)) - time.sleep(min(retry_after, 60)) + if attempt < _MAX_RETRIES - 1: # Don't sleep after the final attempt + retry_after = int(res.headers.get("Retry-After", 2 ** attempt)) + time.sleep(min(retry_after, 60)) return res # Return last response after exhausting retries settings = load_settings() @@ -697,6 +698,7 @@ class SharePointApp(wx.Frame): self.tree_root = None self.is_navigating_back = False self.active_edits = {} # item_id -> { "name": name, "event": Event, "waiting": bool } + self._edits_lock = threading.Lock() self.favorites = settings.get("favorites", []) self.fav_visible = settings.get("fav_visible", True) self.sort_col = 0 # Default (Navn) @@ -1404,7 +1406,8 @@ class SharePointApp(wx.Frame): wx.CallAfter(_do) def on_done_editing_clicked(self, event): - waiting_files = [fid for fid, d in self.active_edits.items() if d.get("waiting")] + with self._edits_lock: + waiting_files = [fid for fid, d in self.active_edits.items() if d.get("waiting")] if not waiting_files: return @@ -2253,12 +2256,16 @@ class SharePointApp(wx.Frame): item_id = item['id'] file_name = item['name'] drive_id = item['drive_id'] - - if item_id in self.active_edits: + + with self._edits_lock: + already_editing = item_id in self.active_edits + at_limit = len(self.active_edits) >= 10 + + # UI dialogs are called outside the lock to avoid holding it during blocking calls + if already_editing: self.show_info(f"'{file_name}' er allerede ved at blive redigeret.", wx.ICON_INFORMATION) return - - if len(self.active_edits) >= 10: + if at_limit: wx.MessageBox("Du kan kun have 10 filer åbne til redigering ad gangen.", "Maksimum grænse nået", wx.OK | wx.ICON_WARNING) return @@ -2295,7 +2302,8 @@ class SharePointApp(wx.Frame): if not self.ensure_valid_token(): return edit_event = threading.Event() - self.active_edits[item_id] = {"name": file_name, "event": edit_event, "waiting": False} + with self._edits_lock: + self.active_edits[item_id] = {"name": file_name, "event": edit_event, "waiting": False} self.update_edit_ui() try: @@ -2382,13 +2390,15 @@ class SharePointApp(wx.Frame): else: self.set_status(self.get_txt("msg_waiting_for_file", name=file_name)) edit_event.clear() - self.active_edits[item_id]["waiting"] = True + with self._edits_lock: + self.active_edits[item_id]["waiting"] = True self.update_edit_ui() - + edit_event.wait() - - if item_id in self.active_edits: - self.active_edits[item_id]["waiting"] = False + + with self._edits_lock: + if item_id in self.active_edits: + self.active_edits[item_id]["waiting"] = False self.update_edit_ui() # 4. Tjek om noget er ændret @@ -2448,8 +2458,9 @@ class SharePointApp(wx.Frame): logger.info(f"Rydder op: Kalder discardCheckout for {file_name}...") _graph_request("POST", f"{base_url}/discardCheckout", headers=self.headers, timeout=30) - if item_id in self.active_edits: - del self.active_edits[item_id] + with self._edits_lock: + if item_id in self.active_edits: + del self.active_edits[item_id] self.update_edit_ui() if __name__ == "__main__": diff --git a/tests/test_review_fixes.py b/tests/test_review_fixes.py index ecd4e37..70b6381 100644 --- a/tests/test_review_fixes.py +++ b/tests/test_review_fixes.py @@ -421,6 +421,81 @@ class TestGraphRequest(unittest.TestCase): "_MAX_RETRIES constant not found in sharepoint_browser" ) + def test_no_sleep_after_final_retry(self): + """Fix #1: sleep() is NOT called after the last exhausted attempt.""" + resp_429 = MagicMock(status_code=429, headers={"Retry-After": "0"}) + responses = [resp_429] * sb._MAX_RETRIES + with patch("sharepoint_browser.requests.request", side_effect=responses): + with patch("sharepoint_browser.time.sleep") as mock_sleep: + sb._graph_request("GET", "https://example.com/", headers={}) + # sleep should be called for the first N-1 failures, NOT the last one + self.assertEqual( + mock_sleep.call_count, + sb._MAX_RETRIES - 1, + f"sleep() called {mock_sleep.call_count} times; expected {sb._MAX_RETRIES - 1} " + f"(no sleep after the final failed attempt)" + ) + + +# --------------------------------------------------------------------------- +# Task 10: threading.Lock for active_edits compound operations +# --------------------------------------------------------------------------- + +class TestActiveEditsLock(unittest.TestCase): + """Verify that _edits_lock exists and guards all compound active_edits operations.""" + + def test_edits_lock_declared_in_init(self): + """Task 10: SharePointApp.__init__ creates self._edits_lock.""" + source = inspect.getsource(sb.SharePointApp.__init__) + self.assertIn("_edits_lock", source, + "__init__ does not declare _edits_lock") + + def test_edits_lock_is_threading_lock(self): + """Task 10: _edits_lock initialisation uses threading.Lock().""" + source = inspect.getsource(sb.SharePointApp.__init__) + self.assertIn("threading.Lock()", source, + "__init__ does not initialise _edits_lock with threading.Lock()") + + def test_open_file_uses_lock(self): + """Task 10: open_file() acquires _edits_lock before checking active_edits.""" + source = inspect.getsource(sb.SharePointApp.open_file) + self.assertIn("_edits_lock", source, + "open_file does not use _edits_lock") + + def test_process_file_uses_lock(self): + """Task 10: process_file() acquires _edits_lock when writing active_edits.""" + source = inspect.getsource(sb.SharePointApp.process_file) + self.assertIn("_edits_lock", source, + "process_file does not use _edits_lock") + + def test_process_file_lock_on_initial_assign(self): + """Task 10: active_edits[item_id] = ... assignment is inside a lock block.""" + source = inspect.getsource(sb.SharePointApp.process_file) + # Check lock wraps the initial dict assignment + lock_idx = source.find("_edits_lock") + assign_idx = source.find('self.active_edits[item_id] = ') + self.assertGreater(assign_idx, 0, + "active_edits assignment not found in process_file") + # The lock must appear before the assignment + self.assertLess(lock_idx, assign_idx, + "_edits_lock must appear before the active_edits assignment in process_file") + + def test_process_file_lock_on_delete(self): + """Task 10: del active_edits[item_id] is inside a lock block in process_file.""" + source = inspect.getsource(sb.SharePointApp.process_file) + self.assertIn("del self.active_edits[item_id]", source, + "del active_edits[item_id] not found in process_file") + # Count lock usages — there should be at least 2 + lock_count = source.count("_edits_lock") + self.assertGreaterEqual(lock_count, 2, + f"Expected at least 2 uses of _edits_lock in process_file, found {lock_count}") + + def test_on_done_editing_uses_lock(self): + """Task 10: on_done_editing_clicked acquires lock for active_edits iteration.""" + source = inspect.getsource(sb.SharePointApp.on_done_editing_clicked) + self.assertIn("_edits_lock", source, + "on_done_editing_clicked does not use _edits_lock") + if __name__ == "__main__": unittest.main(verbosity=2)