Closed jpinho closed 4 months ago
Hello @jpinho,
Thank you for the issue.
We do recommend using promise.race currently to control the user specified timeout.
Something more like:
await Promise.race([
client.waitForInitialization(),
new Promise((resolve) => setTimeout(resolve, 3000)),
]);
If waitForInitialization
isn't returning immediately for an offline client, then that is problematic. Multiple calls to client.waitForInitialization
should also be fine because it caches a promise that remains in the resolved state.
The SDK only fails to initialize under certain situations, a 500 is not such a condition and it will continue to retry using an exponential backoff with jitter. Something like a 404 would be a terminal condition.
I've entered the issue as well, as some people would prefer a built-in timeout.
As a reminder this repository is not the home of development for the launchdarly node SDK at this point in time.
The current node SDK implementation is here: https://github.com/launchdarkly/js-core/tree/main/packages/sdk/server-node
With the latest version being the 9.x release.
Thanks, Ryan
Filed internally as 231798
Thanks @kinyoklion for your answer! I made some points here as well https://github.com/launchdarkly/node-server-sdk/issues/288#issuecomment-1943667112
Touching the same point again, LD resiliency should definitely be improved:
A FF client should never throw runtime exceptions ever, for any reason; instead Log.error should be preferred, and if failing to evaluate a flag, then by default false
should be returned. Up to the applicational code to deal with that then.
An initialisation attempt on a given timeout, should be provided by the client itself 💯
Not complying to these two points, means customers such as myself at epilot, have to come up with their own Shared Library of epilot-launch-darkly-sdk
(a wrapper) that adds resiliency and helps us avoid having to repeat that resiliency code across our over 10+ micro services.
Would love to contribute to the codebase if it's open to outside contributors. Let me know how.
@jpinho This, and the new repository, are open source and have an included CONTRIBUTORS guide you can reference.
Adding a timeout to initialization I would be open to, and we do support in some SDKs.
Please note this repository itself isn't where you would make the contribution. It would be here: https://github.com/launchdarkly/js-core/tree/main/packages/sdk/server-node
Version 9.4.0 and newer of @launchdarkly/node-server-sdk
now supports an optional timeout in waitForInitialization. Additionally in a future (major) version the timeout will start to have a default or will not be optional.
Describe the bug Method
waitForInitialization()
will never resolve when LaunchDarkly APIs fail due to 5XXTo reproduce Mock one of your APIs into 5XX, sample with msw below
Expected behavior LD clients don't want to wait forever for a FF service. Therefore a timeout must exist to let applications bail out of the LD waitForInitialization method.
I can obviously do a Promise.race on my side as below, but such logic should definitely be part of the SDK:
I can provide a contribution, if we have an agreement here.
SDK version
Language version, developer tools
OS/platform