Fix resumable downloads retrying a rejected range, progress on a resumed download without Content-Length, and empty local resources

CHANGELOG.md

@@ -254,6 +254,9 @@ Across the 44 benchmarks comparable with 13.2, `main` is 1.6% faster by geometri
 - Fix `ImageProcessors/GaussianBlur` returning a corrupted near-black image and allocating gigabytes of scratch memory for a radius of about `1544` or more – https://github.com/kean/Nuke/pull/1021
 - Fix `ImageProcessors/GaussianBlur` converting wide-gamut (Display P3) images to device RGB – https://github.com/kean/Nuke/pull/1021
 - Fix `ImageProcessors/Resize/init(width:unit:upscale:)` and `init(height:unit:upscale:)` capping the other dimension at `9999`, which shrank tall or wide images below the requested size – https://github.com/kean/Nuke/pull/1021
+- Fix a resumed download sending the same `Range` again on every attempt after the server rejected it with "416 Range Not Satisfiable" – https://github.com/kean/Nuke/pull/1026
+- Fix the progress of a resumed download reporting a total below the bytes received when the "206 Partial Content" has no `Content-Length` – https://github.com/kean/Nuke/pull/1026
+- Fix an empty local file or `data:` URL succeeding with empty data instead of failing with `ImagePipeline/Error/dataIsEmpty` – https://github.com/kean/Nuke/pull/1026
 
 ### Documentation
 

Sources/Nuke/Pipeline/ImagePipeline+Error.swift

