launchdarkly / android-client-sdk

LaunchDarkly Client-side SDK for Android
Other
45 stars 23 forks source link

Allow disabling all forms of polling #189

Open byencho opened 1 year ago

byencho commented 1 year ago

Is your feature request related to a problem? Please describe. It is not currently possible to disable all polling, which is something we'd like to do because:

  1. We only want to get a single set of new values on app startup and do not want to receive any new ones.
  2. There is a bug related to polling that is causing us some trouble (https://github.com/launchdarkly/android-client-sdk/issues/188) and the only solution is to set a very large polling interval. However, because int is used instead of long to represent milliseconds, the largest value we can choose is theoretically Int.MAX_VALUE, which is ~24 days.

Describe the solution you'd like We would like to be able to choose to disable all polling with a configuration call like disablePolling(true).

Describe alternatives you've considered Using very large polling intervals, which feels a bit arbitrary.

Additional context N/A

eli-darkly commented 1 year ago

Do I understand correctly that you do not want to use streaming mode either, because you explicitly don't ever want to receive any flag updates after startup (as opposed to just "receiving flag updates isn't important to us")?

What you're describing sounds less like "disable all polling", and more like "use polling, but only do it once." That may sound nitpicky, but I want to make sure I understand the intention, and to clarify that in the LaunchDarkly model there are really only two ways the SDK can acquire flags: by opening a long-lived connection to the streaming service, or by calling the polling service to get a snapshot of the flag data. So if you disabled all polling, it would never acquire flag data.

If your main concern was the AlarmManager-related bug, then the workaround would be to use the default of streaming mode but disable background polling; then AlarmManager would never be used, but then you could potentially receive flag updates after startup, so that's why I wanted to be clear on how strong of a requirement item 1 is for you. That's a somewhat unusual use case for LD, since normally one of the main selling points of a feature flag system is that flags can be changed. And the AlarmManager bug, assuming it is real and reproducible, is something we would absolutely want to fix anyway, regardless of whether we would add a new "poll, but only once" mode.

eli-darkly commented 1 year ago

I should also clarify that implementing such a mode wouldn't simply be a matter of telling it not to set an AlarmManager alarm. The SDK also has logic for transitioning from background to foreground mode, or from "network wasn't available" to "network is available", and (since currently the overall design of the SDK is based on the assumption that customers do want to receive updates at some point) it regards those as appropriate times to ask LaunchDarkly for new flag data. So we would need to modify that logic too. I guess it would be more or less equivalent to "after startup, behave as if the SDK is permanently offline (except for sending analytics events)".

byencho commented 1 year ago

@eli-darkly Yeah that's a good point that I should clarify: I just don't want scheduled, repeated polling. Basically, I just want to eliminate the need for any use of the AlarmManager. I do want new values to be fetched after initialization (or some time after initialization when all conditions are met). But I do not want to use streaming as I'm not allowed to use an active streaming connection like that (in order to minimize battery usage). Basically, I want there to be a single network request to fetch updates and that's it. And that is not currently possible.

As for fetching updates when foregrounding or regaining a network connection, I could go either way on those. If I have one fresh set of values already I don't necessarily need those so they represent unnecessary requests. I feel like those should only be relevant when streaming is enabled (since you'd miss those values otherwise).

eli-darkly commented 1 year ago

But I do not want to use streaming as I'm not allowed to use an active streaming connection like that (in order to minimize battery usage). Basically, I want there to be a single network request to fetch updates and that's it.

... As for fetching updates when foregrounding or regaining a network connection, I could go either way on those. If I have one fresh set of values already I don't necessarily need those so they represent unnecessary requests. I feel like those should only be relevant when streaming is enabled (since you'd miss those values otherwise).

I think that last statement is a slight misunderstanding about the point of the logic that I described, so even though this may not be relevant to your particular use case, I'll clarify a bit more.

In more typical use cases, if an application disables streaming, it's because they don't want to (or can't, e.g. due to limitations of a network gateway) maintain an active connection to get flag updates immediately, but they still do want to get flag updates eventually. So, the regular polling interval (when in the foreground) is set to some reasonably frequent time value... and the background polling interval is set to some much higher time value.

In that case, if the app has been in the background for a while, and then it transitions into the foreground, it's likely that if any poll requests have happened in the background, they were not recent. Therefore, it makes sense for the SDK to do a new poll now, and then continue with the regular foreground polling interval.

Similarly, if the network has been unavailable, and then the network becomes available, it's likely that we have not obtained fresh flag values recently so it makes sense to do so. (Again, here I'm talking about the typical case where the customer does want to have reasonably fresh flags— just to clarify why it's not really true that this logic "should only be relevant when streaming is enabled".)

I should also clarify one thing about how streaming works in the SDK, since you mentioned not wanting to have "an active streaming connection" specifically to minimize battery usage. "Active" in this case doesn't mean there is constant or even frequent traffic on the connection; the SDK never sends data over the connection after opening it, and unless there is actually a flag update, LD sends nothing except for a two-byte "heartbeat" every 3 minutes. And if there is a flag update, it sends only the result of the flag that was changed (rather than the whole set of flags, as the polling endpoint does). So streaming is significantly more efficient, in terms of bandwidth and battery usage, than repeated polling. The reason the SDK has an option for disabling streaming isn't to support a use case of "I don't ever want to receive flag updates"— we didn't anticipate such a use case— but to handle cases where the network infrastructure simply won't allow such a connection.

byencho commented 1 year ago

@eli-darkly Sure, that all makes perfect sense. And I completely understand that streaming is much more efficient than polling. I personally advocated for the streaming implementation but was forced to disable it long ago. (We actually also have an SSE implementation that's based on an early version of https://github.com/launchdarkly/okhttp-eventsource so I'm definitely aware of the details of how this all works). I'm just passing along my requirements to see if there's anything you can do to help out with them: we don't want streaming or background polling or foreground polling. We want to sync a single time once all conditions make that possible. And in that case, we don't really require syncing again at any later time since we aren't "missing" anything when in the background or when we have no network connection.

For context, we're trying to reduce every possible network request we can and we also don't have a good use case for updating flag values in real time. I understand that this may not be typical. If this does not make sense to add as a feature, we'll just live with our Int.MAX_VALUE polling period.