Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix schedule(after:interval:leeway:) disposal #584

Merged
merged 10 commits into from
Jan 5, 2018
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# master
*Please add new entries at the top.*

1. Fixed `schedule(after:interval:leeway:)` being cancelled when the returned `Disposable` is not retained.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the PR no & your handle.


# 3.1.0-rc.1
1. Fixed a scenario of downstream interruptions being dropped. (#577, kudos to @andersio)

Expand Down
14 changes: 14 additions & 0 deletions Sources/Scheduler.swift
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,14 @@ public final class QueueScheduler: DateScheduler {

public let queue: DispatchQueue

private let lock: Lock

private var timers: [DispatchSourceTimer]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may use Atomic. I'd prefer Set which fits the scenario here (insert/removal only).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DispatchSourceTimer isn't Hashable, so I think it would have to be an NSMutableSet. Is that an improvement over using a Swift array?


internal init(internalQueue: DispatchQueue) {
queue = internalQueue
lock = Lock.make()
timers = []
}

/// Initializes a scheduler that will target the given queue with its
Expand Down Expand Up @@ -340,8 +346,16 @@ public final class QueueScheduler: DateScheduler {
timer.setEventHandler(handler: action)
timer.resume()

lock.lock()
timers.append(timer)
lock.unlock()

return AnyDisposable {
timer.cancel()

self.lock.lock()
self.timers = self.timers.filter { !$0.isEqual(timer) }
self.lock.unlock()
}
}
}
Expand Down
28 changes: 28 additions & 0 deletions Tests/ReactiveSwiftTests/SchedulerSpec.swift
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,34 @@ class SchedulerSpec: QuickSpec {
scheduler.queue.resume()
expect{count}.toEventually(equal(timesToRun))
}

it("should repeatedly run actions after a given date when the disposable is not retained") {
let disposable = SerialDisposable()

var count = 0
let timesToIncrement = 3

// Start two repeating timers, dispose the first, and ensure only the second runs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd probably be better off as two separate test cases. One for cancellation, while another for not retaining the disposable.


disposable.inner = scheduler.schedule(after: Date(), interval: .milliseconds(10), leeway: .seconds(0)) {
expect(false).to(equal(true), description: "timer not cancelled on disposal")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may replace this with fail("message").

}

scheduler.schedule(after: Date(), interval: .milliseconds(10), leeway: .seconds(0)) {
Copy link
Member

@andersio andersio Dec 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might scope the scheduling call in a function, so that the disposable is guaranteed to be deinitialised.

expect(Thread.isMainThread) == false

if count <= timesToIncrement {
count += 1
}
}

disposable.dispose()

expect(count) == 0

scheduler.queue.resume()
Copy link
Member

@andersio andersio Dec 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the cancellation test, I'd wait for 1.5-2x the interval after resuming the queue so that the test case could actually fail.

expect{count}.toEventually(equal(timesToIncrement))
}
}
}

Expand Down