Browse Source

Fix memory leaks (#3199)

* Add various unowned references to fix cycle leaks.

* Add leaks handler for future automated checking.

* Formatting.

* Move to Xcode 11.5 for CI.

* Fix watchOS target.

* Add missed file to project.

* Add proper copyright header.
Jon Shier 5 years ago
parent
commit
240669132a

+ 10 - 10
.github/workflows/ci.yml

@@ -17,22 +17,22 @@ jobs:
       DEVELOPER_DIR: /Applications/Xcode_11.3.1.app/Contents/Developer
     steps:
       - uses: actions/checkout@v2
-      - name: macOS
+      - name: macOS (5.1)
         run: set -o pipefail && env NSUnbufferedIO=YES xcodebuild -project "Alamofire.xcodeproj" -scheme "Alamofire macOS" -destination "platform=macOS" clean test | xcpretty
   macOS_5_2:
     name: Test macOS (5.2)
     runs-on: macOS-latest
     env: 
-      DEVELOPER_DIR: /Applications/Xcode_11.4.1.app/Contents/Developer
+      DEVELOPER_DIR: /Applications/Xcode_11.5.app/Contents/Developer
     steps:
       - uses: actions/checkout@v2
-      - name: macOS
+      - name: macOS (5.2)
         run: set -o pipefail && env NSUnbufferedIO=YES xcodebuild -project "Alamofire.xcodeproj" -scheme "Alamofire macOS" -destination "platform=macOS" clean test | xcpretty
   Catalyst:
     name: Test Catalyst 
     runs-on: macOS-latest
     env: 
-      DEVELOPER_DIR: /Applications/Xcode_11.4.1.app/Contents/Developer
+      DEVELOPER_DIR: /Applications/Xcode_11.5.app/Contents/Developer
     steps:
       - uses: actions/checkout@v2
       - name: Catalyst
@@ -41,10 +41,10 @@ jobs:
     name: Test iOS 
     runs-on: macOS-latest
     env: 
-      DEVELOPER_DIR: /Applications/Xcode_11.4.1.app/Contents/Developer
+      DEVELOPER_DIR: /Applications/Xcode_11.5.app/Contents/Developer
     strategy:
       matrix:
-        destination: ["OS=13.4.1,name=iPhone 11 Pro"] #, "OS=12.4,name=iPhone XS", "OS=11.4,name=iPhone X", "OS=10.3.1,name=iPhone SE"]
+        destination: ["OS=13.5,name=iPhone 11 Pro"] #, "OS=12.4,name=iPhone XS", "OS=11.4,name=iPhone X", "OS=10.3.1,name=iPhone SE"]
     steps:
       - uses: actions/checkout@v2
       - name: iOS - ${{ matrix.destination }}
@@ -53,7 +53,7 @@ jobs:
     name: Test tvOS 
     runs-on: macOS-latest
     env: 
-      DEVELOPER_DIR: /Applications/Xcode_11.4.1.app/Contents/Developer
+      DEVELOPER_DIR: /Applications/Xcode_11.5.app/Contents/Developer
     strategy:
       matrix:
         destination: ["OS=13.4,name=Apple TV 4K"] #, "OS=11.4,name=Apple TV 4K", "OS=10.2,name=Apple TV 1080p"]
@@ -65,10 +65,10 @@ jobs:
     name: Build watchOS
     runs-on: macOS-latest
     env: 
-      DEVELOPER_DIR: /Applications/Xcode_11.4.1.app/Contents/Developer
+      DEVELOPER_DIR: /Applications/Xcode_11.5.app/Contents/Developer
     strategy:
       matrix:
-        destination: ["OS=6.2,name=Apple Watch Series 5 - 44mm"] #, "OS=4.2,name=Apple Watch Series 3 - 42mm", "OS=3.2,name=Apple Watch Series 2 - 42mm"]
+        destination: ["OS=6.2.1,name=Apple Watch Series 5 - 44mm"] #, "OS=4.2,name=Apple Watch Series 3 - 42mm", "OS=3.2,name=Apple Watch Series 2 - 42mm"]
     steps:
       - uses: actions/checkout@v2
       - name: watchOS - ${{ matrix.destination }}
@@ -77,7 +77,7 @@ jobs:
     name: Test with SPM
     runs-on: macOS-latest    
     env: 
-      DEVELOPER_DIR: /Applications/Xcode_11.4.1.app/Contents/Developer
+      DEVELOPER_DIR: /Applications/Xcode_11.5.app/Contents/Developer
     steps:
       - uses: actions/checkout@v2
       - name: SPM Test

+ 8 - 0
Alamofire.xcodeproj/project.pbxproj

@@ -78,6 +78,9 @@
 		31727422218BB9A50039FFCC /* HTTPBin.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31727421218BB9A50039FFCC /* HTTPBin.swift */; };
 		31727423218BB9A50039FFCC /* HTTPBin.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31727421218BB9A50039FFCC /* HTTPBin.swift */; };
 		31727424218BB9A50039FFCC /* HTTPBin.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31727421218BB9A50039FFCC /* HTTPBin.swift */; };
