Download refactor#133
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 21 minutes and 46 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refactors FileDownloader's internal state management by introducing a new Changes
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/EZNetworking/Services/Downloader/FileDownloader.swift (1)
58-77:⚠️ Potential issue | 🟠 MajorContinuation leak when
getURLRequest()throws.
self.continuationis assigned at line 59 before thedoblock. Ifrequest.getURLRequest()throws, thecatchreturns a separate stream viaearlyExitStream, but the continuation stored onselfis neverfinish()-ed andonTerminationis never invoked. Becausestatealso remains.idle, a subsequentdownloadFileStream()call will passvalidateCanStartDownload()and overwriteself.continuationwithout terminating the previous one — leaking the prior stream's consumers.Construct the URLRequest before wiring up
self.continuation, or clean up on failure:Proposed fix
- let (stream, continuation) = AsyncStream<DownloadEvent>.makeStream() - self.continuation = continuation - - continuation.onTermination = { `@Sendable` [weak self] termination in - guard case .cancelled = termination else { return } - Task { [weak self] in - try? await self?.cancel() - } - } - - do { - let urlRequest = try request.getURLRequest() - state = .downloading - let task = session.urlSession.downloadTaskInspectable(with: urlRequest) - downloadTask = task - task.resume() - return stream - } catch { - return earlyExitStream(yielding: .failed(mapNetworkingError(from: error))) - } + let urlRequest: URLRequest + do { + urlRequest = try request.getURLRequest() + } catch { + return earlyExitStream(yielding: .failed(mapNetworkingError(from: error))) + } + + let (stream, continuation) = AsyncStream<DownloadEvent>.makeStream() + self.continuation = continuation + continuation.onTermination = { `@Sendable` [weak self] termination in + guard case .cancelled = termination else { return } + Task { [weak self] in try? await self?.cancel() } + } + + state = .downloading + let task = session.urlSession.downloadTaskInspectable(with: urlRequest) + downloadTask = task + task.resume() + return stream🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/EZNetworking/Services/Downloader/FileDownloader.swift` around lines 58 - 77, The continuation is created and stored on self before calling request.getURLRequest(), so if getURLRequest() throws the stored self.continuation is never finished and leaks; fix by creating the URLRequest first (call request.getURLRequest() before creating AsyncStream/assigning self.continuation) or, if you prefer minimal change, ensure you clean up in the catch by calling self.continuation?.finish(), setting self.continuation = nil, and returning the earlyExitStream; update FileDownloader's downloadFileStream (the AsyncStream/continuation creation and the catch block) to perform one of these fixes so no continuation remains un-finished when getURLRequest() fails.
🧹 Nitpick comments (1)
Tests/EZNetworkingTests/Services/Downloader/Helpers/DownloadStateTests.swift (1)
8-18: Consider adding negative-equality cases.The test only asserts each case equals itself, which
Equatablesynthesis makes trivially true. A few inequality checks — especially for associated-value cases with differingData— would give the test real signal:Proposed additions
`#expect`(DownloadState.cancelled == DownloadState.cancelled) + + // Different cases should not be equal + `#expect`(DownloadState.idle != DownloadState.downloading) + `#expect`(DownloadState.completed != DownloadState.failed) + + // Associated values must participate in equality + let other = Data([0x01]) + `#expect`(DownloadState.paused(resumeData: data) != .paused(resumeData: other)) + `#expect`(DownloadState.failedButCanResume(resumeData: data) + != .failedButCanResume(resumeData: other)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/EZNetworkingTests/Services/Downloader/Helpers/DownloadStateTests.swift` around lines 8 - 18, The test testDownloadStateEquivalence currently only checks each DownloadState case equals itself; add negative-equality assertions to verify non-equivalence between different enum cases and between associated-value variants with different Data to ensure Equatable behaves correctly: assert that distinct cases like DownloadState.idle != DownloadState.downloading (and other pairings) are not equal, and assert that DownloadState.paused(resumeData: dataA) != DownloadState.paused(resumeData: dataB) and DownloadState.failedButCanResume(resumeData: dataA) != DownloadState.failedButCanResume(resumeData: dataB) where dataA and dataB are different Data instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/EZNetworking/Services/Downloader/FileDownloader.swift`:
- Around line 166-169: The doc comment for private func terminate(with event:
DownloadEvent?, state newState: DownloadState) describes behavior inverted
relative to implementation; update the comment to state that when event is
non-nil the stream yields that final event and then closes, and when event is
nil the stream closes without yielding, and ensure the summary line and the two
bullet lines reflect this exact behavior for DownloadEvent and DownloadState.
---
Outside diff comments:
In `@Sources/EZNetworking/Services/Downloader/FileDownloader.swift`:
- Around line 58-77: The continuation is created and stored on self before
calling request.getURLRequest(), so if getURLRequest() throws the stored
self.continuation is never finished and leaks; fix by creating the URLRequest
first (call request.getURLRequest() before creating AsyncStream/assigning
self.continuation) or, if you prefer minimal change, ensure you clean up in the
catch by calling self.continuation?.finish(), setting self.continuation = nil,
and returning the earlyExitStream; update FileDownloader's downloadFileStream
(the AsyncStream/continuation creation and the catch block) to perform one of
these fixes so no continuation remains un-finished when getURLRequest() fails.
---
Nitpick comments:
In
`@Tests/EZNetworkingTests/Services/Downloader/Helpers/DownloadStateTests.swift`:
- Around line 8-18: The test testDownloadStateEquivalence currently only checks
each DownloadState case equals itself; add negative-equality assertions to
verify non-equivalence between different enum cases and between associated-value
variants with different Data to ensure Equatable behaves correctly: assert that
distinct cases like DownloadState.idle != DownloadState.downloading (and other
pairings) are not equal, and assert that DownloadState.paused(resumeData: dataA)
!= DownloadState.paused(resumeData: dataB) and
DownloadState.failedButCanResume(resumeData: dataA) !=
DownloadState.failedButCanResume(resumeData: dataB) where dataA and dataB are
different Data instances.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6dd6db4-2b6a-4473-98bc-c533d74c2fa0
📒 Files selected for processing (7)
Sources/EZNetworking/Services/Downloader/FileDownloader.swiftSources/EZNetworking/Services/Downloader/Helpers/DownloadState.swiftTests/EZNetworkingTests/Services/Downloader/FileDownloaderCoreFunctionalityTests.swiftTests/EZNetworkingTests/Services/Downloader/FileDownloaderInvalidStateTests.swiftTests/EZNetworkingTests/Services/Downloader/FileDownloaderPauseResumeTests.swiftTests/EZNetworkingTests/Services/Downloader/FileDownloaderTaskCancellationTests.swiftTests/EZNetworkingTests/Services/Downloader/Helpers/DownloadStateTests.swift
💤 Files with no reviewable changes (4)
- Tests/EZNetworkingTests/Services/Downloader/FileDownloaderTaskCancellationTests.swift
- Tests/EZNetworkingTests/Services/Downloader/FileDownloaderInvalidStateTests.swift
- Tests/EZNetworkingTests/Services/Downloader/FileDownloaderPauseResumeTests.swift
- Tests/EZNetworkingTests/Services/Downloader/FileDownloaderCoreFunctionalityTests.swift
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Summary by CodeRabbit
Refactor
Tests