@@ -18,7 +18,8 @@ extension ImagePipeline {
         case dataMissingInCache
         /// Data loader failed to load image data with a wrapped error.
         case dataLoadingFailed(error: Swift.Error)
-        /// Data loader returned empty data.
+        /// The loaded data is empty: an empty download, local resource, or
+        /// result of a `data` closure.
         case dataIsEmpty
         /// No decoder registered for the given data.
         ///

Sources/Nuke/Tasks/TaskFetchOriginalData.swift

@@ -55,6 +55,10 @@ final class TaskFetchOriginalData: AsyncPipelineTask<(Data, URLResponse?)> {
                     $0.source = .file
                     $0.bytes = Int64(data.count)
                 }
+                guard !data.isEmpty else {
+                    send(error: .dataIsEmpty)
+                    return
+                }
                 send(value: (data, nil), isCompleted: true)
             } catch {
                 diagnostics?.endStage(stage) { $0.source = .file }
@@ -244,9 +248,11 @@ final class TaskFetchOriginalData: AsyncPipelineTask<(Data, URLResponse?)> {
     }
 
     /// The size of the whole resource: the advertised content length plus the
-    /// resumed bytes. Saturates, since Foundation reports a content length it
-    /// can't represent as `Int64.max`.
+    /// resumed bytes, or `-1` if the response doesn't say how long it is (a
+    /// resumed download has more bytes than that). Saturates, since Foundation
+    /// reports a content length it can't represent as `Int64.max`.
     private func expectedSize(of response: URLResponse) -> Int64 {
+        guard response.expectedContentLength >= 0 else { return -1 }
         let (size, isOverflow) = response.expectedContentLength.addingReportingOverflow(resumedDataCount)
         return isOverflow ? .max : size
     }
@@ -284,7 +290,7 @@ final class TaskFetchOriginalData: AsyncPipelineTask<(Data, URLResponse?)> {
         diagnostics?.endStage(downloadStage) { recordTransfer(in: &$0) }
 
         if let error {
-            tryToSaveResumableData()
+            tryToSaveResumableData(error: error)
             send(error: error)
             return
         }
@@ -361,19 +367,31 @@ final class TaskFetchOriginalData: AsyncPipelineTask<(Data, URLResponse?)> {
         send(error: .dataLoadingFailed(error: error))
     }
 
-    private func tryToSaveResumableData() {
+    private func tryToSaveResumableData(error: ImagePipeline.Error? = nil) {
         // Try to save resumable data in case the task was cancelled
         // (`URLError.cancelled`) or failed to complete with other error.
         guard pipeline.configuration.isResumableDataEnabled else { return }
         if let response = urlResponse, !data.isEmpty,
            let resumableData = ResumableData(response: response, data: data, resumedDataCount: resumedDataCount) {
             ResumableDataStorage.shared.storeResumableData(resumableData, for: request, pipeline: pipeline)
-        } else if let resumableData {
+        } else if let resumableData, !Self.isRangeRejected(by: error) {
             // The request ended before the server responded – put the data that
-            // `performDataLoad` took out of the storage back where it was.
+            // `performDataLoad` took out of the storage back where it was. Not
+            // when the server rejected the range: the next attempt would only
+            // send it again.
             ResumableDataStorage.shared.storeResumableData(resumableData, for: request, pipeline: pipeline)
         }
     }
+
+    /// `true` if the data loader rejected a "416 Range Not Satisfiable"
+    /// response to the resumed request.
+    private static func isRangeRejected(by error: ImagePipeline.Error?) -> Bool {
+        if case .dataLoadingFailed(let error as DataLoader.Error)? = error,
+           case .statusCodeUnacceptable(416) = error {
+            return true
+        }
+        return false
+    }
 }
 
 extension AsyncPipelineTask where Value == (Data, URLResponse?) {

Tests/Mocks/MockRangeServer.swift

@@ -21,6 +21,9 @@ final class MockRangeServer: DataLoading, @unchecked Sendable {
         case serve
         /// Same as `serve`, but advertises the given "Content-Length".
         case serveAdvertising(contentLength: String)
+        /// Same as `serve`, but the response has no "Content-Length", so its
+        /// length is unknown.
+        case serveWithoutContentLength
         /// Ignores the "Range" header and sends the whole resource with "200 OK".
         case ignoreRange
         /// Fails before sending a response.
@@ -73,14 +76,16 @@ final class MockRangeServer: DataLoading, @unchecked Sendable {
         }
         /// The response that honors the "Range" of the request, if it can,
         /// and the offset to send the resource from.
-        func makeRangeResponse(contentLength: String? = nil) -> (HTTPURLResponse, Int) {
+        func makeRangeResponse(contentLength: String? = nil, omitsContentLength: Bool = false) -> (HTTPURLResponse, Int) {
+            func lengthHeader(_ length: Int) -> [String: String] {
+                omitsContentLength ? [:] : ["Content-Length": contentLength ?? "\(length)"]
+            }
             guard let offset = resumeOffset(for: request, data: data, validator: validator) else {
-                return (makeResponse(statusCode: 200, headers: headers(["Content-Length": contentLength ?? "\(data.count)"])), 0)
+                return (makeResponse(statusCode: 200, headers: headers(lengthHeader(data.count))), 0)
             }
-            let partial = makeResponse(statusCode: 206, headers: headers([
-                "Content-Range": "bytes \(offset)-\(data.count - 1)/\(data.count)",
-                "Content-Length": contentLength ?? "\(data.count - offset)"
-            ]))
+            let partial = makeResponse(statusCode: 206, headers: headers(
+                lengthHeader(data.count - offset).merging(["Content-Range": "bytes \(offset)-\(data.count - 1)/\(data.count)"]) { $1 }
+            ))
             return (partial, offset)
         }
         func send(_ range: Range<Int>, _ response: URLResponse) {
@@ -102,8 +107,8 @@ final class MockRangeServer: DataLoading, @unchecked Sendable {
             completion(nil)
         case .failBeforeResponse:
             completion(URLError(.notConnectedToInternet))
-        case .serve, .serveAdvertising:
-            let (response, offset) = makeRangeResponse(contentLength: contentLength(for: step))
+        case .serve, .serveAdvertising, .serveWithoutContentLength:
+            let (response, offset) = makeRangeResponse(contentLength: contentLength(for: step), omitsContentLength: omitsContentLength(step))
             send(offset..<data.count, response)
             completion(nil)
         }
@@ -114,6 +119,10 @@ final class MockRangeServer: DataLoading, @unchecked Sendable {
         if case .serveAdvertising(let contentLength) = step { contentLength } else { nil }
     }
 
+    private func omitsContentLength(_ step: Step) -> Bool {
+        if case .serveWithoutContentLength = step { true } else { false }
+    }
+
     /// Returns the offset to resume from if the request asks for a range and
     /// its validator matches the resource.
     private func resumeOffset(for request: URLRequest, data: Data, validator: [String: String]) -> Int? {

Tests/NukeTests/ImagePipelineTests/ImagePipelineLocalResourcesTests.swift

@@ -65,6 +65,23 @@ struct ImagePipelineLocalResourcesTests {
         #expect(dataLoader.createdTaskCount == 0)
     }
 
+    /// An empty local resource fails the way an empty download does, instead
+    /// of succeeding with no data, or failing in the decoder.
+    @Test func emptyFileFailsWithDataIsEmpty() async throws {
+        // Given
+        let url = try makeTemporaryFile(with: Data())
+        defer { try? FileManager.default.removeItem(at: url) }
+
+        // When/Then
+        await #expect(throws: ImagePipeline.Error.dataIsEmpty) {
+            try await pipeline.data(for: ImageRequest(url: url))
+        }
+        await #expect(throws: ImagePipeline.Error.dataIsEmpty) {
+            try await pipeline.image(for: ImageRequest(url: url))
+        }
+        #expect(dataLoader.createdTaskCount == 0)
+    }
+
     @Test func fileURLIsNotStoredInTheDataCache() async throws {
         // Given
         let url = try makeTemporaryFile(with: Test.data)
@@ -109,6 +126,20 @@ struct ImagePipelineLocalResourcesTests {
         }
     }
 
+    @Test func emptyDataURLFailsWithDataIsEmpty() async throws {
+        // Given
+        let url = try #require(URL(string: "data:image/jpeg;base64,"))
+
+        // When/Then
+        await #expect(throws: ImagePipeline.Error.dataIsEmpty) {
+            try await pipeline.data(for: ImageRequest(url: url))
+        }
+        await #expect(throws: ImagePipeline.Error.dataIsEmpty) {
+            try await pipeline.image(for: ImageRequest(url: url))
+        }
+        #expect(dataLoader.createdTaskCount == 0)
+    }
+
     // MARK: - Case-Insensitive Schemes
 
     /// URI schemes are case-insensitive (RFC 3986), so `FILE://` has to be

Tests/NukeTests/ImagePipelineTests/ImagePipelineResumableDownloadTests.swift

@@ -4,6 +4,7 @@
 
 import Testing
 import Foundation
+import os
 @testable import Nuke
 
 /// Resumable downloads end-to-end: what the pipeline sends back to the server
@@ -247,6 +248,34 @@ struct ImagePipelineResumableDownloadTests {
         ])
     }
 
+    /// A "206 Partial Content" without "Content-Length" has an unknown length,
+    /// which is reported as unknown, the way it is for a download that was
+    /// never resumed: adding the resumed bytes to `-1` would put the total
+    /// below the bytes already received.
+    @Test func progressOfAResumedDownloadWithUnknownLengthIsNotComplete() async throws {
+        // GIVEN a download that failed after 10000 bytes
+        server.steps = [.fail(after: 10000), .serveWithoutContentLength]
+        _ = try? await pipeline.data(for: Test.request)
+
+        // WHEN it is resumed by a response without "Content-Length"
+        var progress: [ImageTask.Progress] = []
+        let task = pipeline.imageTask(with: Test.request)
+        for await value in task.progress {
+            progress.append(value)
+        }
+        let response = try await task.response
+        #expect((response.urlResponse as? HTTPURLResponse)?.statusCode == 206)
+
+        // THEN the total is unknown, and the download is never reported as
+        // complete before it is
+        #expect(progress.count > 1)
+        #expect(progress.last?.completed == 22789)
+        for value in progress {
+            #expect(value.total == -1, "\(value)")
+            #expect(value.fraction == 0, "\(value)")
+        }
+    }
+
     @Test func resumedBytesAreReportedInTheMetrics() async throws {
         // GIVEN a pipeline that records diagnostics and a download that failed
         // mid-way
@@ -427,6 +456,64 @@ struct ImagePipelineResumableDownloadTests {
         #expect(resumed == newData)
     }
 
+    /// `DataLoader` rejects a "416 Range Not Satisfiable" before the pipeline
+    /// sees the response, which used to look like a request that ended before
+    /// the server responded, so the rejected range was put back and sent
+    /// again on every attempt.
+    @Test func rangeRejectedByTheServerIsNotSentAgain() async throws {
+        // GIVEN a real `DataLoader` talking to a server that serves the first
+        // 10000 bytes and drops the connection, then rejects every range
+        struct State {
+            var ranges: [String?] = []
+            var stalled: StubURLProtocol?
+        }
+        // It holds a `StubURLProtocol`, which isn't `Sendable`, so it is unchecked
+        let state = OSAllocatedUnfairLock<State>(uncheckedState: State())
+        let url = StubURLProtocol.register { stub in
+            let range = stub.request.value(forHTTPHeaderField: "Range")
+            let attempt = state.withLockUnchecked { state -> Int in
+                state.ranges.append(range)
+                return state.ranges.count
+            }
+            let data = Test.data
+            if range != nil {
+                stub.sendResponse(statusCode: 416, headers: ["Content-Range": "bytes */\(data.count)"])
+                stub.finish()
+                return
+            }
+            stub.sendResponse(headers: ["Content-Length": "\(data.count)", "Accept-Ranges": "bytes", "ETag": "\"v1\""])
+            if attempt == 1 {
+                state.withLockUnchecked { $0.stalled = stub }
+                stub.send(data[0..<10000])
+            } else {
+                stub.send(data)
+                stub.finish()
+            }
+        }
+        let configuration = URLSessionConfiguration.ephemeral
+        configuration.protocolClasses = [StubURLProtocol.self]
+        let pipeline = ImagePipeline {
+            $0.dataLoader = DataLoader(configuration: configuration)
+            $0.imageCache = nil
+        }
+
+        // WHEN the download fails once the pipeline has the first 10000 bytes
+        let first = pipeline.imageTask(with: ImageRequest(url: url))
+        var progress = first.progress.makeAsyncIterator()
+        _ = await progress.next()
+        let stalled = try #require(state.withLockUnchecked { $0.stalled })
+        stalled.fail(URLError(.networkConnectionLost))
+        _ = try? await first.response
+
+        // WHEN it is resumed, and the server rejects the range
+        _ = try? await pipeline.data(for: ImageRequest(url: url))
+
+        // THEN the next attempt asks for the whole resource, and gets it
+        let (data, _) = try await pipeline.data(for: ImageRequest(url: url))
+        #expect(data == Test.data)
+        #expect(state.withLockUnchecked { $0.ranges } == [nil, "bytes=10000-", nil])
+    }
+
     // MARK: - Configuration
 
     @Test func resumableDataIsNotUsedWhenDisabled() async throws {