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::close() does not clean up the internal instances map. #108

Closed
valeriyo opened this issue Sep 24, 2020 · 8 comments
Closed

LDClient::close() does not clean up the internal instances map. #108

valeriyo opened this issue Sep 24, 2020 · 8 comments

Comments

@valeriyo
Copy link

valeriyo commented Sep 24, 2020

This sequence:

  • init(...)
  • close()
  • init(...)

does not work. The second init() call will succeed, but the LD library will be non-operational.

This is happening because, LDClient has a static instances map, which isn't cleared by close():

    private static Map<String, LDClient> instances = null;

    ...

    public void close() throws IOException {
        LDClient.closeInstances();
    }

    ...

    private static void closeInstances() {
        for (LDClient client : instances.values()) {
            client.closeInternal();
        }
    }

So, the second init() will find instances and will bail out early:

        if (instances != null) {
            Timber.w("LDClient.init() was called more than once! returning primary instance.");
            return new LDSuccessFuture<>(instances.get(LDConfig.primaryEnvironmentName));
        }

Please do instances = null at the end of closeInstances() so that library can be re-initialized. Thanks.

@valeriyo
Copy link
Author

valeriyo commented Sep 24, 2020

Currently, I have:

        ldClient.close()

        // the workaround to clear internal LDClient state:
        ldClient.javaClass
            .declaredFields
            .firstOrNull { it.name == "instances" }
            ?.apply {
              isAccessible = true
              set(null, null)
            }

it works, but I would rather not have this hack in my code. Thanks.

@gwhelanLD
Copy link
Contributor

Hi @valeriyo,

Could you let us know your use case for re-initializing the client? In general this isn't a feature we consider supported on our mobile SDKs, as the SDK lifecycle is considered tied to the application's lifecycle.

Thanks,
@gwhelanLD

@valeriyo
Copy link
Author

valeriyo commented Oct 8, 2020

Our app has an "environment" switch for Production vs Staging (with our server), and those are mapped to two Launchdarkly environments - which map to two different keys. So, when the app switches to another environment, everything that is environment-specific needs to be cleaned up. What's unusual about that?

@gwhelanLD
Copy link
Contributor

This specific case is actually addressed separately with the multi-environment feature. Take a look at the docs here. You can set either Production or Staging as the primary instance, and identify the other with LDConfig.setSecondaryMobileKeys(Map<String, String>). Then the secondary environment can be retrieved with LDClient.getForMobileKey(String).

Hope this helps!
@gwhelanLD

@valeriyo
Copy link
Author

valeriyo commented Dec 7, 2020

@gwhelanLD No, the multi-environment feature doesn't mitigate the fact that close() doesn't clean stuff up.

@Dill-Lan
Copy link

Dill-Lan commented Mar 9, 2021

Having the same issue with the project I'm on. We need to be able to dynamically configure the LD config and reinitialise the client each time it updates so that the new configuration takes effect.

Is there any reason why instances can't be set to null @gwhelanLD? I'd prefer to not have to reflectively set it to null like @valeriyo has resorted to.

Thanks

@valeriyo
Copy link
Author

valeriyo commented Jun 30, 2022

Alright, this is brain-dead. Is it so hard to add a line to make this right?

    private static void closeInstances() {
        for (LDClient client : instances.values()) {
            client.closeInternal();
        }
        instances = null // THIS F**KING THING RIGHT HERE TO CLEAN SH*T UP
    }

I was considering LD for a new project, but I will look elsewhere because you suck. Bye.

@eli-darkly
Copy link
Contributor

Fixed this in the 3.1.8 release.

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

4 participants