fix: Wire timeout option through to flag polling and event delivery requests#1760
fix: Wire timeout option through to flag polling and event delivery requests#1760joker23 wants to merge 1 commit into
Conversation
…equests The timeout configuration was stored in configuration but never threaded through the datamanger so all connections ran without any timeout regardless of configuration. This commit will fix this for the node server sdk (we still need to assess this for client side sdks and see if it is a problem for other server side sdks). In addition, this commit will also set the timeout default to 10s which is consistent with most other server side sdks (eg python, ruby, c++, etc).
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/browser size report |
| streamUri: TypeValidators.String, | ||
| eventsUri: TypeValidators.String, | ||
| timeout: TypeValidators.Number, | ||
| timeout: TypeValidators.numberWithMin(1), |
There was a problem hiding this comment.
There is no specs on this AFAIK, open to discuss... currently the thought is that timeout should exist and must be a valid timeout. Alternatives that I think could work are:
- accept
0and make to "no timeout" - define a stricter min (eg
5seconds) to prevent friendly fire
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 037606a. Configure here.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
The timeout configuration was stored in configuration but never threaded
through the datamanger so all connections ran without any timeout
regardless of configuration. This commit will fix this for the node
server sdk (we still need to assess this for client side sdks and see if
it is a problem for other server side sdks).
In addition, this commit will also set the timeout default to 10s which
is consistent with most other server side sdks (eg python, ruby, c++,
etc).
Note
Medium Risk
Changes the default network timeout and alters failure behavior for slow or stuck upstreams during flag polling; apps that relied on 5s or sub-1 values will see different timing without code changes.
Overview
Fixes
timeoutso it actually limits how long flag polling HTTP calls can hang, instead of being stored in config but never passed to the platform layer.Requestornow converts configured seconds to milliseconds and setsoptions.timeouton pollingfetchcalls.NodeRequestsadds atimeoutlistener that **destroy**s the request withRequest timed outwhen the socket timeout fires (with a regression test against a non-responding server).Configuration changes: default
timeoutrises from 5s to 10s (aligned with other server SDKs), validation requires ≥ 1 (invalid 0 clamps to 1 with a warning), and diagnostics/tests reflect the new defaults.Reviewed by Cursor Bugbot for commit 037606a. Bugbot is set up for automated code reviews on this repo. Configure here.