Make Segment.io flush asynchronous #259
Conversation
|
Some tests need to be fixed due to the new async nature of flush and tests depending on the previous sync. |
| @@ -113,47 +114,50 @@ public class SegmentDestination: DestinationPlugin, Subscriber { | |||
| } | |||
|
|
|||
| public func flush() { | |||
There was a problem hiding this comment.
It seems like this could be simplified to:
public func flush() {
flushQueue.async { [weak self] in
self?._flush()
}
}
// Existing flush (no need for this comment-- just making the point)
private func _flush() {
// existing code...
}
|
@cprince-foreflight can you give the branch a try and let me know the results? |
Just tried a smoke test of this branch with our app and am still seeing the expected Segment events in the Segment test debugger. So, no issues apparent. (Though it takes longer-- with builds to our beta testers-- to get real tests of the issue that brought this about. I.e., tests with the app in flight). Any ideas of when (a) this will be merged and (b) when a tagged release with this will happen? Thanks! |
|
@cprince-foreflight Some version of this likely sometime next week. We're having an internal dialogue about whether we want to make sync vs. async a choice at the API level for flush. There's scenarios that demand one or the other and the lib wasn't structured to take this into account. |
|
Closing this in favor of #269 |
It's my suspicion that file IO is the slow-down when calling flush() from the main thread with a significant number of events present. As such, in this fix async operation was done in the segment destination instead of the upper level analytics method to prevent unknowns from happening with device mode destinations. Those device mode destinations (now or in the future) may not be as thread-safe, or have expectations about running on the main thread, etc.
A future-todo would be to make asynchronous file iO standard and remove usage of FileManager and the like.