martin/sharepoint-browser
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix: task 10 + review fix #1 — thread safety and sleep bug

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Martin Tranberg
2026-04-12 10:36:29 +02:00
parent 0dfef3e611
commit 7fd69a9c3f
2 changed files with 101 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

41
sharepoint_browser.py
View File

@@ -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__":

75
tests/test_review_fixes.py
View File

@@ -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)