Fixed issue where serializer completion was not called when request was complete (#2810)

Source/AFError.swift

@@ -139,6 +139,8 @@ public enum AFError: Error {
         case decodingFailed(error: Error)
         /// Generic serialization failed for an empty response that wasn't type `Empty` but instead the associated type.
         case invalidEmptyResponse(type: String)
+        /// A response serializer was added to the request after the request was already finished.
+        case responseSerializerAddedAfterRequestFinished
     }
 
     /// Underlying reason a server trust evaluation error occured.
@@ -554,6 +556,8 @@ extension AFError.ResponseSerializationFailureReason {
             return "Empty response could not be serialized to type: \(type). Use Empty as the expected type for such responses."
         case .decodingFailed(let error):
             return "Response could not be decoded because of error:\n\(error.localizedDescription)"
+        case .responseSerializerAddedAfterRequestFinished:
+            return "Response serializer was added to the request after it had already finished."
         }
     }
 }

Source/Request.swift

@@ -38,17 +38,24 @@ public class Request {
     /// - cancelled: Set when `cancel()` is called. Any tasks created for the `Request` will have `cancel()` called on
     ///              them. Unlike `resumed` or `suspended`, once in the `cancelled` state, the `Request` can no longer
     ///              transition to any other state.
+    /// - finished:  Set when all response serialization completion closures have been cleared on the `Request` and
+    ///              queued on their respective queues.
     public enum State {
-        case initialized, resumed, suspended, cancelled
+        case initialized, resumed, suspended, cancelled, finished
 
         /// Determines whether `self` can be transitioned to `state`.
         func canTransitionTo(_ state: State) -> Bool {
             switch (self, state) {
-            case (.initialized, _): return true
-            case (_, .initialized), (.cancelled, _): return false
-            case (.resumed, .cancelled), (.suspended, .cancelled),
-                 (.resumed, .suspended), (.suspended, .resumed): return true
-            case (.suspended, .suspended), (.resumed, .resumed): return false
+            case (.initialized, _):
+                return true
+            case (_, .initialized), (.cancelled, _), (.finished, _):
+                return false
+            case (.resumed, .cancelled), (.suspended, .cancelled), (.resumed, .suspended), (.suspended, .resumed):
+                return true
+            case (.suspended, .suspended), (.resumed, .resumed):
+                return false
+            case (_, .finished):
+                return true
             }
         }
     }
@@ -86,6 +93,8 @@ public class Request {
         var responseSerializers: [() -> Void] = []
         /// Response serialization completion closures executed once all response serialization is complete.
         var responseSerializerCompletions: [() -> Void] = []
+        /// Whether response serializer processing is finished.
+        var responseSerializerProcessingFinished = false
         /// `URLCredential` used for authentication challenges.
         var credential: URLCredential?
         /// All `URLRequest`s created by Alamofire on behalf of the `Request`.
@@ -106,14 +115,16 @@ public class Request {
 
     /// `State` of the `Request`.
     public var state: State { return protectedMutableState.directValue.state }
-    /// Returns whether `state` is `.cancelled`.
-    public var isCancelled: Bool { return state == .cancelled }
+    /// Returns whether `state` is `.initialized`.
+    public var isInitialized: Bool { return state == .initialized }
     /// Returns whether `state is `.resumed`.
     public var isResumed: Bool { return state == .resumed }
     /// Returns whether `state` is `.suspended`.
     public var isSuspended: Bool { return state == .suspended }
-    /// Returns whether `state` is `.initialized`.
-    public var isInitialized: Bool { return state == .initialized }
+    /// Returns whether `state` is `.cancelled`.
+    public var isCancelled: Bool { return state == .cancelled }
+    /// Returns whether `state` is `.finished`.
+    public var isFinished: Bool { return state == .finished }
 
     // Progress
 
@@ -387,7 +398,17 @@ public class Request {
 
     /// Appends the response serialization closure to the `Request`.
     func appendResponseSerializer(_ closure: @escaping () -> Void) {
-        protectedMutableState.write { $0.responseSerializers.append(closure) }
+        protectedMutableState.write { mutableState in
+            mutableState.responseSerializers.append(closure)
+
+            if mutableState.state == .finished {
+                mutableState.error = AFError.responseSerializationFailed(reason: .responseSerializerAddedAfterRequestFinished)
+            }
+
+            if mutableState.responseSerializerProcessingFinished {
+                underlyingQueue.async { self.processNextResponseSerializer() }
+            }
+        }
     }
 
     /// Returns the next response serializer closure to execute if there's one left.
@@ -420,6 +441,12 @@ public class Request {
                 // An example of how this can happen is by calling cancel inside a response completion closure.
                 mutableState.responseSerializers.removeAll()
                 mutableState.responseSerializerCompletions.removeAll()
+
+                if mutableState.state.canTransitionTo(.finished) {
+                    mutableState.state = .finished
+                }
+
+                mutableState.responseSerializerProcessingFinished = true
             }
 
             completions.forEach { $0() }

Source/Session.swift

@@ -485,6 +485,9 @@ open class Session {
             case (_, .cancelled):
                 task.cancel()
                 rootQueue.async { request.didCancelTask(task) }
+            case (_, .finished):
+                // Do nothing
+                break
             }
         }
     }

Tests/AFError+AlamofireTests.swift

@@ -142,6 +142,11 @@ extension AFError {
         return false
     }
 
+    var isResponseSerializerAddedAfterRequestFinished: Bool {
+        if case let .responseSerializationFailed(reason) = self, reason.isResponseSerializerAddedAfterRequestFinished { return true }
+        return false
+    }
+
     // ResponseValidationFailureReason
 
     var isDataFileNil: Bool {
@@ -290,6 +295,11 @@ extension AFError.ResponseSerializationFailureReason {
         if case .invalidEmptyResponse = self { return true }
         return false
     }
+
+    var isResponseSerializerAddedAfterRequestFinished: Bool {
+        if case .responseSerializerAddedAfterRequestFinished = self { return true }
+        return false
+    }
 }
 
 // MARK: -

Tests/DownloadTests.swift

@@ -419,7 +419,7 @@ final class DownloadRequestEventsTestCase: BaseTestCase {
         waitForExpectations(timeout: timeout, handler: nil)
 
         // Then
-        XCTAssertEqual(request.state, .resumed)
+        XCTAssertEqual(request.state, .finished)
     }
 
     func testThatCancelledDownloadRequestTriggersAllAppropriateLifetimeEvents() {

Tests/RequestTests.swift

@@ -26,8 +26,6 @@ import Alamofire
 import Foundation
 import XCTest
 
-// MARK: -
-
 class RequestResponseTestCase: BaseTestCase {
     func testRequestResponse() {
         // Given
@@ -263,7 +261,7 @@ class RequestResponseTestCase: BaseTestCase {
         XCTAssertEqual(receivedResponse?.result.value?.form, ["property": "one"])
     }
 
-    // MARK: - Lifetime Events
+    // MARK: Lifetime Events
 
     func testThatAutomaticallyResumedRequestReceivesAppropriateLifetimeEvents() {
         // Given
@@ -288,7 +286,7 @@ class RequestResponseTestCase: BaseTestCase {
         waitForExpectations(timeout: timeout, handler: nil)
 
         // Then
-        XCTAssertEqual(request.state, .resumed)
+        XCTAssertEqual(request.state, .finished)
     }
 
     func testThatAutomaticallyAndManuallyResumedRequestReceivesAppropriateLifetimeEvents() {
@@ -317,7 +315,7 @@ class RequestResponseTestCase: BaseTestCase {
         waitForExpectations(timeout: timeout, handler: nil)
 
         // Then
-        XCTAssertEqual(request.state, .resumed)
+        XCTAssertEqual(request.state, .finished)
     }
 
     func testThatManuallyResumedRequestReceivesAppropriateLifetimeEvents() {
@@ -346,7 +344,7 @@ class RequestResponseTestCase: BaseTestCase {
         waitForExpectations(timeout: timeout, handler: nil)
 
         // Then
-        XCTAssertEqual(request.state, .resumed)
+        XCTAssertEqual(request.state, .finished)
     }
 
     func testThatRequestManuallyResumedManyTimesOnlyReceivesAppropriateLifetimeEvents() {
@@ -375,7 +373,7 @@ class RequestResponseTestCase: BaseTestCase {
         waitForExpectations(timeout: timeout, handler: nil)
 
         // Then
-        XCTAssertEqual(request.state, .resumed)
+        XCTAssertEqual(request.state, .finished)
     }
 
     func testThatRequestManuallySuspendedManyTimesAfterAutomaticResumeOnlyReceivesAppropriateLifetimeEvents() {
@@ -561,7 +559,7 @@ class RequestResponseTestCase: BaseTestCase {
         waitForExpectations(timeout: timeout, handler: nil)
 
         // Then
-        XCTAssertEqual(request.state, .resumed)
+        XCTAssertEqual(request.state, .finished)
     }
 
     func testThatCancelledRequestTriggersAllAppropriateLifetimeEvents() {
@@ -600,6 +598,68 @@ class RequestResponseTestCase: BaseTestCase {
         // Then
         XCTAssertEqual(request.state, .cancelled)
     }
+
+    func testThatAppendingResponseSerializerToCancelledRequestCallsCompletion() {
+        // Given
+        let session = Session()
+
+        var response1: DataResponse<Any>?
+        var response2: DataResponse<Any>?
+
+        let expect = expectation(description: "both response serializer completions should be called")
+        expect.expectedFulfillmentCount = 2
+
+        // When
+        let request = session.request(URLRequest.makeHTTPBinRequest())
+
+        request.responseJSON { resp in
+            response1 = resp
+            expect.fulfill()
+
+            request.responseJSON { resp in
+                response2 = resp
+                expect.fulfill()
+            }
+        }
+
+        request.cancel()
+
+        waitForExpectations(timeout: timeout, handler: nil)
+
+        // Then
+        XCTAssertEqual(response1?.error?.asAFError?.isExplicitlyCancelledError, true)
+        XCTAssertEqual(response2?.error?.asAFError?.isExplicitlyCancelledError, true)
+    }
+
+    func testThatAppendingResponseSerializerToCompletedRequestCallsCompletion() {
+        // Given
+        let session = Session()
+
+        var response1: DataResponse<Any>?
+        var response2: DataResponse<Any>?
+
+        let expect = expectation(description: "both response serializer completions should be called")
+        expect.expectedFulfillmentCount = 2
+
+        // When
+        let request = session.request(URLRequest.makeHTTPBinRequest())
+
+        request.responseJSON { resp in
+            response1 = resp
+            expect.fulfill()
+
+            request.responseJSON { resp in
+                response2 = resp
+                expect.fulfill()
+            }
+        }
+
+        waitForExpectations(timeout: timeout, handler: nil)
+
+        // Then
+        XCTAssertNotNil(response1?.value)
+        XCTAssertEqual(response2?.error?.asAFError?.isResponseSerializerAddedAfterRequestFinished, true)
+    }
 }
 
 // MARK: -

Tests/SessionTests.swift

@@ -1491,7 +1491,7 @@ class SessionTestCase: BaseTestCase {
         waitForExpectations(timeout: timeout, handler: nil)
 
         // Then
-        XCTAssertEqual(request.state, .resumed)
+        XCTAssertEqual(request.state, .finished)
         XCTAssertEqual(response?.result.isSuccess, true)
     }
 }

Tests/UploadTests.swift

@@ -678,7 +678,7 @@ final class UploadRequestEventsTestCase: BaseTestCase {
         waitForExpectations(timeout: timeout, handler: nil)
 
         // Then
-        XCTAssertEqual(request.state, .resumed)
+        XCTAssertEqual(request.state, .finished)
     }
 
     func testThatCancelledUploadRequestTriggersAllAppropriateLifetimeEvents() {