Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LDClient.isInitialized doesn't evaluate to true immediately after LDClient.init() finishes with a timeout #284

Open
hikaritenchi opened this issue Dec 17, 2024 · 5 comments

Comments

@hikaritenchi
Copy link

Is this a support request?
Shouldn't be, but I think this is either a bug or some documentation should be changed.

Describe the bug
Immediately after LDClient.init(a, conf, cont, 14) returns, I expect LDClient.isInitialized to return true, but it does not do so for a nontrivial amount of time, especially since I specified a nonzero timeout.

To reproduce

Timber.e("start: ${System.currentTimeMillis()}")
val client: LDClient = LDClient.init(application, ldConfig, context, 14)
Timber.e("before: ${System.currentTimeMillis()}")
Timber.e("init: ${client.isInitialized}")
while(!client.isInitialized) {}
Timber.e("after: ${System.currentTimeMillis()}")
Timber.e("client init: ${client.isInitialized}")

Expected behavior
I expect LDClient.isInitialized to return true immediately after LDClient.init() is called with a nonzero timeout.

Logs
2024-12-17 13:08:22.140 E start: 1734458902140
2024-12-17 13:08:22.142 E before: 1734458902141
2024-12-17 13:08:22.142 E client init: false
2024-12-17 13:08:22.441 E after: 1734458902440
2024-12-17 13:08:22.442 E client init: true

SDK version
5.5.0

Language version, developer tools
I think the only relevant information is Java 21 and Gradle 8.9. This was tested on the Android sample app.

OS/platform
Android 11

Additional context
If this isn't actually a bug, then I'd like to request some documentation updates because it's easy to confuse the fact that isInitialized shouldn't be true after init() finishes. I can imagine a scenario where passing 0 for the wait would cause this situation to happen, but I've indicated that I am willing to wait for init().

@tanderson-ld
Copy link
Contributor

tanderson-ld commented Dec 17, 2024

Hi @hikaritenchi, thank you for taking the time to gather those timestamps. I'm happy to help. Without more info, this may be expected behavior. Version 5.5.0 added some polling interval logic that may be at play here.

  1. Could you provide your LDConfig creation code?
  2. Is this 100% reproducible?
  3. Do you see the same behavior on version 5.4.0?

Theories:

  • Polling mode is used and the polling interval is longer than the time between the last time the application ran and was restarted. This will result in init kicking off the datasource internally, but it will wait a moment to not make requests too quickly.
  • The internet connection was not available for a moment, but then became available later. This would likely be eliminated as a theory if the internet is known to be reliable and stable with high certainty.

@hikaritenchi
Copy link
Author

Hi @tanderson-ld, I think I figured out the problem. I was using a Hilt-provided LDClient which had the wait set to 0, so trying to init() again with a different timeout wasn't working. These logs feel expected for a wait of 0: an instant return from init() but it then takes ~300ms for isInitialized to return true. I believe it is as you said, expected behavior.

When I changed the Hilt LDClient to have a wait of 1, it returned isInitialized true instantly with my setup (e.g. good internet connection). The Hilt setup I had was to have LDClient be provided in the constructor of a ViewModel and use it immediately in the init{} block. I suspect this wouldn't have happened if init() was called in the Application class and then used in the ViewModel.

I've spent a long time shortening this response from my previous long drafts, but I feel like the crux of the problem that I had was a disconnect between:

  • After init() returns, isInitialized should immediately return true. (not true)
  • isInitialized represents connecting to the servers. (unexpected language to me)
  • The LDClient returned by init() is usable even if isInitialized == false for after-first-run situations because the values are cached from the previous run. (makes sense, but now it feels like the timeout parameter is confusing and unnecessary?)
  • How a non-zero timeout affects all of the above.

So while I agree that this is expected behavior, it is not explicitly explained in the documentation, which is what started this multi-day adventure for me.

I would love to see a new section of the SDK reference docs for first run situations, or mention the distinction between the disconnects I referenced above, or just better organized in general so the information is not so easily glossed over, requiring extra interpretation.

Additional documentation in the javadoc would be very welcome as well.

Other wishlist items I had:

  • Show registerFeatureFlagListener in the reference docs.
  • Where should we call init(), Application class?
  • Hilt DI and init{} blocks

I'm happy to attempt to write some of this documentation if that would be helpful.

@tanderson-ld
Copy link
Contributor

tanderson-ld commented Dec 18, 2024

You are certainly not the first to face this confusion around isInitialized and there is room for improvement on our end. I will take some time to review the documentation with one of our technical writers and hopefully we can incorporate your feedback. Thank you for taking the time to provide detail.

I recommend initializing the SDK in a class that extends the Android Application class as the LDClient instance will be globally useable within your application and there are likely components of your application that want to use feature flags that don't have UI/ViewModel components, such as a background service.

@tanderson-ld
Copy link
Contributor

tanderson-ld commented Dec 19, 2024

Is this clarifying?

    /**
     * This returns true if the client has successfully connected to LaunchDarkly and received feature flags after
     * {@link LDClient#init(Application, LDConfig, LDContext, int)} was called, or if the client has been placed in
     * offline mode.
     *
     * This returns false if the client is unable to retrieve feature flag data from LaunchDarkly services. If the
     * client has not connected to LaunchDarkly within the start wait time provided to 
     * {@link LDClient#init(Application, LDConfig, LDContext, int)}, then this will return false (even if the SDK has
     * cached feature flags).
     *
     * @return true if the client is able to retrieve flag data from LaunchDarkly or offline, false if the client is unable to retrieve flag data.
     */
    boolean isInitialized();

A follow up to this part: The LDClient returned by init() is usable even if isInitialized == false for after-first-run situations because the values are cached from the previous run. (makes sense, but now it feels like the timeout parameter is confusing and unnecessary?)

The need to use the timeout really depends on the customer use case and the app architecture. A well designed app will be reactive to feature flag changes and will not need to wait at the start to get feature flags. When the SDK gets the latest data, the app will fluidly handle the change. The reality is that can be very difficult to accomplish, even in a new codebase with that as a goal from the start of the project. Most customers have architectures that prefer to have the flags up front and then kept stable. For those customers, they typically make a tradeoff of getting feature flags in parallel to other startup tasks/requests and will wait at most a second or two. After that, they choose to proceed into the app with cached values or if there are really critical feature flags, they may block the user from getting into the app (very rare).

@hikaritenchi
Copy link
Author

Yes, I believe that is sufficient clarification for isInitialized(). Obviously for my specific use case, it could be optimized, but I also understand that there are many other consumers of the SDK, not just me, so I really appreciate the javadoc update!

As for my comment, and in particular the part in parenthesis, that was more so me thinking out loud with regards to my specific implementation. I agree that these kinds of "I need the freshest data before the app can start!" requirements are very difficult to accomplish, and it's very helpful to have a timeout built in to the SDK rather than having to deal with potentially confusing Java Futures, especially since the deprecation reasoning is to prevent accidental blocking.

I'm guessing the equivalent strategy might be to have a 0 timeout and attach a LDStatusListener to receive a callback for a successful connection.

For our use case, we will probably try and force init() at the Application level instead of the lazier Hilt injection I was using before, as well as the registerFeatureFlagListener() method to get streaming updates if necessary, and simple xVariation() calls for values that don't require streamed changes.

I'm comfortable with this issue being closed alongside the documentation updates being merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants