Destination plugin calls `update(settings:)` multiple times

[tcldr]

Describe the bug
The included AppsFlyer example currently results in the plugin reaching an indeterminate state.

I'm not sure whether this is because the example needs to be re-architected, or because the plugin architecture is not behaving as it was designed.

My expectation is that the set-up for AppsFlyer should always be done prior to AppsFlyerLib.shared().start() being called.

However, with the current example this isn't guaranteed.

From my observations on an iPhone X and iPhone 7 Plus, I can see that update(settings:) is often called twice. This is the method that contains the AppsFlyer set-up code.

The first time it is called is prior to the plugin's applicationDidBecomeActive(application:) method being called. This would be what I expect, but at this point Segment doesn't appear to have loaded the AppsFlyer specific settings and so the majority of this method's execution, and therefore the AppsFlyer set-up, is skipped.

Shortly after, applicationDidBecomeActive(application:) is then called. This invokes AppsFlyerLib.shared().start(), but as the settings were missing previously, AppsFlyer is started without necessary setup having taken place.

So when update(settings:) is called a second time, this time with the associated AppsFlyer settings, the plug-in doesn't behave as expected. For example, the AppsFlyerLibDelegate method onConversionDataSuccess(_:) may not get called at all.

[tcldr]

I should also add that I moved the applicationDidBecomeActive(application:), openURL(_ url:) and receivedRemoteNotification(userInfo:) methods outside of the body of the update(settings:) method because afaict they are meant to be called as part of the destination's conformance to iOSLifecycle – but perhaps I'm missing something here.

[bsneed]

I'll have a look at this and see if I can get some more clarity here as to what's happening. Is your implementation relatively similar to the DestinationExamples demo app in the repo?

[tcldr]

Yeah, it's basically a copy and paste of the example. The only thing I really changed is moving the app lifecycle methods outside of the update(settings:) function body as I thought that was most likely unintentional :)

[bsneed]

Would love to hear your thoughts here @tcldr .

Digging into this further, much of it stems from the fact that some destination SDKs are singletons while others are not. While we don't want to prevent some SDKs from being able to update when settings are changed on segment.com, but we need to protect against those who can't function this way, such as AppsFlyer here.

I think the choices seem to boil down to ...

1. We only call update(settings:) once when we have settings. This would prevent some destinations from being able to update until the next time they're cold launched.
2. We add something to the DestinationPlugin protocol such that we avoid calling update(settings:) if it's a singleInstance destination.
3. We just deal with it on a per destination basis and leave DestinationPlugin's interface clean and the analytics code ignorant of this detail.

What are your thoughts?

ps. I saw what you were referring to re the functions inside applicationDidBecomeActive and will correct it when I have a PR for this ready.

[tcldr]

Hi @bsneed ,

Thanks for taking a look at this. Yes, I certainly see the difficulty. Of the options listed I would probably go with 3, but perhaps with an additional flag which leaves Plugin developers with no doubt that this is an initial launch vs a restart e.g. update(settings:isRestart:) or a dual API of applyInitial(settings:) and update(settings:).

However, I'm not sure how this solves the first launch scenario.

This is the situation where a user opens the app the first time after downloading it, so Segment is yet to cache its settings. This is difficult because at this point these plugins are yet to receive their settings, but there are also a bunch of potentially critical events being emitted. These are the most important for me as they contain things like attribution, on-boarding data, deep-links, etc.

In the case of plugins that enrich events, this will create inconsistent data as some site's will generate events with enriched data and others without. As a plugin may, or may not be set-up during launch, Identify calls will be missed, applicationDidBecomeActive events will be missed, etc.

Also, it becomes very difficult to design a plugin as you have to juggle all the various states the plugin could be in. If you need something to happen on applicationDidBecomeActive, but the dependency hasn't been configured, you'll need to create a flag for it and check it during update(settings:).

Really, I think Segment should hold-off sending any events until it receives its initial settings payload and then replay them through the pipeline once the plugins have had their initial set-up. In fact, I'm not sure Plugins should be directly listening for system events at all, I think they should probably all flow through the pipeline – so they can be replayed if it's required. A plugin can then do what it needs to when it receives an event from Segment, rather than the system. This is the easiest way that I can see to maintain consistency.

What do you think?

[bsneed]

Hi @tcldr,

It very much works like you describe already. Those changes are relatively recent on master, so not sure if you have them yet. StartupQueue is doing the storage/replay of events until we're in a running state.

update(settings: ) is called whenever we receive settings... these cases would be:

1. on first-launch-ever once the /settings endpoint has data.
2. on launch if we have settings cached. we'll still hit the /settings endpoint, so it would get called a 2nd time.
3. on return from background we'll hit the /settings endpoint again, so it would get called again.

The intent is to tell plugins when we've received settings. There's not really any state to manage other than in the case of a plugin using a singleton like Firebase or something, that you've already set that up, so don't do it again. Internally, we've decided to go with option 3 in the previous message above. I'll look and see if we can provide a param to give guidance on which of the above scenarios is happening.

[tcldr]

That’s great to hear! It would be a shame to miss those early identify/attribution events.

I know a lot of this is getting rapidly sorted out so I hope I’m not creating to much noise here.

[bsneed]

No, not at all! We're very appreciative of the feedback, questions and discussion. Helps us make it all better. :D