Bläddra i källkod

Prevent Multiple finish() Calls (#3116)

* Add additional state to prevent multiple finish calls.

* Put test session on the main queue.

* Reset isFinishing for retry.

* Add dispatchPreconditions to Request, fix issues.
Jon Shier 5 år sedan
förälder
incheckning
f3f29dccc8

+ 8 - 0
Alamofire.xcodeproj/project.pbxproj

@@ -51,6 +51,9 @@
 		311B199420B0ED980036823B /* UploadTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F8111E5F19A9674D0040E7D1 /* UploadTests.swift */; };
 		311B199520B0ED980036823B /* UploadTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F8111E5F19A9674D0040E7D1 /* UploadTests.swift */; };
 		311B199620B0ED990036823B /* UploadTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F8111E5F19A9674D0040E7D1 /* UploadTests.swift */; };
+		31425AC1241F098000EE3CCC /* InternalRequestTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31425AC0241F098000EE3CCC /* InternalRequestTests.swift */; };
+		31425AC2241F098000EE3CCC /* InternalRequestTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31425AC0241F098000EE3CCC /* InternalRequestTests.swift */; };
+		31425AC3241F098000EE3CCC /* InternalRequestTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31425AC0241F098000EE3CCC /* InternalRequestTests.swift */; };
 		31501E882196962A005829F2 /* ParameterEncoderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31501E872196962A005829F2 /* ParameterEncoderTests.swift */; };
 		31501E892196962A005829F2 /* ParameterEncoderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31501E872196962A005829F2 /* ParameterEncoderTests.swift */; };
 		31501E8A2196962A005829F2 /* ParameterEncoderTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31501E872196962A005829F2 /* ParameterEncoderTests.swift */; };
@@ -351,6 +354,7 @@
 		311B198F20B0D3B40036823B /* MultipartUpload.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MultipartUpload.swift; sourceTree = "<group>"; };
 		312D1E0B1FC2551400E51FF1 /* Usage.md */ = {isa = PBXFileReference; lastKnownFileType = net.daringfireball.markdown; name = Usage.md; path = Documentation/Usage.md; sourceTree = "<group>"; };
 		312D1E0C1FC2551400E51FF1 /* AdvancedUsage.md */ = {isa = PBXFileReference; lastKnownFileType = net.daringfireball.markdown; name = AdvancedUsage.md; path = Documentation/AdvancedUsage.md; sourceTree = "<group>"; };
+		31425AC0241F098000EE3CCC /* InternalRequestTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = InternalRequestTests.swift; sourceTree = "<group>"; };
 		31501E872196962A005829F2 /* ParameterEncoderTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ParameterEncoderTests.swift; sourceTree = "<group>"; };
 		316250E41F00ABE900E207A6 /* ISSUE_TEMPLATE.md */ = {isa = PBXFileReference; lastKnownFileType = net.daringfireball.markdown; name = ISSUE_TEMPLATE.md; path = .github/ISSUE_TEMPLATE.md; sourceTree = "<group>"; };
 		316250E51F00ACD000E207A6 /* PULL_REQUEST_TEMPLATE.md */ = {isa = PBXFileReference; lastKnownFileType = net.daringfireball.markdown; name = PULL_REQUEST_TEMPLATE.md; path = .github/PULL_REQUEST_TEMPLATE.md; sourceTree = "<group>"; };
