Using `timeout` does not take effect?

[bobisjan]

Describe the bug
Using (default) timeout option does not take effect.

To reproduce
During the outage https://status.launchdarkly.com/incidents/2272yx0d6fh8 our monitoring reported average response time over the default 5 seconds timeout.

Expected behavior
Using (default) timeout option does take effect.

Logs

SDK version
5.13.4

Additional context
Based on nodejs/node#12005 (comment) the httpRequest util should subscribe to timeout event and abort request?

req.on('timeout', () => {
 req.abort();
});

[eli-darkly]

I don't know how your monitoring tools work, but measuring "total response time" for endpoints on stream.launchdarkly.com is meaningless because those are streaming connections that are meant to stay open. If a streaming connection was made at time T0, and then is is terminated at time T1 due to an outage, the total time it was open is T1 minus T0, but that has no relationship to timeouts since if there had not been an outage it would have been open for the entire lifetime of the application.

[bobisjan]

@eli-darkly thanks for you response.

those are streaming connections that are meant to stay open.

The SDK is initialised per every request, because lifetime of our application instances are tied to the individual requests, does this mean that the SDK should be initialised once before any instance of the application is created?

Since the lifetime of the SDK is tied to every request then the graph shows valid data, because opening connections was taking longer as usual?

[eli-darkly]

@bobisjan There are a couple of questions there, and I'm not sure I'm understand you correctly.

The SDK is initialised per every request, because lifetime of our application instances are tied to the individual requests, does this mean that the SDK should be initialised once before any instance of the application is created?

Normally we strongly advise that people do not re-instantiate the SDK for every request. As the server-side Node SDK documentation says, "It's important to make this a singleton. The client instance maintains internal state that allows us to serve feature flags without making any remote requests. Be sure that you're not instantiating a new client with every request." You can do that, but it's very inefficient.

Since the lifetime of the SDK is tied to every request then the graph shows valid data, because opening connections was taking longer as usual?

As I said, that depends on what your monitoring tool is actually measuring. What it says on the graph is "total response time". That is not the same thing as connection time.

Every HTTP connection, streaming or not, has several phases: 1. the client establishes a connection to the server; 2. the client sends a request over the connection; 3. the server sends data back; 4. the connection is closed by either the client or the server. The connection timeout applies to step 1 only. Depending on the kind of outage, it can be possible for the server to accept the connection quickly, but then not send data back right away or at all, in which case the connection timeout does not apply, but rather the read timeout (a.k.a. socket timeout). For streaming connections, that is necessarily a much higher number because it's normal for there to be long periods of time without any new data being sent on the stream; in our SDK, that setting is not configurable by the application because it has to match the server's behavior (that is, the server guarantees that it will send "heartbeat" data every few minutes to keep the connection alive, so the read timeout must not be shorter than that).

Anyway, it sounds like your main concern here is not just what the graph looks like, but practically speaking, how long does your application take to initialize since you are currently starting a new client each time. If it is really necessary for you to do that because of your architecture, then I think what you want to do is impose an additional timeout in the client initialization step. You would need to do that even if the timeout on individual requests was short, because the SDK always retries stream connections— it will not return early from the init() call unless it either succeeds or finds out that your SDK key is invalid.

Node makes it fairly easy to provide your own timeout logic for any asynchronous call, especially for code that uses Promises, as the SDK does. That takes only a few lines of code and there are commonly used helper libraries to make it even simpler, such as promise-timeout.

Supposing that your current initialization logic looked something like this—

const client = launchDarkly.init('my-sdk-key', options);
client.waitForInitialization().then(() => doSomethingAfterInitialization());

—adding a 5-second cutoff using the promise-timeout helper might look like this:

const client = launchDarkly.init('my-sdk-key', options);
timeout(client.waitForInitialization(), 5000)
 .then(() => doSomethingAfterInitialization())
 .catch((err) => doSomethingForFailedInitialization(err));

The SDK tries to go on connecting in the background no matter what, unless you call client.close(), but the promise created by timeout causes your doSomethingForFailedInitialization() to be called instead of doSomethingAfterInitialization() if the timeout expires first. The basic method being used by the helper is Promise.race().

If you try to evaluate flags before the client has successfully initialized, the SDK's defined behavior is for every evaluation to return whatever default value you specified as the last parameter to variation(); that's what it does whenever it does not yet have any flag data (the same as if you specified a flag key that doesn't exist).

[eli-darkly]

Closing this issue due to inactivity. Note that in the 5.13.5 release we added some new timeout behavior, so that the SDK will restart the stream connection if it has received no heartbeats from the server within 5 minutes (indicating a dead connection). But that is unrelated to the discussion in this issue.