Flush running on main thread and may take a long time

[cprince-foreflight]

Describe the bug
We recently have switched over to using your Segment Swift-based library (from your Objective-C library).
We have code in our app that checks whether the main thread is blocked for relatively long periods of time.
We have noticed that:
(a) flush operations take place on the main thread, and
(b) those flush operations can take significant amounts of time. E.g., > 3 seconds.

To Reproduce
Steps to reproduce the behavior:

1. Turn off all networking on device. E.g., airplane mode.
2. Log numerous Segment events.
3. Turn networking back on on device.
4. Observe that flush takes place on main thread and that main thread is blocked for a relatively long period of time.

Expected behavior
Given that the flush can take a relatively long period of time (roughly proportional to the number of events pending to be sent), I expect the flush to not take place on the main thread-- which will lock up the UI if the app is in the foreground.

Screenshots
N/A

Platform (please complete the following information):

• Library Version in use: 1.4.7
• Platform being tested: iOS

Additional context
Our app serves pilots flying aircraft and in this context the devices typically do not have a networking connection, but Segment events are logged during flights. When the pilot lands, and again has networking, Segment events get uploaded.

Questions

1. We are planning to work around this issue by using custom flush policies which directly call the flush operation (e.g., as in IntervalBasedFlushPolicy), but which wrap the flush call in use of a background thread. Do you see any problematic consequences of this strategy in terms of the broader Segment library and system? So far in initial testing it works with no problem.

2. Would you be open to a PR to change this? The simplest approach seems to me to wrap the bulk of the flush code in a block executing on a background thread. Example:

    public func flush() {
        // only flush if we're enabled.
        guard enabled == true else { return }
        
        DispatchQueue.global(qos: .background).async {
            apply { plugin in
                if let p = plugin as? EventPlugin {
                    p.flush()
                }
            }
        }
    }

[bsneed]

Thanks @cprince-foreflight for sharing that use case! I was curious if this would work ok. The calls were all made to be thread safe, so you could call them from any thread as needed. However, your use case provides a strong reason to make flush work this way. Since I was curious, I already tested this here and it works well. My only addition would be to make self weak, as you'll need it when you call apply. I can either commit and PR here, or you can do the PR so you get the contribution credit. Lemme know which, and I'll get it released for you.

[cprince-foreflight]

Thanks! I have no need for credit.
I did have one further thought--which might be overly general. Would it be useful to indicate the queue that the flush runs on in the Configuration? That could even default to DispatchQueue.main if there's concern that someone is relying on that (which seems unlikely).

[justin-foreflight]

I would follow the guidance here (https://developer.apple.com/forums/thread/711736) and not use a global concurrent thread (if that is part of the upcoming change). Just using a custom serial queue would be preferred.

[dannys42]

Depending on the intent behind flush, it may also be prudent to use dispatch sources to coalesce multiple "flush" events in case another flush happens while a long-running one is already taking place.

[bsneed]

@cprince-foreflight @justin-foreflight @dannys42 thanks for all the input! 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. Please have a look at the PR here. Would be nice to see if it works for you before merging.

#259

[cprince-foreflight]

OK-- thanks. We've had related discussions in our team and it would be useful for us to be able to pass in our own DispatchQueue on which to run the flush operation.

[bsneed]

PR #269 supercedes the previous PR and will be released soon to address this issue.

[tristan-warner-smith]

We just integrated the SDK this week and we're getting app hangs on start so we're keen to see this go through. We're forking it now and applying PR #269 until it's merged, we'll let you know if we encounter any issues in our testing @bsneed.

[bsneed]

Thanks @tristan-warner-smith! I'm seeing some issues with this still and making some additional changes in #270

[bsneed]

#270 merged. I've got a couple more fixes to do elsewhere, and will make a release.

[cprince-foreflight]

Thanks for the 1.5.0 release. Just incorporating it now into our app. Looking good so far. I think this issue can be closed?

[bsneed]

Thanks for the reminder!