search

launchdarkly / android-client-sdk

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

LDClient.init doesn't really return a Future that will complete with the latest feature flag values #143

Open JinW-Airwallex opened 2 years ago

JinW-Airwallex commented 2 years ago

Is this a support request? No

Describe the bug I'm not sure if I misunderstand this but the doc about LDClient.init says: The result is a Future which will complete once the client has been initialized with the latest feature flag values.. However, it looks like even if I turn off the device's WiFi and let the network requests fail, calling get() on the returned Future will still succeed without any exception. Where is the so called latest feature flag values?

The goal I'm trying to achieve is to block users from accessing the app before we successfully load the feature flags from LD. But that doesn't seem to be supported by the SDK or am I missing something here?

What's more, let's just assume the network issue is temporary, it's actually impossible for app to retry the connection proactively because LDClient.init is only allowed to be called once as mentioned in #108! And the only other option is setOnline on the returned client which unfortunately doesn't have a callback or anything to notify the results.

To reproduce

  1. Turn off WiFi and call LDClient.init.
  2. Call get() on the returned Future.
  3. Check the Logcat and also monitor the pass of the get() without throwing.
  4. The allFlags() on the LDClient will return an empty map.

Expected behavior If the device is offline and the LDConfig specifies offline to be false, an exception should be thrown upon calling get() on the Future returned from LDClient.init.

Logs

2021-11-12 16:41:19.270 15942-16193/com.airwallex.internal W/DiagnosticEventProcessor: Unhandled exception in LaunchDarkly client attempting to connect to URI: https://mobile.launchdarkly.com/mobile/events/diagnostic
    java.net.UnknownHostException: Unable to resolve host "mobile.launchdarkly.com": No address associated with hostname
        at java.net.Inet6AddressImpl.lookupHostByName(Inet6AddressImpl.java:124)
        at java.net.Inet6AddressImpl.lookupAllHostAddr(Inet6AddressImpl.java:103)
        at java.net.InetAddress.getAllByName(InetAddress.java:1152)
        at okhttp3.Dns$Companion$DnsSystem.lookup(Dns.kt:49)
        at okhttp3.internal.connection.RouteSelector.resetNextInetSocketAddress(RouteSelector.kt:164)
        at okhttp3.internal.connection.RouteSelector.nextProxy(RouteSelector.kt:129)
        at okhttp3.internal.connection.RouteSelector.next(RouteSelector.kt:71)
        at okhttp3.internal.connection.ExchangeFinder.findConnection(ExchangeFinder.kt:205)
        at okhttp3.internal.connection.ExchangeFinder.findHealthyConnection(ExchangeFinder.kt:106)
        at okhttp3.internal.connection.ExchangeFinder.find(ExchangeFinder.kt:74)
        at okhttp3.internal.connection.RealCall.initExchange$okhttp(RealCall.kt:255)
        at okhttp3.internal.connection.ConnectInterceptor.intercept(ConnectInterceptor.kt:32)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
        at okhttp3.internal.cache.CacheInterceptor.intercept(CacheInterceptor.kt:95)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
        at okhttp3.internal.http.BridgeInterceptor.intercept(BridgeInterceptor.kt:83)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
        at okhttp3.internal.http.RetryAndFollowUpInterceptor.intercept(RetryAndFollowUpInterceptor.kt:76)
        at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.kt:109)
        at okhttp3.internal.connection.RealCall.getResponseWithInterceptorChain$okhttp(RealCall.kt:201)
        at okhttp3.internal.connection.RealCall.execute(RealCall.kt:154)
        at com.launchdarkly.sdk.android.DiagnosticEventProcessor.sendDiagnosticEventSync(DiagnosticEventProcessor.java:129)
        at com.launchdarkly.sdk.android.DiagnosticEventProcessor.lambda$sendDiagnosticEventAsync$0$com-launchdarkly-sdk-android-DiagnosticEventProcessor(DiagnosticEventProcessor.java:116)
        at com.launchdarkly.sdk.android.DiagnosticEventProcessor$$ExternalSyntheticLambda1.run(Unknown Source:4)
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:462)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:301)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:919)

SDK version 3.1.1

Language version, developer tools Android, Kotlin

OS/platform Android API 30

Additional context

louis-launchdarkly commented 2 years ago

Hello @JinW-Airwallex, thank you for waiting. The initialization behavior documented in the javadoc says the init call will return an already completed Future if the device is offline. Because there are mechanisms like local cache and default value, the intention is to not block the app user from using the app just because LDClient cannot reach the LaunchDarkly servers.

If you want to block the app until it establishes a connection with LaunchDarkly, we recommend using the getConnectionInformation method or registering a callback using registerStatusListener.

The ConnectivityManager class should manage the network transition for you and try to reconnect when network status changes so you do not need to manually retry.

Please let us know does the explanation and suggestion above work for you or not.

JinW-Airwallex commented 2 years ago

@louis-launchdarkly Thanks for replying.

Two things:

louis-launchdarkly commented 2 years ago

