Browse Source

Refactor Lock Usage and Introduce Protector (#2290)

* Drop iOS 8 / macOS 10.10 support and remove all workarounds.

* First draft of response serializer refactor and decodable serializers.

* Refactor serializer protocols and implementations.

* Finish refactors, update inline docs.

* Remove download serializer from Data, as it’s default now.

* Update whitespace.

* Add failure expectation test.

* Initial versions of Mutex and Protector.

* Rename value to unsafeValue.

* Add UnfairLock.

* Use UnfairLock on supported OSes, hide everything.

* Clean whitespace.

* Cleanup based on comments.

* Remove TODO.
Jon Shier 8 years ago
parent
commit
71d09002e9

+ 10 - 0
Alamofire.xcodeproj/project.pbxproj

@@ -21,6 +21,10 @@
 /* End PBXAggregateTarget section */
 
 /* Begin PBXBuildFile section */
+		3191B5751F5F53A6003960A8 /* Mutex+Protector.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3191B5741F5F53A6003960A8 /* Mutex+Protector.swift */; };
+		3191B5761F5F53A6003960A8 /* Mutex+Protector.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3191B5741F5F53A6003960A8 /* Mutex+Protector.swift */; };
+		3191B5771F5F53A6003960A8 /* Mutex+Protector.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3191B5741F5F53A6003960A8 /* Mutex+Protector.swift */; };
+		3191B5781F5F53A6003960A8 /* Mutex+Protector.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3191B5741F5F53A6003960A8 /* Mutex+Protector.swift */; };
 		31ED52E81D73891B00199085 /* AFError+AlamofireTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31ED52E61D73889D00199085 /* AFError+AlamofireTests.swift */; };
 		31ED52E91D73891C00199085 /* AFError+AlamofireTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31ED52E61D73889D00199085 /* AFError+AlamofireTests.swift */; };
 		31ED52EA1D73891C00199085 /* AFError+AlamofireTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31ED52E61D73889D00199085 /* AFError+AlamofireTests.swift */; };
@@ -289,6 +293,7 @@
 		312D1E0C1FC2551400E51FF1 /* AdvancedUsage.md */ = {isa = PBXFileReference; lastKnownFileType = net.daringfireball.markdown; name = AdvancedUsage.md; path = Documentation/AdvancedUsage.md; sourceTree = "<group>"; };
 		316250E41F00ABE900E207A6 /* ISSUE_TEMPLATE.md */ = {isa = PBXFileReference; lastKnownFileType = net.daringfireball.markdown; path = ISSUE_TEMPLATE.md; sourceTree = "<group>"; };
 		316250E51F00ACD000E207A6 /* PULL_REQUEST_TEMPLATE.md */ = {isa = PBXFileReference; lastKnownFileType = net.daringfireball.markdown; path = PULL_REQUEST_TEMPLATE.md; sourceTree = "<group>"; };
+		3191B5741F5F53A6003960A8 /* Mutex+Protector.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Mutex+Protector.swift"; sourceTree = "<group>"; };
 		31ED52E61D73889D00199085 /* AFError+AlamofireTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "AFError+AlamofireTests.swift"; sourceTree = "<group>"; };
 		4C0B58381B747A4400C0B99C /* ResponseSerializationTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = ResponseSerializationTests.swift; sourceTree = "<group>"; };
 		4C0B62501BB1001C009302D3 /* Response.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Response.swift; sourceTree = "<group>"; };
@@ -602,6 +607,7 @@
 			isa = PBXGroup;
 			children = (
 				4C1DC8531B68908E00476DE3 /* AFError.swift */,
+				3191B5741F5F53A6003960A8 /* Mutex+Protector.swift */,
 				4CB928281C66BFBC00CE5F08 /* Notifications.swift */,
 				4CE2724E1AF88FB500F1D59A /* ParameterEncoding.swift */,
 				4CDE2C391AF899EC00BABAE5 /* Request.swift */,
@@ -1190,6 +1196,7 @@
 				4CFCFE301D56D31700A76388 /* SessionDelegate.swift in Sources */,
 				4CF6270C1BA7CBF60011A099 /* Result.swift in Sources */,
 				4CF627081BA7CBF60011A099 /* AFError.swift in Sources */,
+				3191B5771F5F53A6003960A8 /* Mutex+Protector.swift in Sources */,
 				4CF627131BA7CBF60011A099 /* Validation.swift in Sources */,
 				4CF6270E1BA7CBF60011A099 /* MultipartFormData.swift in Sources */,
 				4C80F9F81BB730EF001B46D2 /* Response.swift in Sources */,
