Fix dispatchSource #1344
Fix dispatchSource #1344
Conversation
* fix constraints warning by adding leading and trailing constraints for nested stackViews * remove print statement
| case resumed | ||
| } | ||
|
|
||
| private var state: State = .suspended |
There was a problem hiding this comment.
Curious why we need to manage this state ourselves?
There was a problem hiding this comment.
GCD timers can be somewhat sensitive. If you try and resume/suspend an already resumed/suspended timer, you will get a crash with a reason like:
BUG IN CLIENT OF LIBDISPATCH: Over-resume of an object
This tells us we have tried to resume an already resumed timer. We just need to balance the calls to suspend and resume, like the documentation states:
As a result, you must balance each call to dispatch_suspend with a matching call to dispatch_resume before event delivery resumes.
There was a problem hiding this comment.
Gotcha, thanks for the background here. This makes sense
There was a problem hiding this comment.
Can we include that link & those details in a comment?
There was a problem hiding this comment.
Sure, added: 3be7fd8
|
|
||
| class RepeatingTimerTests: XCTestCase { | ||
|
|
||
| func testTimer_resumesAndSuspends() { |
There was a problem hiding this comment.
Nitpick - we try to name tests like this format: test<MethodName>_{optional when<Condition>}_<ExpectedResult>
This test might be more readable as two separate tests. For ex: testResume_callsEventHandler or testSuspend_doesNotCallEventHandler
There was a problem hiding this comment.
Thanks for let me know, updated here: d4bb6aa
| /// Amount of time, in seconds. | ||
| private let timeInterval: Int | ||
|
|
||
| init(timeInterval: Int) { |
There was a problem hiding this comment.
Since we've separate out this logic can we remove the timeInterval property from BTAnalyticsService and set the default in this class? That way the timer logic is fully contained here and we are only instantiating it in BTAnalyticsService.
There was a problem hiding this comment.
Yeh, we can just hard code it to 15 in the RepeatingTimer class. BTAnalyticsService doesn't need to change the interval
There was a problem hiding this comment.
@richherrera wanted to make sure you saw this
There was a problem hiding this comment.
By removing timeInterval from class BTAnalyticsService and setting a default value in this class, we're mixing the "logic" again. Imagine we want to change the time interval from 15 seconds to 30, like in Android. I would expect to make this change in BTAnalyticsService, not RepeatingTimer. What do you think? Any ideas? What I came up with still looks like a magic number:
| init(timeInterval: Int) { | |
| init(timeInterval: Int = 15) { |
Any suggestions on how to set the value 15 without it looking like a magic number?
There was a problem hiding this comment.
I am not sure that I agree with this. BTAnalyticsService only responsibility should be sending events, it should not care about how often we send events at all. On the other hand the RepeatingTimer should control all time related logic including the interval of the timer. I don't think BTAnalyticsService needs to care how often events are sent only that it's sending events.
There was a problem hiding this comment.
Sure, I completely agree with you. I was thinking more about making this class usable in multiple places if needed, considering that in some other context, 15 seconds might be too short. But I'm happy to make the change. Do you think it would be good to put it in the initializer?
| For more information: https://developer.apple.com/library/archive/documentation/General/Conceptual/ConcurrencyProgrammingGuide/GCDWorkQueues/GCDWorkQueues.html#//apple_ref/doc/uid/TP40008091-CH103-SW8 | ||
| */ | ||
|
|
||
| func resume() { |
There was a problem hiding this comment.
This PR is great work Rich. I am wondering if we want to synchronize resume and suspends as well since analytics could be sent from different threads. We could use actor which would require awaits on function calls but we could also use serial dispatch queue. wdyt?
There was a problem hiding this comment.
Excellent suggestion. For now, I think DispatchSource is a task that runs periodically, so we could proceed without needing actor's. Also, based on Jax's implementation, the DispatchSource is created to run on the main thread. From what I see, the task of sending analytics can be on different threads, but both only enqueue events. The task of sending them is handled by the single instance of DispatchSource.
There was a problem hiding this comment.
Interesting. I didn't know that this line let timerSource = DispatchSource.makeTimerSource(queue: DispatchQueue.main) ensures that the handler is run on main as well.
Summary of changes
RepeatingTimerthat usesDispatchSource, moving all the logic from theBTAnalyticsServiceclass. This way, we can reuse the same logic wherever needed. This class ensures it can be properly deallocated from memory or the implementation of pause/resume functionality. Also,RepeatingTimeraims to fix an intermittent crash.Steps to reproduce the crash:
Xcode 15.4iOS 17.2Simulator iPhone 14This branch was created where the
APIClientandPaymentClientare instantiated almost at the same time, based on a potential implementation by the merchants. The steps to reproduce the crash are totokenizeand thencancel(crash is very intermittent, so you'll need to run the application multiple times and follow the tokenization process repeatedly).Update: This branch includes the changes from the branch that reproduces the crash, as well as the changes from this branch.
Checklist
Authors