Split WebDAVSession into foreground data and background transfer sessions#40
Split WebDAVSession into foreground data and background transfer sessions#40tobihagemann wants to merge 1 commit into
Conversation
WalkthroughThis PR introduces a two-session architecture for WebDAV operations: a foreground Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/CryptomatorCloudAccess/WebDAV/WebDAVSession.swift`:
- Around line 203-208: The two URLSessionConfiguration instances create separate
HTTPCookieStorage objects causing cookies from metadata requests to not be
available to transfers; create a single shared HTTPCookieStorage instance (e.g.
let sharedCookieStorage = HTTPCookieStorage()) and assign that same instance to
both transferConfiguration.httpCookieStorage and
dataConfiguration.httpCookieStorage so transferSession and dataSession share
cookies; ensure any existing explicit nil for urlCredentialStorage remains
unchanged and reuse the sharedCookieStorage when constructing the sessions
(transferSession, dataSession).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db610bdd-0352-48ba-849e-a214779675da
📒 Files selected for processing (4)
Sources/CryptomatorCloudAccess/WebDAV/WebDAVClient.swiftSources/CryptomatorCloudAccess/WebDAV/WebDAVSession.swiftTests/CryptomatorCloudAccessTests/WebDAV/URLProtocolMock.swiftTests/CryptomatorCloudAccessTests/WebDAV/WebDAVClientTests.swift
| transferConfiguration.httpCookieStorage = HTTPCookieStorage() | ||
| transferConfiguration.urlCredentialStorage = nil | ||
| let transferSession = URLSession(configuration: transferConfiguration, delegate: delegate, delegateQueue: nil) | ||
| let dataConfiguration = URLSessionConfiguration.default | ||
| dataConfiguration.httpCookieStorage = HTTPCookieStorage() | ||
| dataConfiguration.urlCredentialStorage = nil |
There was a problem hiding this comment.
Sessions do not share cookie storage.
Each session creates its own HTTPCookieStorage() instance. Cookies received during metadata operations (dataSession) will not be sent with file transfers (transferSession), which may break WebDAV servers relying on session cookies for authentication.
Consider using a shared cookie storage instance:
Proposed fix to share cookie storage
static func withBackgroundSession(with delegate: WebDAVClientURLSessionDelegate, sessionIdentifier: String, sharedContainerIdentifier: String? = nil) -> WebDAVSession {
+ let sharedCookieStorage = HTTPCookieStorage()
let transferConfiguration = URLSessionConfiguration.background(withIdentifier: sessionIdentifier)
transferConfiguration.sharedContainerIdentifier = sharedContainerIdentifier
- transferConfiguration.httpCookieStorage = HTTPCookieStorage()
+ transferConfiguration.httpCookieStorage = sharedCookieStorage
transferConfiguration.urlCredentialStorage = nil
let transferSession = URLSession(configuration: transferConfiguration, delegate: delegate, delegateQueue: nil)
let dataConfiguration = URLSessionConfiguration.default
- dataConfiguration.httpCookieStorage = HTTPCookieStorage()
+ dataConfiguration.httpCookieStorage = sharedCookieStorage
dataConfiguration.urlCredentialStorage = nil
let dataSession = URLSession(configuration: dataConfiguration, delegate: delegate, delegateQueue: nil)
return WebDAVSession(dataSession: dataSession, transferSession: transferSession, delegate: delegate)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transferConfiguration.httpCookieStorage = HTTPCookieStorage() | |
| transferConfiguration.urlCredentialStorage = nil | |
| let transferSession = URLSession(configuration: transferConfiguration, delegate: delegate, delegateQueue: nil) | |
| let dataConfiguration = URLSessionConfiguration.default | |
| dataConfiguration.httpCookieStorage = HTTPCookieStorage() | |
| dataConfiguration.urlCredentialStorage = nil | |
| static func withBackgroundSession(with delegate: WebDAVClientURLSessionDelegate, sessionIdentifier: String, sharedContainerIdentifier: String? = nil) -> WebDAVSession { | |
| let sharedCookieStorage = HTTPCookieStorage() | |
| let transferConfiguration = URLSessionConfiguration.background(withIdentifier: sessionIdentifier) | |
| transferConfiguration.sharedContainerIdentifier = sharedContainerIdentifier | |
| transferConfiguration.httpCookieStorage = sharedCookieStorage | |
| transferConfiguration.urlCredentialStorage = nil | |
| let transferSession = URLSession(configuration: transferConfiguration, delegate: delegate, delegateQueue: nil) | |
| let dataConfiguration = URLSessionConfiguration.default | |
| dataConfiguration.httpCookieStorage = sharedCookieStorage | |
| dataConfiguration.urlCredentialStorage = nil | |
| let dataSession = URLSession(configuration: dataConfiguration, delegate: delegate, delegateQueue: nil) | |
| return WebDAVSession(dataSession: dataSession, transferSession: transferSession, delegate: delegate) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/CryptomatorCloudAccess/WebDAV/WebDAVSession.swift` around lines 203 -
208, The two URLSessionConfiguration instances create separate HTTPCookieStorage
objects causing cookies from metadata requests to not be available to transfers;
create a single shared HTTPCookieStorage instance (e.g. let sharedCookieStorage
= HTTPCookieStorage()) and assign that same instance to both
transferConfiguration.httpCookieStorage and dataConfiguration.httpCookieStorage
so transferSession and dataSession share cookies; ensure any existing explicit
nil for urlCredentialStorage remains unchanged and reuse the sharedCookieStorage
when constructing the sessions (transferSession, dataSession).
|
Closing this. I tested it and it doesn't fix the original problem, and I'm not confident the split is a net improvement either way. Not worth carrying the churn without a clear win. |
1 participant
The File Provider extension uses a background
URLSessionso transfers continue while it's suspended. But Apple's background sessions only support upload and download tasks, so issuing PROPFIND and the other metadata/control requests as data tasks on that session is undefined behavior and can stall transfers.WebDAVSessionnow holds two sessions and routes by intent: a foreground.defaultdata session for metadata/control ops, and the background transfer session for file transfers. The foreground-only path collapses both to one session, and both carry the same cookie/credential hardening.Includes a routing regression test that stands up the two sessions with distinct recording
URLProtocols and asserts each operation rides its intended session.Flow