Fix thread-safety issues in concurrent access#171
Conversation
- drive.py: Copy cookies before iteration in _get_token_from_cookie() - drive.py: Copy dict and items list in get_children() to avoid mutation - session.py: Copy _data dict before JSON dump in _save_session_data() - cookie_jar.py: Snapshot cookies before iteration in load() and save() These fixes prevent "dictionary changed size during iteration" errors when multiple threads make concurrent HTTP requests that modify shared state (cookies, session data, drive node items). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdded a public PyiCloudCookieJar.copy() and changed cookie, drive and session persistence to operate on snapshots/copies before iterating or saving to avoid mutation/race issues; tests added for cookie-copy behaviour and Drive token extraction edge cases. No breaking API changes besides the new copy() method. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Pylint (4.0.4)pyicloud/cookie_jar.pypyicloud/services/drive.pyThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pyicloud/cookie_jar.py (2)
67-84: Consider removing redundant import.
LWPCookieJaris already imported at line 3, so the alias at line 72 is redundant. You could either use the existing import directly or create a module-level alias for clarity.🔎 Proposed refactor
# Copy cookies to avoid "dictionary changed size during iteration" # when concurrent HTTP responses modify the cookie jar try: cookies_snapshot = list(self) - # Create temp jar with snapshot for thread-safe save - from http.cookiejar import LWPCookieJar as TempJar - - temp_jar = TempJar(filename=resolved) + # Create temp jar with snapshot for thread-safe save + temp_jar = LWPCookieJar(filename=resolved) for cookie in cookies_snapshot: temp_jar.set_cookie(cookie) temp_jar.save( filename=resolved, ignore_discard=ignore_discard, ignore_expires=ignore_expires, ) except RuntimeError: # If we still hit a race, silently skip this save pass
82-84: Silent failure on RuntimeError prevents crash but loses data.Silently skipping the save on
RuntimeErroris a reasonable defensive choice to prevent crashes, but users won't be notified if cookies fail to persist. Consider logging a warning if this tradeoff becomes problematic in practice.🔎 Optional: Add logging for silent failures
Import logging at the top of the file if not already present, then:
except RuntimeError: # If we still hit a race, silently skip this save - pass + import logging + logging.getLogger(__name__).warning( + "Failed to save cookies due to concurrent modification; skipping save" + )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pyicloud/cookie_jar.py(2 hunks)pyicloud/services/drive.py(2 hunks)pyicloud/session.py(1 hunks)
🔇 Additional comments (4)
pyicloud/session.py (1)
104-105: Good defensive pattern for concurrent access.The shallow copy prevents "dictionary changed size during iteration" errors when
_update_session_data()runs concurrently. Since session data contains simple header strings at the top level, a shallow copy is adequate.pyicloud/cookie_jar.py (1)
41-50: Defensive snapshot pattern prevents iteration errors.The fallback to an empty list on
RuntimeErroris a reasonable tradeoff—it prevents crashes whilst accepting that FMIP cookies might not be cleared in rare race conditions.pyicloud/services/drive.py (2)
46-58: Robust cookie snapshot prevents concurrent modification errors.The fallback to an empty list on
RuntimeErroris well-handled—it leads to aTokenExceptionrather than attempting to extract a token from potentially corrupted state.
415-427: Effective defensive copies for concurrent access.The dict merging at line 419 and list copy at line 423 both prevent "dictionary changed size during iteration" errors. The shallow copy approach is consistent with the patterns in session.py and adequate for the current data structures.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@pyicloud/cookie_jar.py`:
- Around line 48-62: The current broad RuntimeError handler around building and
clearing cookies (the block that computes cookies_to_clear via self.copy() and
iterates calling self.clear(domain=cookie.domain, path=cookie.path,
name=cookie.name) for cookies matching _FMIP_AUTH_COOKIE_NAME) should be
tightened: catch RuntimeError as e, check that str(e) (or e.args[0]) contains
the specific "dictionary changed size during iteration" text before swallowing;
for that case log a warning including the exception and context (e.g., that FMIP
auth cookie clearing was skipped) using the module logger, and re-raise any
other RuntimeError so unrelated errors are not hidden.
- Around line 74-86: The except RuntimeError block that silently drops save
failures in the cookie persistence logic (around PyiCloudCookieJar.copy() and
LWPCookieJar.save) should surface the failure: replace the silent pass with
logging a warning (include exception details and the filename/resolved variable)
and either re-raise the RuntimeError or return a boolean/raise a custom
exception so callers can react; update the except block to capture the exception
(e.g., as exc), call the project's logger (or the module logger) to log a
warning/error with context including resolved, ignore_discard/ignore_expires,
and then re-raise or propagate the error instead of swallowing it.
In `@pyicloud/services/drive.py`:
- Around line 46-48: The loop claims to copy cookies but iterates directly over
self.session.cookies, so make a snapshot before iterating to avoid
concurrent-modification errors: replace the iteration over self.session.cookies
with iteration over a shallow copy (e.g., cookies_snapshot =
list(self.session.cookies) and then for cookie in cookies_snapshot) so the loop
in the Drive service uses the snapshot instead of the live cookie jar.
🧹 Nitpick comments (2)
tests/test_cookie_jar.py (1)
120-120: Type annotation is inaccurate.
jar.copy()returnsPyiCloudCookieJar(as defined inpyicloud/cookie_jar.py:25), notRequestsCookieJar. While this works at runtime due to inheritance, the annotation could be misleading.✏️ Suggested fix
- jar2: RequestsCookieJar = jar.copy() + jar2: PyiCloudCookieJar = jar.copy()And update the import:
-from requests.cookies import RequestsCookieJar +from pyicloud.cookie_jar import _FMIP_AUTH_COOKIE_NAME, PyiCloudCookieJartests/services/test_drive.py (1)
729-746: Test name overstates what is being verified.This test doesn't actually validate concurrent modification safety. It simply passes a list and checks that iteration succeeds, which would pass even without any thread-safety fix. A true concurrent modification test would require multiple threads modifying the cookie jar whilst another iterates.
Consider renaming to
test_get_token_from_cookie_iterates_cookiesor adding an actual concurrency test usingthreading.
|
@olenzma - many thanks for this update |
3 participants
Summary
This PR fixes "dictionary changed size during iteration" errors that occur when multiple threads make concurrent HTTP requests that modify shared state.
Changes
drive.py
_get_token_from_cookie(): Snapshot cookies before iteration to avoid race with concurrent HTTP responses adding cookiesget_children(): Copy dict before update and copy items list before iteration to prevent mutation during concurrent accesssession.py
_save_session_data(): Copy_datadict before JSON dump to avoid mutation during concurrent_update_session_data()callscookie_jar.py
load(): Snapshot cookies before iteration when clearing FMIP cookiessave(): Create temporary jar with cookie snapshot for thread-safe save, avoiding iteration over live cookie jarBackground
These issues manifest during parallel iCloud Drive downloads where multiple threads trigger HTTP requests simultaneously. The HTTP responses modify cookies and session data while other threads iterate over these collections.
🤖 Generated with Claude Code