Add a flushInterval configuration option
#767
Conversation
Codecov Report
@@ Coverage Diff @@
## master #767 +/- ##
==========================================
+ Coverage 85.54% 85.71% +0.16%
==========================================
Files 52 52
Lines 2588 2618 +30
==========================================
+ Hits 2214 2244 +30
Misses 374 374 |
| @@ -109,17 +109,44 @@ - (id)initWithAnalytics:(SEGAnalytics *)analytics httpClient:(SEGHTTPClient *)ht | |||
| if ([NSThread isMainThread]) { | |||
| [self setupFlushTimer]; | |||
| } else { | |||
| dispatch_sync(dispatch_get_main_queue(), ^{ | |||
| dispatch_async(dispatch_get_main_queue(), ^{ | |||
There was a problem hiding this comment.
This one is interesting. After updating Xcode I started getting random deadlocks when testing, even on a clean repo on origin/master without any changes and DerivedData nuked.
The debugger showed that two threads (main and our serial queue) are locking the main queue :
Because we just need to set the timer to let ARC keep a reference on it I replaced the code to use async instead of sync, which should prevent any deadlock on this section. What do you think @f2prateek?
There was a problem hiding this comment.
I actually think we've gone back and forth on this. I can try digging up the history, you might be able to find it via git blame as well.
| [self setupFlushTimer]; | ||
| }); | ||
| } | ||
|
|
||
| // Observe changes to flushInterval and update the timer | ||
| [self.configuration addObserver:self |
There was a problem hiding this comment.
Figured we might want to observe this using KVO instead of having cyclic references between Configuration and Analytics. Let me know if you think this is overkill.
fathyb
force-pushed
the
feat/configure-flush-interval
branch
from
f6901b4 to
c6c763b
Compare
There was a problem hiding this comment.
This change is a bit bigger than I expected. I think we should narrow it down a bit more, and avoid changing too much logic without scoping out the impact in a bit more detail.
I was thinking all we need to change here would be not hardcode 30 here https://github.com/segmentio/analytics-ios/blob/master/Analytics/Classes/Internal/SEGSegmentIntegration.m#L122, and pull it from the configuration instead.
We can still do bigger changes if we need to but I think it should be a different PR/discussion. What do you think?
Ref: LIB-443
fathyb
force-pushed
the
feat/configure-flush-interval
branch
from
9197f06 to
fcc084b
Compare
|
@f2prateek you're right, I was a bit worried about that. I pushed a simpler version with no observers and no logic changes |
| /** | ||
| * The amount of time to wait before each tick of the flush timer. Smaller values will make events delivered in a more real-time manner and also use more battery. `30` by default. | ||
| */ | ||
| @property (nonatomic, assign) NSTimeInterval flushInterval; |
There was a problem hiding this comment.
Can someone set this to less than < 0?
There was a problem hiding this comment.
I actually think something too low (1ms) may also be problematic. We should just say something like "Don't set this below 10seconds".
There was a problem hiding this comment.
interval
The number of seconds between firings of the timer. If interval is less than or equal to 0.0, this method chooses the nonnegative value of 0.1 milliseconds instead.
I added this note :
A value smaller than 10 seconds will seriously degrade overall performance.
| @@ -61,6 +61,10 @@ typedef NSMutableURLRequest *_Nonnull (^SEGRequestFactory)(NSURL *_Nonnull); | |||
| */ | |||
| @property (nonatomic, assign) NSUInteger flushAt; | |||
|
|
|||
| /** | |||
| * The amount of time to wait before each tick of the flush timer. Smaller values will make events delivered in a more real-time manner and also use more battery. `30` by default. | |||
There was a problem hiding this comment.
30 seconds will be clearer than just 30 I think
* Add a `flushInterval` configuration option Ref: LIB-443 * Improve property documentation
* Add a `flushInterval` configuration option Ref: LIB-443 * Improve property documentation
* Add a `flushInterval` configuration option Ref: LIB-443 * Improve property documentation
Ref: LIB-443