@@ -1241,6 +1248,7 @@
 				4CFCFE2F1D56D31700A76388 /* SessionDelegate.swift in Sources */,
 				4CE272501AF88FB500F1D59A /* ParameterEncoding.swift in Sources */,
 				4CDE2C3B1AF899EC00BABAE5 /* Request.swift in Sources */,
+				3191B5761F5F53A6003960A8 /* Mutex+Protector.swift in Sources */,
 				4CDE2C471AF89FF300BABAE5 /* ResponseSerialization.swift in Sources */,
 				4C1DC8551B68908E00476DE3 /* AFError.swift in Sources */,
 				4CDE2C381AF8932A00BABAE5 /* SessionManager.swift in Sources */,
@@ -1265,6 +1273,7 @@
 				4CEE82AD1C6813CF00E9C9F0 /* NetworkReachabilityManager.swift in Sources */,
 				4CFCFE311D56D31700A76388 /* SessionDelegate.swift in Sources */,
 				E4202FD01B667AA100C997FB /* ParameterEncoding.swift in Sources */,
+				3191B5781F5F53A6003960A8 /* Mutex+Protector.swift in Sources */,
 				E4202FD11B667AA100C997FB /* Request.swift in Sources */,
 				4CEC605A1B745C9100E684F4 /* AFError.swift in Sources */,
 				E4202FD21B667AA100C997FB /* ResponseSerialization.swift in Sources */,
@@ -1289,6 +1298,7 @@
 				4CFCFE2E1D56D31700A76388 /* SessionDelegate.swift in Sources */,
 				4CE2724F1AF88FB500F1D59A /* ParameterEncoding.swift in Sources */,
 				4CDE2C3A1AF899EC00BABAE5 /* Request.swift in Sources */,
+				3191B5751F5F53A6003960A8 /* Mutex+Protector.swift in Sources */,
 				4CDE2C461AF89FF300BABAE5 /* ResponseSerialization.swift in Sources */,
 				4C1DC8541B68908E00476DE3 /* AFError.swift in Sources */,
 				4CDE2C371AF8932A00BABAE5 /* SessionManager.swift in Sources */,

+ 167 - 0
Source/Mutex+Protector.swift

