Merge pull request #2779 from readdle/operation-queue-overrelease

Balance OperationQueue retain/release

Author: compnerd

Sources/Foundation/Operation.swift

@@ -125,7 +125,7 @@ open class Operation : NSObject {
     internal var _queue: OperationQueue? {
         _lock()
         defer { _unlock() }
-        return __queue?.takeRetainedValue()
+        return __queue?.takeUnretainedValue()
     }
 
     internal func _adopt(queue: OperationQueue, schedule: DispatchWorkItem) {
@@ -230,7 +230,7 @@ open class Operation : NSObject {
         let kind: Transition?
         if keyPath == _NSOperationIsFinished || keyPath == _NSOperationIsFinishedAlternate {
             kind = .toFinished
-        } else if keyPath == _NSOperationIsExecuting || keyPath == _NSOperationIsReadyAlternate {
+        } else if keyPath == _NSOperationIsExecuting || keyPath == _NSOperationIsExecutingAlternate {
             kind = .toExecuting
         } else if keyPath == _NSOperationIsReady || keyPath == _NSOperationIsReadyAlternate {
             kind = .toReady
@@ -474,7 +474,7 @@ open class Operation : NSObject {
 
     internal func changePriority(_ newPri: Operation.QueuePriority.RawValue) {
         _lock()
-        guard let oq = __queue?.takeRetainedValue() else {
+        guard let oq = __queue?.takeUnretainedValue() else {
             __priorityValue = newPri
             _unlock()
             return
@@ -963,6 +963,11 @@ open class OperationQueue : NSObject, ProgressReporting {
         OperationQueue._currentQueue.set(self)
         op.start()
         OperationQueue._currentQueue.clear()
+        // We've just cleared _currentQueue storage.
+        // NSThreadSpecific doesn't release stored value on clear.
+        // This means `self` will leak if we don't release manually.
+        Unmanaged.passUnretained(self).release()
+        
         // unset current tsd
         if op.isFinished && op._state.rawValue < Operation.__NSOperationState.finishing.rawValue {
             Operation.observeValue(forKeyPath: _NSOperationIsFinished, ofObject: op)

Tests/Foundation/Tests/TestOperationQueue.swift

@@ -28,17 +28,17 @@ class TestOperationQueue : XCTestCase {
             ("test_isSuspended", test_isSuspended),
             ("test_OperationDependencyCount", test_OperationDependencyCount),
             ("test_CancelDependency", test_CancelDependency),
-            /* ⚠️ */ ("test_Deadlock", testExpectedToFail(test_Deadlock, "Crashes due to overrelease of OperationQueue")),
+            ("test_Deadlock", test_Deadlock),
             ("test_CancelOutOfQueue", test_CancelOutOfQueue),
-            /* ⚠️ */ ("test_CrossQueueDependency", testExpectedToFail(test_CrossQueueDependency, "Crashes due to overrelease of OperationQueue")),
+            ("test_CrossQueueDependency", test_CrossQueueDependency),
             ("test_CancelWhileSuspended", test_CancelWhileSuspended),
             ("test_OperationOrder", test_OperationOrder),
             ("test_OperationOrder2", test_OperationOrder2),
-            /* ⚠️ */ ("test_WaitUntilFinished", testExpectedToFail(test_WaitUntilFinished, "Crashes due to overrelease of OperationQueue")),
+            ("test_WaitUntilFinished", test_WaitUntilFinished),
             ("test_OperationWaitUntilFinished", test_OperationWaitUntilFinished),
-            /* ⚠️ */ ("test_CustomOperationReady", testExpectedToFail(test_CustomOperationReady, "Crashes due to overrelease of OperationQueue")),
-            /* ⚠️ */ ("test_DependencyCycleBreak", testExpectedToFail(test_DependencyCycleBreak, "Crashes due to overrelease of OperationQueue")),
-            /* ⚠️ */ ("test_Lifecycle", testExpectedToFail(test_Lifecycle, "Crashes due to overrelease of OperationQueue")),
+            ("test_CustomOperationReady", test_CustomOperationReady),
+            ("test_DependencyCycleBreak", test_DependencyCycleBreak),
+            ("test_Lifecycle", test_Lifecycle),
         ]
     }
 
@@ -55,7 +55,12 @@ class TestOperationQueue : XCTestCase {
         queue.addOperation(op2)
         queue.addOperation(op3)
         XCTAssertEqual(queue.operationCount, 2)
-        XCTAssertEqual(queue.operations.count, 2)
+        let operations = queue.operations
+        XCTAssertEqual(operations.count, 2)
+        if (operations.count == 2) {
+            XCTAssertEqual(operations[0], op2)
+            XCTAssertEqual(operations[1], op3)
+        }
         queue.waitUntilAllOperationsAreFinished()
         XCTAssertEqual(queue.operationCount, 0)
         XCTAssertEqual(queue.operations.count, 0)
@@ -547,6 +552,11 @@ class TestOperationQueue : XCTestCase {
         let op1 = BlockOperation { Thread.sleep(forTimeInterval: 1) }
         queue1.addOperation(op1)
         op1.waitUntilFinished()
+        
+        // Operation is not removed from Queue simultaneously
+        // with transitioning to "Finished" state. Wait a bit
+        // to allow OperationQueue to deal with finished op.
+        Thread.sleep(forTimeInterval: 0.1)
         XCTAssertEqual(queue1.operationCount, 0)
     }
 
@@ -656,7 +666,7 @@ class TestOperationQueue : XCTestCase {
         }()
 
         wait(for: [opStarted], timeout: 1)
-        op2.cancel() // op2
+        op2.cancel()
         wait(for: [opDone], timeout: 1)
 
         Thread.sleep(forTimeInterval: 1) // Let queue to be deallocated