+		31762DCA247738FA0025C704 /* LeaksTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31762DC9247738FA0025C704 /* LeaksTests.swift */; };
+		31762DCB247738FA0025C704 /* LeaksTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31762DC9247738FA0025C704 /* LeaksTests.swift */; };
+		31762DCC247738FA0025C704 /* LeaksTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 31762DC9247738FA0025C704 /* LeaksTests.swift */; };
 		317A6A7620B2207F00A9FEC5 /* DownloadTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F8111E5B19A9674D0040E7D1 /* DownloadTests.swift */; };
 		317A6A7720B2208000A9FEC5 /* DownloadTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F8111E5B19A9674D0040E7D1 /* DownloadTests.swift */; };
 		317A6A7820B2208000A9FEC5 /* DownloadTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = F8111E5B19A9674D0040E7D1 /* DownloadTests.swift */; };
@@ -391,6 +394,7 @@
 		31727417218BAEC90039FFCC /* HTTPMethod.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HTTPMethod.swift; sourceTree = "<group>"; };
 		3172741C218BB1790039FFCC /* ParameterEncoder.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ParameterEncoder.swift; sourceTree = "<group>"; };
 		31727421218BB9A50039FFCC /* HTTPBin.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = HTTPBin.swift; sourceTree = "<group>"; };
+		31762DC9247738FA0025C704 /* LeaksTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LeaksTests.swift; sourceTree = "<group>"; };
 		318DD40E2439780500963291 /* Combine.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Combine.swift; sourceTree = "<group>"; };
 		3191B5741F5F53A6003960A8 /* Protected.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Protected.swift; sourceTree = "<group>"; };
 		31991790209CDA7F00103A19 /* Request.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Request.swift; sourceTree = "<group>"; };
@@ -907,6 +911,7 @@
 			children = (
 				4C256A501B096C2C0065714F /* BaseTestCase.swift */,
 				31727421218BB9A50039FFCC /* HTTPBin.swift */,
+				31762DC9247738FA0025C704 /* LeaksTests.swift */,
 				31F9683B20BB70290009606F /* NSLoggingEventMonitor.swift */,
 				4C256A4E1B09656A0065714F /* Core */,
 				4C7C8D201B9D0D7300948136 /* Extensions */,
@@ -1379,6 +1384,7 @@
 				31C2B0EC20B271060089BA7C /* CacheTests.swift in Sources */,
 				3111CE9120A7EC27008315E2 /* NetworkReachabilityManagerTests.swift in Sources */,
 				31BADE502439A8D1007D2AB9 /* CombineTests.swift in Sources */,
+				31762DCC247738FA0025C704 /* LeaksTests.swift in Sources */,
 				31425AC3241F098000EE3CCC /* InternalRequestTests.swift in Sources */,
 				4CF627171BA7CC240011A099 /* ParameterEncodingTests.swift in Sources */,
 			);
@@ -1546,6 +1552,7 @@
 				31C2B0EA20B271040089BA7C /* CacheTests.swift in Sources */,
 				3111CE8F20A7EC26008315E2 /* NetworkReachabilityManagerTests.swift in Sources */,
 				31BADE4E2439A8D1007D2AB9 /* CombineTests.swift in Sources */,
+				31762DCA247738FA0025C704 /* LeaksTests.swift in Sources */,
 				31425AC1241F098000EE3CCC /* InternalRequestTests.swift in Sources */,
 				F8111E6119A9674D0040E7D1 /* ParameterEncodingTests.swift in Sources */,
 			);
@@ -1587,6 +1594,7 @@
 				31C2B0EB20B271050089BA7C /* CacheTests.swift in Sources */,
 				3111CE9020A7EC27008315E2 /* NetworkReachabilityManagerTests.swift in Sources */,
 				31BADE4F2439A8D1007D2AB9 /* CombineTests.swift in Sources */,