@@ -0,0 +1,167 @@
+//
+//  Mutex+Protector.swift
+//
+//  Copyright (c) 2014-2017 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.
+//
+
+import Foundation
+
+/// A lock abstraction.
+private protocol Lock {
+    func lock()
+    func unlock()
+    func around<T>(_ closure: () -> T) -> T
+    func around(_ closure: () -> Void)
+}
+
+extension Lock {
+    /// Execute a value producing closure while aquiring the mutex.
+    ///
+    /// - Parameter closure: The closure to run.
+    /// - Returns:           The value the closure generated.
+    func around<T>(_ closure: () -> T) -> T {
+        lock(); defer { unlock() }
+        return closure()
+    }
+
+    /// Execute a closure while aquiring the mutex.
+    ///
+    /// - Parameter closure: The closure to run.
+    func around(_ closure: () -> Void) {
+        lock(); defer { unlock() }
+        return closure()
+    }
+}
+
+// MARK: -
+
+/// A `pthread_mutex` wrapper, inspired by ProcedureKit.
+final class Mutex: Lock {
+    private var mutex = pthread_mutex_t()
+
+    init() {
+        let result = pthread_mutex_init(&mutex, nil)
+        precondition(result == 0, "Failed to create pthread mutex")
+    }
+
+    deinit {
+        let result = pthread_mutex_destroy(&mutex)
+        assert(result == 0, "Failed to destroy mutex")
+    }
+
+    fileprivate func lock() {
+        let result = pthread_mutex_lock(&mutex)
+        assert(result == 0, "Failed to lock mutex")
+    }
+
+    fileprivate func unlock() {
+        let result = pthread_mutex_unlock(&mutex)
+        assert(result == 0, "Failed to unlock mutex")
+    }
+}
+
+// MARK: -
+
+/// An `os_unfair_lock` wrapper.
+@available (iOS 10.0, macOS 10.12, tvOS 10.0, watchOS 3.0, *)
+final class UnfairLock: Lock {
+    private var unfairLock = os_unfair_lock()
+
+    public init() { }
+
+    fileprivate func lock() {
+        os_unfair_lock_lock(&unfairLock)
+    }
+
+    fileprivate func unlock() {
+        os_unfair_lock_unlock(&unfairLock)
+    }
+}
+
+// MARK: -
+
+/// A thread-safe wrapper around a value.
+final class Protector<T> {
+    private let lock: Lock = {
+        if #available(iOS 10.0, macOS 10.12, tvOS 10.0, watchOS 3.0, *) {
+            return UnfairLock()
+        } else {
+            return Mutex()
+        }
+    }()
+
+    private var value: T
+
+    init(_ value: T) {
+        self.value = value
+    }
+
+    /// The contained value. Unsafe for anything more than direct read or write.
+    var directValue: T {
+        get { return lock.around { value } }
+        set { lock.around { value = newValue } }
+    }
+
+    /// Synchronously read or transform the contained value.
+    ///
+    /// - Parameter closure: The closure to execute.
+    /// - Returns:           The return value of the closure passed.
+    func read<U>(_ closure: (T) -> U) -> U {
+        return lock.around { closure(self.value) }
+    }
+
+    /// Synchronously modify the protected value.
+    ///
+    /// - Parameter closure: The closure to execute.
+    /// - Returns:           The modified value.
+    @discardableResult
+    func write<U>(_ closure: (inout T) -> U) -> U {
+        return lock.around { closure(&self.value) }
+    }
+}
+
+extension Protector where T: RangeReplaceableCollection {
+    func append(_ newElement: T.Iterator.Element) {
+        write { (ward: inout T) in
+            ward.append(newElement)
+        }
+    }
+
+    func append<S: Sequence>(contentsOf newElements: S) where S.Iterator.Element == T.Iterator.Element {
+        write { (ward: inout T) in
+            ward.append(contentsOf: newElements)
+        }
+    }
+
+    func append<C: Collection>(contentsOf newElements: C) where C.Iterator.Element == T.Iterator.Element {
+        write { (ward: inout T) in
+            ward.append(contentsOf: newElements)
+        }
+    }
+}
+
+extension Protector where T: Strideable {
+    func advance(by stride: T.Stride) {
+        write { (ward: inout T) in
+            ward = ward.advanced(by: stride)
+        }
+    }
+}

+ 7 - 14
Source/Request.swift