Hello @JinW-Airwallex

For the first point, the ConnectivityManager will sense the shift from Offline to Online, and retry the connection accordingly. You don't need to explicitly call setOnline() in this case.

For the second point, this behavior is not comparable with the iOS SDK - in iOS SDK, the startup behavior works in conjunction with a timeout parameter, and if the device is offline without setting Offline mode, the initialization will complete and not block the application.

JinW-Airwallex commented 2 years ago

@louis-launchdarkly Thanks for the reply.

if the device is offline without setting Offline mode, the initialization will complete and not block the application.

The difference is on iOS, the SDK returns a Boolean to indicate whether the connection was established. (So no need to have an async status listener) And it also allows us to close the client and reinit again. (which the Android doesn't allow) And the Android init method does accept a timeout parameter as well so it's definitely not consistent from my point of view.

For the first one, what I need is not an auto retry because it's too implicit to build a retry UI on top of it. Imagine if you show the user an error screen saying that the loading failed and then without the user clicking the retry button, the LDClient retried successfully in the background. I mean, it could work but it's not what we're after here.

JinW-Airwallex commented 2 years ago

To be fair, my conclusion is that the Android SDK doesn't support the scenario we're trying to build here: Blocking the users until the latest LD values are fully loaded and allow easy retry when requested by the user.

Even with the conversations we had I think the above is still true. We could hack it but the implementation will feel ugly. I mean, it's fair that if you guys decide that the scenario isn't a typical one you'd support. I just personally think it's a rather common pattern in the mobile industry. So really what I'm asking here is if you guys could spend sometime supporting this, it'd be much appreciated.

rubensousa commented 1 year ago

Is there any news here? We're migrating from another feature toggle provider and we need this feature as well. What's the reliable method to check if the configuration was loaded?

orafaaraujo commented 1 year ago

Hello, This would be helpful for us as well.

We are waiting for the flags to start the app. We are waiting for the Future.get() result meaning that all flags are fetched and ready to be consumed by the app. 

val future = LDClient.init(application, config, user)
try {
    future.get(INITIATION_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS)
} catch (exception: Exception) {
    error = exception
}

During the incident on Elevated client-side streaming errors we noticed by custom tracers that this doesn't affect the start-up on Android side, but it was on the iOS. On the Android side, the p90 continued for 600 milliseconds while iOS increased to 5 seconds, the limit of the timeout implemented in both platforms.

So we also feel that client start-up does not mean flags are ready to be used.

Stuart-campbell commented 1 year ago

+1 We want to move away from client defaults and perform a one-time guaranteed sync on the first install. Was easy enough on iOS (although still a little hacky) but doing so on Android seems like it would tie you to network status not the status of flags, which is going to open the door to race conditions.

Putting this on the roadmap would be a very nice to have.

jarrodrobins commented 1 year ago

This absolutely should be on the roadmap - trying to work out if the app has the latest flags should not be this difficult. I have noticed additional bizarre behaviour with this SDK where I'd argue it basically... lies?

I know LD does a lot of fancy stuff behind the scenes with streaming and polling - but I have no intention of ever 'observing' changes to LD flags - I need to know everything up front in one go when a user logs in to show/hide various features of the app. If in the extremely rare event I go and turn a flag off while a user is using the app, I don't particularly care. I want to be able to fetch once (per user login, not app instance) and be done with it. There does not appear to be an easy way to do this right now, and while my implementation works it's very far from ideal.

tanderson-ld commented 1 year ago

Sorry for the delay in getting back to you on this. We have been making wide spread changes to support Contexts (a more flexible structure than User). We have filed an internal ticket (202065) to investigate what it would take to make the Android SDK consistent with the iOS SDK with respect to this init behavior.

mikevercoelen commented 10 months ago

@tanderson-ld Any update on this? We never get feature flags back on Android, iOS works fine. We also have to specify a custom timeout to even make our app load LaunchDarkly. We're on "launchdarkly-react-native-client-sdk": "^8.0.1",

Note: our production app works fine, it's our local development client that doesn't like something.

tanderson-ld commented 10 months ago

@mikevercoelen, we are in the process of creating a pure JS version of our React Native SDK to decouple it from the Android and iOS SDKs and then we will be fixing portions of the Android SDK to have it be more in line with the behavior of the iOS SDK.

We never get feature flags back on Android, iOS works fine.

What version are you seeing this behavior on? Is this 100% reproducible? Are you by chance using hot reload? We have seen issues related to hot reload.

Note: our production app works fine, it's our local development client that doesn't like something.

What version are you using in production to help us get a clear picture?

mikevercoelen commented 10 months ago

@tanderson-ld I've just tried to toggle live reload, both on and off no flags are returned. And yes, this is happening 100% of the time.

We are using 8.0.1 in both dev and prod. We're on the latest Expo 49.0.15 and React Native 0.72.6

We've had this issue for quite a while now, on Android LaunchDarkly was even blocking the rendering of our app (it was getting stuck at client.configure) and we had to add a manual timeout of 1ms. This is probably related.