+				31762DCB247738FA0025C704 /* LeaksTests.swift in Sources */,
 				31425AC2241F098000EE3CCC /* InternalRequestTests.swift in Sources */,
 				4C256A541B096C770065714F /* BaseTestCase.swift in Sources */,
 			);

+ 3 - 1
Source/Request.swift

@@ -1599,7 +1599,9 @@ public class DownloadRequest: Request {
 
             let result = validation(self.request, response, self.fileURL)
 
-            if case let .failure(error) = result { self.error = error.asAFError(or: .responseValidationFailed(reason: .customValidationFailed(error: error))) }
+            if case let .failure(error) = result {
+                self.error = error.asAFError(or: .responseValidationFailed(reason: .customValidationFailed(error: error)))
+            }
 
             self.eventMonitor?.request(self,
                                        didValidateRequest: self.request,

+ 3 - 3
Source/ResponseSerialization.swift

@@ -952,7 +952,7 @@ extension DataStreamRequest {
     /// - Returns:  The `DataStreamRequest`.
     @discardableResult
     public func responseStream(on queue: DispatchQueue = .main, stream: @escaping Handler<Data, Never>) -> Self {
-        $streamMutableState.write { state in
+        $streamMutableState.write { [unowned self] state in
             let capture = (queue, { data in
                 self.capturingError {
                     try stream(.init(event: .stream(.success(data)), token: .init(self)))
@@ -977,7 +977,7 @@ extension DataStreamRequest {
     public func responseStream<Serializer: DataStreamSerializer>(using serializer: Serializer,
                                                                  on queue: DispatchQueue = .main,
                                                                  stream: @escaping Handler<Serializer.SerializedObject, AFError>) -> Self {
-        let parser = { (data: Data) in
+        let parser = { [unowned self] (data: Data) in
             self.serializationQueue.async {
                 // Start work on serialization queue.
                 let result = Result { try serializer.serialize(data) }
@@ -1011,7 +1011,7 @@ extension DataStreamRequest {
     @discardableResult
     public func responseStreamString(on queue: DispatchQueue = .main,
                                      stream: @escaping Handler<String, Never>) -> Self {
-        let parser = { (data: Data) in
+        let parser = { [unowned self] (data: Data) in
             self.serializationQueue.async {
                 // Start work on serialization queue.
                 let string = String(decoding: data, as: UTF8.self)

+ 4 - 1
Source/Validation.swift

@@ -232,7 +232,10 @@ extension DataStreamRequest {
     /// - Returns: The instance.
     @discardableResult
     public func validate() -> Self {
-        validate(statusCode: acceptableStatusCodes).validate(contentType: self.acceptableContentTypes)
+        let contentTypes: () -> [String] = { [unowned self] in
+            self.acceptableContentTypes
+        }
+        return validate(statusCode: acceptableStatusCodes).validate(contentType: contentTypes())
     }
 }
 

+ 8 - 8
Tests/CombineTests.swift

@@ -863,21 +863,21 @@ final class DataStreamRequestCombineTests: CombineTestCase {
         // Given
         let responseReceived = expectation(description: "response should be received")
         let completionReceived = expectation(description: "stream should complete")
-        var stream: DataStreamRequest.Stream<HTTPBinResponse, AFError>?
+        var error: AFError?
 
         // When
         let request = AF.streamRequest(URLRequest.makeHTTPBinRequest())
         var token: AnyCancellable? = request
             .publishDecodable(type: HTTPBinResponse.self)
             .sink(receiveCompletion: { _ in completionReceived.fulfill() },
-                  receiveValue: { stream = $0; responseReceived.fulfill() })
+                  receiveValue: { error = $0.completion?.error; responseReceived.fulfill() })
         token = nil
 
         waitForExpectations(timeout: timeout)
 
         // Then
-        XCTAssertNotNil(stream?.completion?.error)
-        XCTAssertTrue(stream?.completion?.error?.isExplicitlyCancelledError == true)
+        XCTAssertNotNil(error)
+        XCTAssertTrue(error?.isExplicitlyCancelledError == true)
         XCTAssertTrue(request.isCancelled)
         XCTAssertNil(token)
     }
@@ -887,7 +887,7 @@ final class DataStreamRequestCombineTests: CombineTestCase {
         // Given
         let responseReceived = expectation(description: "response should be received")
         let completionReceived = expectation(description: "stream should complete")
-        var stream: DataStreamRequest.Stream<HTTPBinResponse, AFError>?
+        var error: AFError?
 
         // When
         let request = AF.streamRequest(URLRequest.makeHTTPBinRequest())
@@ -895,15 +895,15 @@ final class DataStreamRequestCombineTests: CombineTestCase {
             request
                 .publishDecodable(type: HTTPBinResponse.self)
                 .sink(receiveCompletion: { _ in completionReceived.fulfill() },
-                      receiveValue: { stream = $0; responseReceived.fulfill() })
+                      receiveValue: { error = $0.completion?.error; responseReceived.fulfill() })
         }
         request.cancel()
 
         waitForExpectations(timeout: timeout)
 
         // Then
-        XCTAssertNotNil(stream?.completion?.error)
-        XCTAssertTrue(stream?.completion?.error?.isExplicitlyCancelledError == true)
+        XCTAssertNotNil(error)
+        XCTAssertTrue(error?.isExplicitlyCancelledError == true)
         XCTAssertTrue(request.isCancelled)
     }
 

+ 6 - 6
Tests/DownloadTests.swift

@@ -459,7 +459,7 @@ final class DownloadRequestEventsTestCase: BaseTestCase {
             responseHandler.fulfill()
         }
 
-        eventMonitor.requestDidResumeTask = { _, _ in
+        eventMonitor.requestDidResumeTask = { [unowned request] _, _ in
             request.cancel()
             didResumeTask.fulfill()
         }
@@ -487,7 +487,7 @@ final class DownloadResumeDataTestCase: BaseTestCase {
 
         // When
         let download = AF.download(urlString)
-        download.downloadProgress { progress in
+        download.downloadProgress { [unowned download] progress in
             guard !cancelled else { return }
 
             if progress.fractionCompleted > 0.1 {
@@ -521,7 +521,7 @@ final class DownloadResumeDataTestCase: BaseTestCase {
 
         // When
         let download = AF.download(urlString)
-        download.downloadProgress { progress in
+        download.downloadProgress { [unowned download] progress in
             guard !cancelled else { return }
 
             if progress.fractionCompleted > 0.1 {
@@ -557,7 +557,7 @@ final class DownloadResumeDataTestCase: BaseTestCase {
 
         // When
         let download = AF.download(urlString)
-        download.downloadProgress { progress in
+        download.downloadProgress { [unowned download] progress in
             guard !cancelled else { return }
 
             if progress.fractionCompleted > 0.1 {
@@ -594,7 +594,7 @@ final class DownloadResumeDataTestCase: BaseTestCase {
 
         // When
         let download = AF.download(urlString)
-        download.downloadProgress { progress in
+        download.downloadProgress { [unowned download] progress in
             guard !cancelled else { return }
 
             if progress.fractionCompleted > 0.1 {
@@ -654,7 +654,7 @@ final class DownloadResumeDataTestCase: BaseTestCase {
 
         // When
         let download = AF.download(urlString)
-        download.downloadProgress { progress in
+        download.downloadProgress { [unowned download] progress in
             guard !cancelled else { return }
 
             if progress.fractionCompleted > 0.1 {

+ 69 - 0
Tests/LeaksTests.swift

@@ -0,0 +1,69 @@
+//
+//  LeaksTests.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.
+//
+
+import XCTest
+
+// Only build when built through SPM, as tests run through Xcode don't like this.
+// Add LEAKS flag once we figure out a way to automate this.
+// Can run by invoking swift test -c debug -Xswiftc -DLEAKS in the Alamofire directory.
+// Sample code from the Swift forums: https://forums.swift.org/t/test-for-memory-leaks-in-ci/36526/19
+#if SWIFT_PACKAGE && LEAKS && os(macOS)
+final class LeaksTests: XCTestCase {
+    func testForLeaks() {
+        // Sets up an atexit handler that invokes the leaks tool.
+        atexit {
+            @discardableResult
+            func leaksTo(_ file: String) -> Process {
+                let out = FileHandle(forWritingAtPath: file)!
+                defer {
+                    if #available(OSX 10.15, *) {
+                        try! out.close()
+                    } else {
+                        // Fallback on earlier versions
+                    }
+                }
+                let process = Process()
+                process.launchPath = "/usr/bin/leaks"
+                process.arguments = ["\(getpid())"]
+                process.standardOutput = out
+                process.standardError = out
+                process.launch()
+                process.waitUntilExit()
+                return process
+            }
+            let process = leaksTo("/dev/null")
+            guard process.terminationReason == .exit && [0, 1].contains(process.terminationStatus) else {
+                print("Process terminated: \(process.terminationReason): \(process.terminationStatus)")
+                exit(255)
+            }
+            if process.terminationStatus == 1 {
+                print("================")
+                print("Leaks Detected!!!")
+                leaksTo("/dev/tty")
+            }
+            exit(process.terminationStatus)
+        }
+    }
+}
+#endif

+ 4 - 4
Tests/RequestTests.swift

@@ -490,7 +490,7 @@ final class RequestResponseTestCase: BaseTestCase {
         // When
         let request = session.request(URLRequest.makeHTTPBinRequest())
         // Cancellation stops task creation, so don't cancel the request until the task has been created.
-        eventMonitor.requestDidCreateTask = { _, _ in
+        eventMonitor.requestDidCreateTask = { [unowned request] _, _ in
             for _ in 0..<100 {
                 request.cancel()
             }
@@ -521,7 +521,7 @@ final class RequestResponseTestCase: BaseTestCase {
         // When
         let request = session.request(URLRequest.makeHTTPBinRequest())
         // Cancellation stops task creation, so don't cancel the request until the task has been created.
-        eventMonitor.requestDidCreateTask = { _, _ in
+        eventMonitor.requestDidCreateTask = { [unowned request] _, _ in
             for _ in 0..<100 {
                 request.cancel()
             }
@@ -552,7 +552,7 @@ final class RequestResponseTestCase: BaseTestCase {
         // When
         let request = session.request(URLRequest.makeHTTPBinRequest(path: "delay/5")).response { _ in expect.fulfill() }
         // Cancellation stops task creation, so don't cancel the request until the task has been created.
-        eventMonitor.requestDidCreateTask = { _, _ in
+        eventMonitor.requestDidCreateTask = { [unowned request] _, _ in
             DispatchQueue.concurrentPerform(iterations: 100) { i in
                 request.cancel()
 
@@ -650,7 +650,7 @@ final class RequestResponseTestCase: BaseTestCase {
             responseHandler.fulfill()
         }
 
-        eventMonitor.requestDidResumeTask = { _, _ in
+        eventMonitor.requestDidResumeTask = { [unowned request] _, _ in
             request.cancel()
             didResumeTask.fulfill()
         }

+ 1 - 1
Tests/SessionTests.swift

@@ -1589,7 +1589,7 @@ final class SessionCancellationTestCase: BaseTestCase {
         let createTask = expectation(description: "should create task twice")
         createTask.expectedFulfillmentCount = 2
         var tasksCreated = 0
-        monitor.requestDidCreateTask = { _, _ in
+        monitor.requestDidCreateTask = { [unowned session] _, _ in
             tasksCreated += 1
             createTask.fulfill()
             // Cancel after the second task is created to ensure proper lifetime events.

+ 2 - 2
Tests/URLProtocolTests.swift

@@ -41,12 +41,12 @@ class ProxyURLProtocol: URLProtocol {
             return configuration
         }()
 
-        let session = Foundation.URLSession(configuration: configuration, delegate: self, delegateQueue: nil)
+        let session = URLSession(configuration: configuration, delegate: self, delegateQueue: nil)
 
         return session
     }()
 
-    var activeTask: URLSessionTask?
+    weak var activeTask: URLSessionTask?
 
     // MARK: Class Request Methods
 

+ 1 - 1
Tests/UploadTests.swift

@@ -754,7 +754,7 @@ final class UploadRequestEventsTestCase: BaseTestCase {
             responseHandler.fulfill()
         }
 
-        eventMonitor.requestDidResumeTask = { _, _ in
+        eventMonitor.requestDidResumeTask = { [unowned request] _, _ in
             request.cancel()
             didResumeTask.fulfill()
         }

+ 1 - 1
Tests/ValidationTests.swift

@@ -734,7 +734,7 @@ extension DataRequest {
 
 extension DownloadRequest {
     func validateDataExists() -> Self {
-        validate { _, _, _ in
+        validate { [unowned self] _, _, _ in
             let fileURL = self.fileURL
 
             guard let validFileURL = fileURL else { return .failure(ValidationError.missingFile) }