From 7464aca028b9dc6ce9df414baa34e1866d6c2f5f Mon Sep 17 00:00:00 2001 From: Yasuhiro Inami Date: Thu, 25 Dec 2014 22:29:39 +0900 Subject: [PATCH 1/2] [Test] Improve testPauseResume_innerTask & testTry_cancel to reproduce chained-task-pause/resume bug & try() bug. --- SwiftTaskTests/SwiftTaskTests.swift | 185 ++++++++-------------------- 1 file changed, 52 insertions(+), 133 deletions(-) diff --git a/SwiftTaskTests/SwiftTaskTests.swift b/SwiftTaskTests/SwiftTaskTests.swift index 57c795b..e5ec567 100644 --- a/SwiftTaskTests/SwiftTaskTests.swift +++ b/SwiftTaskTests/SwiftTaskTests.swift @@ -137,7 +137,7 @@ class SwiftTaskTests: _TestCase self.wait() } - func testFulfill_successTaskFulfill() + func testFulfill_success_innerTask_fulfill() { var expect = self.expectationWithDescription(__FUNCTION__) @@ -169,7 +169,7 @@ class SwiftTaskTests: _TestCase self.wait() } - func testFulfill_successTaskReject() + func testFulfill_success_innerTask_reject() { var expect = self.expectationWithDescription(__FUNCTION__) @@ -317,7 +317,7 @@ class SwiftTaskTests: _TestCase self.wait() } - func testReject_failureTaskFulfill() + func testReject_failure_innerTask_fulfill() { var expect = self.expectationWithDescription(__FUNCTION__) @@ -350,7 +350,7 @@ class SwiftTaskTests: _TestCase self.wait() } - func testReject_failureTaskReject() + func testReject_failure_innerTask_reject() { var expect = self.expectationWithDescription(__FUNCTION__) @@ -473,7 +473,7 @@ class SwiftTaskTests: _TestCase self.wait() } - func testProgress_async_then() + func testProgress_innerTask_then() { // NOTE: this is async test if !self.isAsync { return } @@ -502,7 +502,7 @@ class SwiftTaskTests: _TestCase self.wait() } - func testProgress_async_success() + func testProgress_innerTask_success() { // NOTE: this is async test if !self.isAsync { return } @@ -531,7 +531,7 @@ class SwiftTaskTests: _TestCase self.wait() } - func testProgress_async_failure() + func testProgress_innerTask_failure() { // NOTE: this is async test if !self.isAsync { return } @@ -606,7 +606,7 @@ class SwiftTaskTests: _TestCase self.wait() } - func testCancel_thenTask() + func testCancel_then_innerTask() { var expect = self.expectationWithDescription(__FUNCTION__) @@ -673,22 +673,23 @@ class SwiftTaskTests: _TestCase } - // pause at time between _interruptableTask's 1st & 2nd delay (t=0.3) + // pause at t=0.3 (between _interruptableTask's 1st & 2nd delay before pause-check) Async.main(after: 0.3) { task.pause() XCTAssertEqual(task.state, TaskState.Paused) - XCTAssertTrue(task.progress? == 2) + XCTAssertTrue(task.progress? == 2, "`task` should be progressed halfway.") - // resume after 0.3sec (t=0.6) + // resume at t=0.6 Async.main(after: 0.3) { XCTAssertEqual(task.state, TaskState.Paused) - XCTAssertTrue(task.progress? == 2) + XCTAssertTrue(task.progress? == 2, "`task` should pause progressing.") task.resume() - XCTAssertEqual(task.state, TaskState.Running) + + XCTAssertEqual(task.state, TaskState.Running, "`task` should start running again.") } } @@ -696,7 +697,7 @@ class SwiftTaskTests: _TestCase self.wait() } - func testPauseResume_async_then() + func testPauseResume_innerTask() { // NOTE: this is async test if !self.isAsync { return } @@ -706,7 +707,7 @@ class SwiftTaskTests: _TestCase let task = _interruptableTask(progressCount: 5) weak var innerTask: _InterruptableTask? - // chain async-task with then + // chain async-task with `then` let task2 = task.then { [weak self] _ -> _InterruptableTask in innerTask = _interruptableTask(progressCount: 5) return innerTask! @@ -717,138 +718,43 @@ class SwiftTaskTests: _TestCase expect.fulfill() } - // pause at time between _interruptableTask's 1st & 2nd delay (t=0.3) + // pause at t=0.3 (between _interruptableTask's 1st & 2nd delay before pause-check) Async.main(after: 0.3) { - // NOTE: parentTask should also be paused (if not, `task` will never be fulfilled/rejected) + // NOTE: task2 will be paused, task2.pause() - XCTAssertNil(innerTask, "`innerTask` has not been created yet.") - XCTAssertEqual(task2.state, TaskState.Paused) XCTAssertNil(task2.progress, "`task2.progress` should be nil because `innerTask.progress()` has not been invoked yet.") - XCTAssertEqual(task.state, TaskState.Paused, "Parent task should also be paused.") - XCTAssertTrue(task.progress? == 2) + XCTAssertNil(innerTask, "`innerTask` should NOT be created yet.") - // resume after 0.3sec (t=0.6) - Async.main(after: 0.3) { - - XCTAssertEqual(task2.state, TaskState.Paused) - XCTAssertNil(task2.progress, "`task2.progress` should still be nil.") - - XCTAssertEqual(task.state, TaskState.Paused, "Parent task should also be paused.") - XCTAssertTrue(task.progress? == 2) - - task2.resume() - XCTAssertEqual(task2.state, TaskState.Running) - XCTAssertEqual(task.state, TaskState.Running, "Parent task should also be resumed.") - - } - } - - self.wait() - } - - func testPauseResume_async_success() - { - // NOTE: this is async test - if !self.isAsync { return } - - var expect = self.expectationWithDescription(__FUNCTION__) - - let task = _interruptableTask(progressCount: 5) - weak var innerTask: _InterruptableTask? - - // chain async-task with success - let task2 = task.success { [weak self] _ -> _InterruptableTask in - innerTask = _interruptableTask(progressCount: 5) - return innerTask! - } - - task2.success { value -> Void in - XCTAssertEqual(value, "OK") - expect.fulfill() - } - - // pause at time between _interruptableTask's 1st & 2nd delay (t=0.3) - Async.main(after: 0.3) { - - // NOTE: parentTask should also be paused (if not, `task` will never be fulfilled/rejected) - task2.pause() + XCTAssertEqual(task.state, TaskState.Running, "`task` should NOT be paused.") + XCTAssertTrue(task.progress? == 2, "`task` should be halfway progressed.") + XCTAssertNil(task.value, "`task` should NOT be fulfilled yet.") - XCTAssertEqual(task2.state, TaskState.Paused) - XCTAssertNil(task2.progress, "`task2.progress` should be nil because `innerTask.progress()` has not been invoked yet.") - - XCTAssertEqual(task.state, TaskState.Paused, "Parent task should also be paused.") - XCTAssertTrue(task.progress? == 2) - - // resume after 0.3sec (t=0.6) + // resume at t=0.6 Async.main(after: 0.3) { - XCTAssertNil(innerTask, "`innerTask` has not been created yet.") - XCTAssertEqual(task2.state, TaskState.Paused) XCTAssertNil(task2.progress, "`task2.progress` should still be nil.") - XCTAssertEqual(task.state, TaskState.Paused, "Parent task should also be paused.") - XCTAssertTrue(task.progress? == 2) + XCTAssertNotNil(innerTask, "`innerTask` should be created at this point.") + XCTAssertEqual(innerTask!.state, task2.state, "`innerTask!.state` should be same as `task2.state`.") - task2.resume() - XCTAssertEqual(task2.state, TaskState.Running) - XCTAssertEqual(task.state, TaskState.Running, "Parent task should also be resumed.") + XCTAssertEqual(task.state, TaskState.Fulfilled, "`task` should NOT be paused, and it should be fulfilled at this point.") + XCTAssertEqual(task.value!, "OK", "`task` should be fulfilled.") - } - } - - self.wait() - } - - func testPauseResume_async_failure() - { - // NOTE: this is async test - if !self.isAsync { return } - - var expect = self.expectationWithDescription(__FUNCTION__) - - let task = _interruptableTask(progressCount: 5) - weak var innerTask: _InterruptableTask? - - // chain async-task with failure - let task2 = task.failure { [weak self] _ -> _InterruptableTask in - innerTask = _interruptableTask(progressCount: 5) - return innerTask! - } - - task2.success { value -> Void in - XCTAssertEqual(value, "OK") - expect.fulfill() - } - - // pause at time between _interruptableTask's 1st & 2nd delay (t=0.3) - Async.main(after: 0.3) { - - // NOTE: parentTask should also be paused (if not, `task` will never be fulfilled/rejected) - task2.pause() - - XCTAssertEqual(task2.state, TaskState.Paused) - XCTAssertNil(task2.progress, "`task2.progress` should be nil because `innerTask.progress()` has not been invoked yet.") - - XCTAssertEqual(task.state, TaskState.Paused, "Parent task should also be paused.") - XCTAssertTrue(task.progress? == 2) - - // resume after 0.3sec (t=0.6) - Async.main(after: 0.3) { - - XCTAssertEqual(task2.state, TaskState.Paused) - XCTAssertNil(task2.progress, "`task2.progress` should still be nil.") + task2.resume() - XCTAssertEqual(task.state, TaskState.Paused, "Parent task should also be paused.") - XCTAssertTrue(task.progress? == 2) + XCTAssertEqual(task2.state, TaskState.Running, "`task2` should be resumed.") - task2.resume() - XCTAssertEqual(task2.state, TaskState.Running) - XCTAssertEqual(task.state, TaskState.Running, "Parent task should also be resumed.") + // check tasks's states at t=0.7 + Async.main(after: 0.1) { + XCTAssertEqual(task2.state, TaskState.Running) + XCTAssertEqual(innerTask!.state, task2.state, "`innerTask!.state` should be same as `task2.state`.") + XCTAssertEqual(task.state, TaskState.Fulfilled) + } } } @@ -1054,9 +960,9 @@ class SwiftTaskTests: _TestCase var maxTryCount = 3 var actualTryCount = 0 - let retryableTask = Task { progress, fulfill, reject, configure in + let task = Task { progress, fulfill, reject, configure in - Async.main(after: 0.1) { + Async.main(after: 0.3) { actualTryCount++ @@ -1070,7 +976,9 @@ class SwiftTaskTests: _TestCase return - }.try(maxTryCount) + } + + let retryableTask = task.try(maxTryCount) retryableTask.success { value -> Void in @@ -1079,8 +987,19 @@ class SwiftTaskTests: _TestCase }.failure { errorInfo -> Void in XCTAssertTrue(errorInfo.isCancelled) - expect.fulfill() +// expect.fulfill() + + } + + task.success { value -> Void in + XCTFail("Should never reach here because `retryableTask` is cancelled so original-`task` should also be cancelled.") + + }.failure { errorInfo -> Void in + + XCTAssertTrue(errorInfo.isCancelled) + expect.fulfill() + } // cancel `retryableTask` at some point before all tries completes From 6d6fbcbdf18cfcf44abe6a054e00804e46cbd1e7 Mon Sep 17 00:00:00 2001 From: Yasuhiro Inami Date: Thu, 25 Dec 2014 22:42:09 +0900 Subject: [PATCH 2/2] Fix 7464aca by not propagating pause/resume/cancel to upstream by default, but do so only in try() method instead (partially reverting #10 d489ef9) --- SwiftTask/SwiftTask.swift | 95 +++++++++++---------------------------- 1 file changed, 27 insertions(+), 68 deletions(-) diff --git a/SwiftTask/SwiftTask.swift b/SwiftTask/SwiftTask.swift index 91633ce..50b0ed1 100644 --- a/SwiftTask/SwiftTask.swift +++ b/SwiftTask/SwiftTask.swift @@ -65,20 +65,7 @@ public class TaskConfiguration } } -// abstract class for `weak _parentTask` with arbitrary `Progress` & `Value` types -public class _Task -{ - internal weak var _parentTask: _Task? - - internal let _weakified: Bool - - public init(weakified: Bool, paused: Bool) { self._weakified = weakified } - public func pause() -> Bool { return true } - public func resume() -> Bool { return true } - public func cancel(error: Error? = nil) -> Bool { return true } -} - -public class Task: _Task +public class Task { public typealias ErrorInfo = (error: Error?, isCancelled: Bool) @@ -101,6 +88,7 @@ public class Task: _Task private var machine: Machine! // store initial parameters for cloning task when using `try()` + internal let _weakified: Bool internal var _initClosure: _InitClosure? // will be nil on fulfilled/rejected /// wrapper closure for `_initClosure` to invoke only once when started `.Running` @@ -144,7 +132,7 @@ public class Task: _Task /// public init(weakified: Bool, paused: Bool, initClosure: InitClosure) { - super.init(weakified: weakified, paused: paused) + self._weakified = weakified let _initClosure: _InitClosure = { machine, progress, fulfill, _reject, configure in // NOTE: don't expose rejectHandler with ErrorInfo (isCancelled) for public init @@ -214,7 +202,8 @@ public class Task: _Task /// (NOTE: _initClosure has _RejectHandler as argument) internal init(weakified: Bool = false, paused: Bool = false, _initClosure: _InitClosure) { - super.init(weakified: weakified, paused: paused) + self._weakified = weakified + self.setup(weakified, paused: paused, _initClosure) } @@ -335,46 +324,6 @@ public class Task: _Task _initClosure(machine: self_.machine, progress: progressHandler, fulfill: fulfillHandler, _reject: rejectHandler, configure: configuration) - let userPauseClosure = configuration.pause - let userResumeClosure = configuration.resume - let userCancelClosure = configuration.cancel - - // add parentTask-pause/resume/cancel functionalities after retrieving user-defined configuration - configuration.pause = { [weak self_] in - userPauseClosure?() - - var task: _Task? = self_ - while let parentTask = task?._parentTask { - if parentTask._weakified { break } - - parentTask.pause() - task = parentTask - } - - } - configuration.resume = { [weak self_] in - userResumeClosure?() - - var task: _Task? = self_ - while let parentTask = task?._parentTask { - if parentTask._weakified { break } - - parentTask.resume() - task = parentTask - } - } - configuration.cancel = { [weak self_] in - userCancelClosure?() - - var task: _Task? = self_ - while let parentTask = task?._parentTask { - if parentTask._weakified { break } - - parentTask.cancel() - task = parentTask - } - } - } } @@ -407,6 +356,7 @@ public class Task: _Task let newTask = Task { [weak self] machine, progress, fulfill, _reject, configure in + var chainedTasks = [self!] var nextTask: Task = self! for i in 1...maxTryCount-1 { @@ -415,6 +365,8 @@ public class Task: _Task }.failure { _ -> Task in return Task(weakified: weakified, _initClosure: initClosure!) // create a clone-task when rejected } + + chainedTasks += [nextTask] } nextTask.progress { _, progressValue in @@ -425,9 +377,22 @@ public class Task: _Task _reject(errorInfo) } - configure.pause = { nextTask.pause(); return } - configure.resume = { nextTask.resume(); return } - configure.cancel = { nextTask.cancel(); return } + configure.pause = { + for task in chainedTasks { + task.pause(); + } + } + configure.resume = { + for task in chainedTasks { + task.resume(); + } + } + configure.cancel = { + // NOTE: use `reverse()` to cancel from downstream to upstream + for task in chainedTasks.reverse() { + task.cancel(); + } + } } @@ -540,8 +505,6 @@ public class Task: _Task } - newTask._parentTask = self - return newTask } @@ -621,8 +584,6 @@ public class Task: _Task } - newTask._parentTask = self - return newTask } @@ -705,17 +666,15 @@ public class Task: _Task } - newTask._parentTask = self - return newTask } - public override func pause() -> Bool + public func pause() -> Bool { return self.machine <-! .Pause } - public override func resume() -> Bool + public func resume() -> Bool { // always try `_performInitClosure` only once on `resume()` // even when to-Resume-transition fails, e.g. already been fulfilled/rejected @@ -725,7 +684,7 @@ public class Task: _Task return self.machine <-! .Resume } - public override func cancel(error: Error? = nil) -> Bool + public func cancel(error: Error? = nil) -> Bool { return self._cancel(error: error) }