Skip to content

fix: Wire timeout option through to flag polling and event delivery requests #1760

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.

Sign up for GitHub

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

Open
joker23 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Wire timeout option through to flag polling and event delivery requests #1760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions packages/sdk/server-node/__tests__/platform/NodeRequests.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,38 @@ describe('given a default instance of NodeRequests', () => {
});
});

describe('given a request to a server that never responds', () => {
let hangingServer: http.Server;
let hangingPort: number;

beforeAll(
async () =>
new Promise<void>((resolveListening) => {
hangingServer = http.createServer((_req, _res) => {
// Intentionally never respond — simulates a hung upstream.
});
hangingServer.listen(0, () => {
hangingPort = (hangingServer.address() as { port: number }).port;
resolveListening();
});
}),
);

afterAll(
async () =>
new Promise<void>((resolveClose) => {
hangingServer.close(() => resolveClose());
}),
);

it('rejects with "Request timed out" when the timeout elapses', async () => {
const requests = new NodeRequests();
await expect(
requests.fetch(`http://localhost:${hangingPort}`, { timeout: 50 }),
).rejects.toThrow('Request timed out');
});
});

describe('given an instance of NodeRequests with enableEventCompression turned on', () => {
const requests = new NodeRequests(undefined, undefined, undefined, true);
it('can make a basic post with compressBodyIfPossible enabled', async () => {
Expand Down
4 changes: 4 additions & 0 deletions packages/sdk/server-node/src/platform/NodeRequests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,10 @@ export default class NodeRequests implements platform.Requests {
reject(err);
});

req.on('timeout', () => {
req.destroy(new Error('Request timed out'));
});

req.end();
});
}
Expand Down
21 changes: 17 additions & 4 deletions packages/shared/sdk-server/__tests__/data_sources/Requestor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,15 @@ describe('given a requestor', () => {
let requestor: Requestor;

let requestsMade: Array<{ url: string; options: Options }>;
let requests: Requests;

let testHeaders: Record<string, string>;
let testStatus = 200;
let testResponse: string | undefined;
let throwThis: string | undefined;

const baseHeaders = { authorization: 'sdkKey' };

function resetRequestState() {
requestsMade = [];
testHeaders = {};
Expand All @@ -33,7 +36,7 @@ describe('given a requestor', () => {
beforeEach(() => {
resetRequestState();

const requests: Requests = {
requests = {
async fetch(url: string, options?: Options): Promise<Response> {
return new Promise<Response>((a, r) => {
if (throwThis) {
Expand Down Expand Up @@ -80,9 +83,7 @@ describe('given a requestor', () => {
},
};

requestor = new Requestor(new Configuration({}), requests, {
authorization: 'sdkKey',
});
requestor = new Requestor(new Configuration({}), requests, baseHeaders);
});

it('gets data', (done) => {
Expand Down Expand Up @@ -213,4 +214,16 @@ describe('given a requestor', () => {
expect(res.err).toBeInstanceOf(LDFlagDeliveryFallbackError);
expect(res.fallbackToFDv1).toBe(true);
});

it('passes the configured timeout (converted to milliseconds) in the request options', async () => {
testResponse = 'a response';
const localRequestor = new Requestor(new Configuration({ timeout: 10 }), requests, baseHeaders);
const res = await promisify<{ err: any; body: any }>((cb) => {
localRequestor.requestAllData((err, body) => cb({ err, body }));
});
expect(res.err).toBeUndefined();
expect(requestsMade.length).toBe(1);
// timeout: 10 (seconds) must be converted to milliseconds before being passed to fetch.
expect(requestsMade[0].options.timeout).toBe(10000);
});
});
4 changes: 2 additions & 2 deletions packages/shared/sdk-server/__tests__/diagnostics/createDiagnosticsInitConfig.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ describe.each([
{},
{
allAttributesPrivate: false,
connectTimeoutMillis: 5000,
connectTimeoutMillis: 10000,
customBaseURI: false,
customEventsURI: false,
customStreamURI: false,
Expand All @@ -29,7 +29,7 @@ describe.each([
offline: false,
pollingIntervalMillis: 30000,
reconnectTimeMillis: 1000,
socketTimeoutMillis: 5000,
socketTimeoutMillis: 10000,
streamingDisabled: false,
contextKeysCapacity: 1000,
contextKeysFlushIntervalMillis: 300000,
Expand Down
6 changes: 3 additions & 3 deletions packages/shared/sdk-server/__tests__/options/Configuration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ describe.each([undefined, null, 'potat0', 17, [], {}])('constructed without opti
expect(config.stream).toBe(true);
expect(config.streamInitialReconnectDelay).toEqual(1);
expect(config.tags.value).toBeUndefined();
expect(config.timeout).toEqual(5);
expect(config.timeout).toEqual(10);
expect(config.tlsParams).toBeUndefined();
expect(config.useLdd).toBe(false);
expect(config.wrapperName).toBeUndefined();
Expand Down Expand Up @@ -138,9 +138,9 @@ describe('when setting different options', () => {
});

it.each([
[0, 0, []],
[0, 1, [{ level: LogLevel.Warn, matches: /Config option "timeout" had invalid value/ }]],
[6, 6, []],
['potato', 5, [{ level: LogLevel.Warn, matches: /Config option "timeout" should be of type/ }]],
['potato', 10, [{ level: LogLevel.Warn, matches: /Config option "timeout" should be of type/ }]],
])('allow setting timeout and validates timeout', (value, expected, logs) => {
// @ts-ignore
const config = new Configuration(withLogger({ timeout: value }));
Expand Down
3 changes: 3 additions & 0 deletions packages/shared/sdk-server/src/data_sources/Requestor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import Configuration from '../options/Configuration';
export default class Requestor implements LDFeatureRequestor {
private readonly _headers: Record<string, string>;
private readonly _serviceEndpoints: ServiceEndpoints;
private readonly _timeoutMs: number;
private readonly _eTagCache: Record<
string,
{
Expand All @@ -38,6 +39,7 @@ export default class Requestor implements LDFeatureRequestor {
) {
this._headers = { ...baseHeaders };
this._serviceEndpoints = serviceEndpointsOverride ?? config.serviceEndpoints;
this._timeoutMs = config.timeout * 1000;
}

/**
Expand Down Expand Up @@ -81,6 +83,7 @@ export default class Requestor implements LDFeatureRequestor {
const options: Options = {
method: 'GET',
headers: this._headers,
timeout: this._timeoutMs,
};

const uri = getPollingUri(this._serviceEndpoints, this._path, queryParams);
Expand Down
4 changes: 2 additions & 2 deletions packages/shared/sdk-server/src/options/Configuration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ const validations: Record<string, TypeValidator> = {
baseUri: TypeValidators.String,
streamUri: TypeValidators.String,
eventsUri: TypeValidators.String,
timeout: TypeValidators.Number,
timeout: TypeValidators.numberWithMin(1),

@joker23 joker23 Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. accept 0 and make to "no timeout"
  2. define a stricter min (eg 5 seconds) to prevent friendly fire

@kinyoklion kinyoklion Jun 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can be fine with 1 for now, but I do think I would be fine with something larger as well.

capacity: TypeValidators.Number,
logger: TypeValidators.Object,
featureStore: TypeValidators.ObjectOrFactory,
Expand Down Expand Up @@ -112,7 +112,7 @@ export const defaultValues: ValidatedOptions = {
stream: true,
streamInitialReconnectDelay: DEFAULT_STREAM_RECONNECT_DELAY,
sendEvents: true,
timeout: 5,
timeout: 10,
capacity: 10000,
flushInterval: 5,
pollInterval: DEFAULT_POLL_INTERVAL,
Expand Down
Loading