@@ -524,6 +528,7 @@
 			children = (
 				F8E6024419CB46A800A3E7F1 /* AuthenticationTests.swift */,
 				F8111E5B19A9674D0040E7D1 /* DownloadTests.swift */,
+				31425AC0241F098000EE3CCC /* InternalRequestTests.swift */,
 				31501E872196962A005829F2 /* ParameterEncoderTests.swift */,
 				F8111E5C19A9674D0040E7D1 /* ParameterEncodingTests.swift */,
 				F8111E5D19A9674D0040E7D1 /* RequestTests.swift */,
@@ -1328,6 +1333,7 @@
 				3107EA4120A1267D00445260 /* SessionDelegateTests.swift in Sources */,
 				31C2B0EC20B271060089BA7C /* CacheTests.swift in Sources */,
 				3111CE9120A7EC27008315E2 /* NetworkReachabilityManagerTests.swift in Sources */,
+				31425AC3241F098000EE3CCC /* InternalRequestTests.swift in Sources */,
 				4CF627171BA7CC240011A099 /* ParameterEncodingTests.swift in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
@@ -1480,6 +1486,7 @@
 				3107EA3F20A1267C00445260 /* SessionDelegateTests.swift in Sources */,
 				31C2B0EA20B271040089BA7C /* CacheTests.swift in Sources */,
 				3111CE8F20A7EC26008315E2 /* NetworkReachabilityManagerTests.swift in Sources */,
+				31425AC1241F098000EE3CCC /* InternalRequestTests.swift in Sources */,
 				F8111E6119A9674D0040E7D1 /* ParameterEncodingTests.swift in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
@@ -1515,6 +1522,7 @@
 				3107EA4020A1267C00445260 /* SessionDelegateTests.swift in Sources */,
 				31C2B0EB20B271050089BA7C /* CacheTests.swift in Sources */,
 				3111CE9020A7EC27008315E2 /* NetworkReachabilityManagerTests.swift in Sources */,
+				31425AC2241F098000EE3CCC /* InternalRequestTests.swift in Sources */,
 				4C256A541B096C770065714F /* BaseTestCase.swift in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;

+ 49 - 1
Source/Request.swift

@@ -113,6 +113,9 @@ public class Request {
         var retryCount = 0
         /// Final `AFError` for the `Request`, whether from various internal Alamofire calls or as a result of a `task`.
         var error: AFError?
+        /// Whether the instance has had `finish()` called and is running the serializers. Should be replaced with a
+        /// representation in the state machine in the future.
+        var isFinishing = false
     }
 
     /// Protected `MutableState` value that provides thread-safe access to state values.
@@ -272,6 +275,8 @@ public class Request {
     ///
     /// - Parameter request: The `URLRequest` created.
     func didCreateInitialURLRequest(_ request: URLRequest) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         protectedMutableState.write { $0.requests.append(request) }
 
         eventMonitor?.request(self, didCreateInitialURLRequest: request)
@@ -283,6 +288,8 @@ public class Request {
     ///
     /// - Parameter error: `AFError` thrown from the failed creation.
     func didFailToCreateURLRequest(with error: AFError) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         self.error = error
 
         eventMonitor?.request(self, didFailToCreateURLRequestWithError: error)
@@ -298,6 +305,8 @@ public class Request {
     ///   - initialRequest: The `URLRequest` that was adapted.
     ///   - adaptedRequest: The `URLRequest` returned by the `RequestAdapter`.
     func didAdaptInitialRequest(_ initialRequest: URLRequest, to adaptedRequest: URLRequest) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         protectedMutableState.write { $0.requests.append(adaptedRequest) }
 
         eventMonitor?.request(self, didAdaptInitialRequest: initialRequest, to: adaptedRequest)
@@ -311,6 +320,8 @@ public class Request {
     ///   - request: The `URLRequest` the adapter was called with.
     ///   - error:   The `AFError` returned by the `RequestAdapter`.
     func didFailToAdaptURLRequest(_ request: URLRequest, withError error: AFError) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         self.error = error
 
         eventMonitor?.request(self, didFailToAdaptURLRequest: request, withError: error)
@@ -324,6 +335,8 @@ public class Request {
     ///
     /// - Parameter request: The `URLRequest` created.
     func didCreateURLRequest(_ request: URLRequest) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         eventMonitor?.request(self, didCreateURLRequest: request)
 
         callCURLHandlerIfNecessary()
@@ -343,6 +356,8 @@ public class Request {
     ///
     /// - Parameter task: The `URLSessionTask` created.
     func didCreateTask(_ task: URLSessionTask) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         protectedMutableState.write { $0.tasks.append(task) }
 
         eventMonitor?.request(self, didCreateTask: task)
@@ -350,6 +365,8 @@ public class Request {
 
     /// Called when resumption is completed.
     func didResume() {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         eventMonitor?.requestDidResume(self)
     }
 
@@ -357,11 +374,15 @@ public class Request {
     ///
     /// - Parameter task: The `URLSessionTask` resumed.
     func didResumeTask(_ task: URLSessionTask) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         eventMonitor?.request(self, didResumeTask: task)
     }
 
     /// Called when suspension is completed.
     func didSuspend() {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         eventMonitor?.requestDidSuspend(self)
     }
 
@@ -369,11 +390,15 @@ public class Request {
     ///
     /// - Parameter task: The `URLSessionTask` suspended.
     func didSuspendTask(_ task: URLSessionTask) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         eventMonitor?.request(self, didSuspendTask: task)
     }
 
     /// Called when cancellation is completed, sets `error` to `AFError.explicitlyCancelled`.
     func didCancel() {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         error = AFError.explicitlyCancelled
 
         eventMonitor?.requestDidCancel(self)
@@ -383,6 +408,8 @@ public class Request {
     ///
     /// - Parameter task: The `URLSessionTask` cancelled.
     func didCancelTask(_ task: URLSessionTask) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         eventMonitor?.request(self, didCancelTask: task)
     }
 
@@ -390,6 +417,8 @@ public class Request {
     ///
     /// - Parameter metrics: The `URLSessionTaskMetrics` gathered.
     func didGatherMetrics(_ metrics: URLSessionTaskMetrics) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         protectedMutableState.write { $0.metrics.append(metrics) }
 
         eventMonitor?.request(self, didGatherMetrics: metrics)
@@ -401,6 +430,8 @@ public class Request {
     ///   - task:  The `URLSessionTask` which failed.
     ///   - error: The early failure `AFError`.
     func didFailTask(_ task: URLSessionTask, earlyWithError error: AFError) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         self.error = error
 
         // Task will still complete, so didCompleteTask(_:with:) will handle retry.
@@ -416,7 +447,10 @@ public class Request {
     ///   - error: The `AFError` `task` may have completed with. If `error` has already been set on the instance, this
     ///            value is ignored.
     func didCompleteTask(_ task: URLSessionTask, with error: AFError?) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         self.error = self.error ?? error
+
         protectedValidators.directValue.forEach { $0() }
 
         eventMonitor?.request(self, didCompleteTask: task, with: error)
@@ -426,6 +460,8 @@ public class Request {
 
     /// Called when the `RequestDelegate` is going to retry this `Request`. Calls `reset()`.
     func prepareForRetry() {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         protectedMutableState.write { $0.retryCount += 1 }
 
         reset()
@@ -438,6 +474,8 @@ public class Request {
     ///
     /// - Parameter error: The possible `AFError` which may trigger retry.
     func retryOrFinish(error: AFError?) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
         guard let error = error, let delegate = delegate else { finish(); return }
 
         delegate.retryResult(for: self, dueTo: error) { retryResult in
@@ -456,6 +494,12 @@ public class Request {
     ///
     /// - Parameter error: The possible `Error` with which the instance will finish.
     func finish(error: AFError? = nil) {
+        dispatchPrecondition(condition: .onQueue(underlyingQueue))
+
+        guard !protectedMutableState.directValue.isFinishing else { return }
+
+        protectedMutableState.directValue.isFinishing = true
+
         if let error = error { self.error = error }
 
         // Start response handlers
@@ -525,6 +569,7 @@ public class Request {
                 }
 
                 mutableState.responseSerializerProcessingFinished = true
+                mutableState.isFinishing = false
             }
 
             completions.forEach { $0() }
@@ -556,7 +601,10 @@ public class Request {
         downloadProgress.totalUnitCount = 0
         downloadProgress.completedUnitCount = 0
 
-        protectedMutableState.write { $0.responseSerializerCompletions = [] }
+        protectedMutableState.write { state in
+            state.isFinishing = false
+            state.responseSerializerCompletions = []
+        }
     }
 
     /// Called when updating the upload progress.

+ 5 - 1
Source/Session.swift

@@ -964,7 +964,11 @@ open class Session {
     // MARK: - Invalidation
 
     func finishRequestsForDeinit() {
-        requestTaskMap.requests.forEach { $0.finish(error: AFError.sessionDeinitialized) }
+        requestTaskMap.requests.forEach { request in
+            rootQueue.async {
+                request.finish(error: AFError.sessionDeinitialized)
+            }
+        }
     }
 }
 

+ 49 - 0
Tests/InternalRequestTests.swift

@@ -0,0 +1,49 @@
+//
+//  InternalRequestTests.swift
+//
+//  Copyright (c) 2020 Alamofire Software Foundation (http://alamofire.org/)
+//
+//  Permission is hereby granted, free of charge, to any person obtaining a copy
+//  of this software and associated documentation files (the "Software"), to deal
+//  in the Software without restriction, including without limitation the rights
+//  to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+//  copies of the Software, and to permit persons to whom the Software is
+//  furnished to do so, subject to the following conditions:
+//
+//  The above copyright notice and this permission notice shall be included in
+//  all copies or substantial portions of the Software.
+//
+//  THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+//  IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+//  FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+//  AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+//  LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+//  OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+//  THE SOFTWARE.
+//
+
+@testable import Alamofire
+import XCTest
+
+final class InternalRequestTests: BaseTestCase {
+    func testThatMultipleFinishInvocationsDoNotCallSerializersMoreThanOnce() {
+        // Given
+        let session = Session(rootQueue: .main, startRequestsImmediately: false)
+        let expect = expectation(description: "request complete")
+        var response: DataResponse<Data?, AFError>?
+
+        // When
+        let request = session.request(URLRequest.makeHTTPBinRequest()).response { resp in
+            response = resp
+            expect.fulfill()
+        }
+        for _ in 0..<100 {
+            request.finish()
+        }
+
+        waitForExpectations(timeout: timeout)
+
+        // Then
+        XCTAssertNotNil(response)
+    }
+}

+ 1 - 1
Tests/RetryPolicyTests.swift

@@ -62,7 +62,7 @@ class BaseRetryPolicyTestCase: BaseTestCase {
     let nonIdempotentMethods: Set<HTTPMethod> = [.post, .patch, .connect]
     var methods: Set<HTTPMethod> { return idempotentMethods.union(nonIdempotentMethods) }
 
-    let session = Session(startRequestsImmediately: false)
+    let session = Session(rootQueue: .main, startRequestsImmediately: false)
 
     let url = URL(string: "https://api.alamofire.org")!