@@ -88,14 +88,8 @@ open class Request {
 
     /// The delegate for the underlying task.
     open internal(set) var delegate: TaskDelegate {
-        get {
-            taskDelegateLock.lock() ; defer { taskDelegateLock.unlock() }
-            return taskDelegate
-        }
-        set {
-            taskDelegateLock.lock() ; defer { taskDelegateLock.unlock() }
-            taskDelegate = newValue
-        }
+        get { return protectedDelegate.directValue }
+        set { protectedDelegate.directValue = newValue }
     }
 
     /// The underlying task.
@@ -120,8 +114,7 @@ open class Request {
 
     var validations: [() -> Void] = []
 
-    private var taskDelegate: TaskDelegate
-    private var taskDelegateLock = NSLock()
+    private var protectedDelegate: Protector<TaskDelegate>
 
     // MARK: Lifecycle
 
@@ -130,16 +123,16 @@ open class Request {
 
         switch requestTask {
         case .data(let originalTask, let task):
-            taskDelegate = DataTaskDelegate(task: task)
+            protectedDelegate = Protector(DataTaskDelegate(task: task))
             self.originalTask = originalTask
         case .download(let originalTask, let task):
-            taskDelegate = DownloadTaskDelegate(task: task)
+            protectedDelegate = Protector(DownloadTaskDelegate(task: task))
             self.originalTask = originalTask
         case .upload(let originalTask, let task):
-            taskDelegate = UploadTaskDelegate(task: task)
+            protectedDelegate = Protector(UploadTaskDelegate(task: task))
             self.originalTask = originalTask
         case .stream(let originalTask, let task):
-            taskDelegate = TaskDelegate(task: task)
+            protectedDelegate = Protector(TaskDelegate(task: task))
             self.originalTask = originalTask
         }
 

+ 4 - 4
Source/ResponseSerialization.swift

@@ -56,14 +56,14 @@ public extension DownloadResponseSerializerProtocol where Self: DataResponseSeri
         guard let fileURL = fileURL else {
             throw AFError.responseSerializationFailed(reason: .inputFileNil)
         }
-        
+
         let data: Data
         do {
             data = try Data(contentsOf: fileURL)
         } catch {
             throw AFError.responseSerializationFailed(reason: .inputFileReadFailed(at: fileURL))
         }
-        
+
         do {
             return try serialize(request: request, response: response, data: data, error: error)
         } catch {
@@ -122,7 +122,7 @@ public final class AnyResponseSerializer<Value>: ResponseSerializer {
             } catch {
                 throw AFError.responseSerializationFailed(reason: .inputFileReadFailed(at: fileURL))
             }
-            
+
             do {
                 return try serialize(request: request, response: response, data: data, error: error)
             } catch {
@@ -392,7 +392,7 @@ public final class StringResponseSerializer: ResponseSerializer {
         }
 
         let actualEncoding = convertedEncoding ?? .isoLatin1
-        
+
         guard let string = String(data: validData, encoding: actualEncoding) else {
             throw AFError.responseSerializationFailed(reason: .stringSerializationFailed(encoding: actualEncoding))
         }

+ 3 - 10
Source/SessionDelegate.swift

@@ -126,19 +126,12 @@ open class SessionDelegate: NSObject {
     var retrier: RequestRetrier?
     weak var sessionManager: SessionManager?
 
-    private var requests: [Int: Request] = [:]
-    private let lock = NSLock()
+    private var protectedRequests = Protector<[Int: Request]>([:])
 
     /// Access the task delegate for the specified task in a thread-safe manner.
     open subscript(task: URLSessionTask) -> Request? {
-        get {
-            lock.lock() ; defer { lock.unlock() }
-            return requests[task.taskIdentifier]
-        }
-        set {
-            lock.lock() ; defer { lock.unlock() }
-            requests[task.taskIdentifier] = newValue
-        }
+        get { return protectedRequests.read { $0[task.taskIdentifier] } }
+        set { protectedRequests.write { $0[task.taskIdentifier] = newValue } }
     }
 
     // MARK: Lifecycle

+ 6 - 13
Source/TaskDelegate.swift

@@ -41,31 +41,24 @@ open class TaskDelegate: NSObject {
 
     var task: URLSessionTask? {
         set {
-            taskLock.lock(); defer { taskLock.unlock() }
-            _task = newValue
-        }
-        get {
-            taskLock.lock(); defer { taskLock.unlock() }
-            return _task
+            protectedTask.directValue = newValue
+            reset()
         }
+        get { return protectedTask.directValue }
     }
 
     var initialResponseTime: CFAbsoluteTime?
     var credential: URLCredential?
     var metrics: AnyObject? // URLSessionTaskMetrics
 
-    private var _task: URLSessionTask? {
-        didSet { reset() }
-    }
-
-    private let taskLock = NSLock()
+    private let protectedTask: Protector<URLSessionTask?>
 
     // MARK: Lifecycle
 
     init(task: URLSessionTask?) {
-        _task = task
+        protectedTask = Protector(task)
 
-        self.queue = {
+        queue = {
             let operationQueue = OperationQueue()
 
             operationQueue.maxConcurrentOperationCount = 1

+ 1 - 1
Tests/ResponseSerializationTests.swift

@@ -433,7 +433,7 @@ class DataResponseSerializationTestCase: BaseTestCase {
     }
 
     // MARK: JSONDecodableResponseSerializer
-    
+
     struct DecodableValue: Codable {
         let string: String
     }

+ 3 - 3
Tests/ResultTests.swift

@@ -213,7 +213,7 @@ class ResultTestCase: BaseTestCase {
         let result = Result<String>.success("success value")
 
         // When
-        let mappedResult = result.map { $0.characters.count }
+        let mappedResult = result.map { $0.count }
 
         // Then
         XCTAssertEqual(mappedResult.value, 13)
@@ -225,7 +225,7 @@ class ResultTestCase: BaseTestCase {
         let result = Result<String>.failure(ResultError())
 
         // When
-        let mappedResult = result.map { $0.characters.count }
+        let mappedResult = result.map { $0.count }
 
         // Then
         if let error = mappedResult.error {
@@ -242,7 +242,7 @@ class ResultTestCase: BaseTestCase {
         let result = Result<String>.success("success value")
 
         // When
-        let mappedResult = result.flatMap { $0.characters.count }
+        let mappedResult = result.flatMap { $0.count }
 
         // Then
         XCTAssertEqual(mappedResult.value, 13)