-
Notifications
You must be signed in to change notification settings - Fork 429
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
Conversation
Retain timers in `QueueScheduler` so that they will not be cancelled if the user doesn’t retain the returned `Disposable`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. 😸
The timers would be released upon deinitialisation of the scheduler, so there should have no leakage AFAICU.
expect(false).to(equal(true), description: "timer not cancelled on disposal") | ||
} | ||
|
||
scheduler.schedule(after: Date(), interval: .milliseconds(10), leeway: .seconds(0)) { |
There was a problem hiding this comment.
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.
var count = 0 | ||
let timesToIncrement = 3 | ||
|
||
// Start two repeating timers, dispose the first, and ensure only the second runs. |
There was a problem hiding this comment.
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.
Sources/Scheduler.swift
Outdated
@@ -193,8 +193,14 @@ public final class QueueScheduler: DateScheduler { | |||
|
|||
public let queue: DispatchQueue | |||
|
|||
private let lock: Lock | |||
|
|||
private var timers: [DispatchSourceTimer] |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
CHANGELOG.md
Outdated
@@ -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. |
There was a problem hiding this comment.
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.
// Start two repeating timers, dispose the first, and ensure only the second runs. | ||
|
||
disposable.inner = scheduler.schedule(after: Date(), interval: .milliseconds(10), leeway: .seconds(0)) { | ||
expect(false).to(equal(true), description: "timer not cancelled on disposal") |
There was a problem hiding this comment.
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")
.
|
||
expect(count) == 0 | ||
|
||
scheduler.queue.resume() |
There was a problem hiding this comment.
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.
A private wrapper for |
Sources/Scheduler.swift
Outdated
private let value: DispatchSourceTimer | ||
|
||
fileprivate var hashValue: Int { | ||
return value.hash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use ObjectIdentifier(value).hashValue
. The Dispatch protocol hierarchies do not inherit from NSObjectProtocol
on platforms w/o ObjC interop.
Sources/Scheduler.swift
Outdated
timers.modify { timers in | ||
timers.append(timer) | ||
timers.insert(wrappedTimer) | ||
} | ||
|
||
return AnyDisposable { | ||
timer.cancel() | ||
|
||
self.timers.modify { timers in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we weakify self
here?
count += 1 | ||
|
||
if count == timesToRun { | ||
disposable1.dispose() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be disposable2
. The macOS CI seems failing because of this.
expect(count) == 0 | ||
|
||
scheduler.queue.resume() | ||
expect{count}.toEventually(equal(timesToIncrement)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC expect
uses @autoclosure, so it is already implicitly captured.
|
||
// This expectation should take about 2.0 * interval to be fulfilled, and that's | ||
// enough time to ensure that the first timer was actually cancelled. | ||
expect{count}.toEventually(equal(timesToRun)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto for @autoclosure.
Hmm
|
I think I see the bug, but strange that it’s not failing on my machine. I guess there’s a race condition where the expectation sees that it got incremented to 3 and succeeds before it gets incremented that last time. I will fix when I get back to my PC. |
CI has limited concurrency (1 logical processor IIRC). |
Specify `pollInterval` on the asynchronous expectation to avoid false positive test success.
Is there a test still failing on Linux? I'm having a tough time understanding the log. |
OK, I found the error on Linux:
Will fix when I get home tonight. |
`DispatchSourceTimer` doesn’t inherit from `NSObjectProtocol` on Linux. Basing hashing and equality on the wrapper is sufficient because the same wrapper instance can be used insert and remove from the `Set` of retained timers.
Stumped by this one:
|
Usually that's downstream from whatever the actual error was. |
Seems to be working now. Judging by this Stackoverflow thread, that's an error that can occur randomly. |
Addresses #582. Retain timers in
QueueScheduler
so that they will not be cancelled if the user doesn’t retain the returnedDisposable
.Note that the test leaks a timer, but I'm not sure how else to test that the timer continues running even when the
Disposable
is not retained.Checklist