From 0ad42ff1a2aa46d9082b4966040f6c93325317c0 Mon Sep 17 00:00:00 2001 From: Josh Mock Date: Thu, 30 Jan 2025 10:37:53 -0600 Subject: [PATCH] Drop default 30-second timeout (#2573) --- docs/basic-config.asciidoc | 3 +-- docs/changelog.asciidoc | 5 +++++ docs/child.asciidoc | 23 +++++++++++------------ docs/connecting.asciidoc | 4 ++-- docs/timeout-best-practices.asciidoc | 8 +++----- package.json | 4 ++-- src/client.ts | 6 +++--- test/unit/client.test.ts | 28 ++++++++++++++++++---------- test/unit/helpers/bulk.test.ts | 4 ++-- test/unit/helpers/msearch.test.ts | 2 +- 10 files changed, 48 insertions(+), 39 deletions(-) diff --git a/docs/basic-config.asciidoc b/docs/basic-config.asciidoc index 799866f93..0d4a0a73d 100644 --- a/docs/basic-config.asciidoc +++ b/docs/basic-config.asciidoc @@ -13,7 +13,6 @@ const client = new Client({ cloud: { id: '' }, auth: { apiKey: 'base64EncodedKey' }, maxRetries: 5, - requestTimeout: 60000, sniffOnStart: true }) ---- @@ -82,7 +81,7 @@ _Default:_ `3` |`requestTimeout` |`number` - Max request timeout in milliseconds for each request. + -_Default:_ `30000` +_Default:_ No value |`pingTimeout` |`number` - Max ping request timeout in milliseconds for each request. + diff --git a/docs/changelog.asciidoc b/docs/changelog.asciidoc index dec73d23e..78b23e45a 100644 --- a/docs/changelog.asciidoc +++ b/docs/changelog.asciidoc @@ -12,6 +12,11 @@ In 8.0, the top-level `body` parameter that was available on all API functions <>. In 9.0 this property is completely removed. +[discrete] +===== Remove the default 30-second timeout on all requests sent to Elasticsearch + +Setting HTTP timeouts on Elasticsearch requests goes against Elastic's recommendations. See <> for more information. + [discrete] === 8.17.0 diff --git a/docs/child.asciidoc b/docs/child.asciidoc index 0bd7ace21..25575bbe2 100644 --- a/docs/child.asciidoc +++ b/docs/child.asciidoc @@ -1,22 +1,22 @@ [[child]] === Creating a child client -There are some use cases where you may need multiple instances of the client. -You can easily do that by calling `new Client()` as many times as you need, but -you will lose all the benefits of using one single client, such as the long -living connections and the connection pool handling. To avoid this problem, the -client offers a `child` API, which returns a new client instance that shares the +There are some use cases where you may need multiple instances of the client. +You can easily do that by calling `new Client()` as many times as you need, but +you will lose all the benefits of using one single client, such as the long +living connections and the connection pool handling. To avoid this problem, the +client offers a `child` API, which returns a new client instance that shares the connection pool with the parent client. -NOTE: The event emitter is shared between the parent and the child(ren). If you -extend the parent client, the child client will have the same extensions, while +NOTE: The event emitter is shared between the parent and the child(ren). If you +extend the parent client, the child client will have the same extensions, while if the child client adds an extension, the parent client will not be extended. -You can pass to the `child` every client option you would pass to a normal -client, but the connection pool specific options (`ssl`, `agent`, `pingTimeout`, +You can pass to the `child` every client option you would pass to a normal +client, but the connection pool specific options (`ssl`, `agent`, `pingTimeout`, `Connection`, and `resurrectStrategy`). -CAUTION: If you call `close` in any of the parent/child clients, every client +CAUTION: If you call `close` in any of the parent/child clients, every client will be closed. [source,js] @@ -28,9 +28,8 @@ const client = new Client({ }) const child = client.child({ headers: { 'x-foo': 'bar' }, - requestTimeout: 1000 }) client.info().then(console.log, console.log) child.info().then(console.log, console.log) ----- \ No newline at end of file +---- diff --git a/docs/connecting.asciidoc b/docs/connecting.asciidoc index 4646ee5f1..f87961edb 100644 --- a/docs/connecting.asciidoc +++ b/docs/connecting.asciidoc @@ -414,8 +414,8 @@ The supported request specific options are: _Default:_ `null` |`requestTimeout` -|`number | string` - Max request timeout for the request in milliseconds, it overrides the client default. + -_Default:_ `30000` +|`number | string | null` - Max request timeout for the request in milliseconds. This overrides the client default, which is to not time out at all. See https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-network.html#_http_client_configuration[Elasticsearch best practices for HTML clients] for more info. + +_Default:_ No timeout |`retryOnTimeout` |`boolean` - Retry requests that have timed out. diff --git a/docs/timeout-best-practices.asciidoc b/docs/timeout-best-practices.asciidoc index 0d2fb4772..5116034af 100644 --- a/docs/timeout-best-practices.asciidoc +++ b/docs/timeout-best-practices.asciidoc @@ -1,10 +1,8 @@ [[timeout-best-practices]] === Timeout best practices -This client is configured by default to operate like many HTTP client libraries do, by using a relatively short (30 second) timeout on all requests sent to {es}, raising a `TimeoutError` when that time period has elapsed without receiving a response. However, {es} will always eventually respond to any request, even if it takes several minutes. The {ref}/modules-network.html#_http_client_configuration[official {es} recommendation] is to disable response timeouts entirely by default. +Starting in 9.0.0, this client is configured to not time out any HTTP request by default. {es} will always eventually respond to any request, even if it takes several minutes. Reissuing a request that it has not responded to yet can cause performance side effects. See the {ref}/modules-network.html#_http_client_configuration[official {es} recommendations for HTTP clients] for more information. -Since changing this default would be a breaking change, we won't do that until the next major release. In the meantime, here is our recommendation for properly configuring your client: +Prior to 9.0, this client was configured by default to operate like many HTTP client libraries do, by using a relatively short (30 second) timeout on all requests sent to {es}, raising a `TimeoutError` when that time period elapsed without receiving a response. -* Ensure keep-alive is enabled; this is the default, so no settings need to be changed, unless you have set `agent` to `false` or provided an alternate `agent` that disables keep-alive -* If using the default `UndiciConnection`, disable request timeouts by setting `timeout` to `0` -* If using the legacy `HttpConnection`, set `timeout` to a very large number (e.g. `86400000`, or one day) +If your circumstances require you to set timeouts on Elasticsearch requests, setting the `requestTimeout` value to a millisecond value will cause this client to operate as it did prior to 9.0. diff --git a/package.json b/package.json index 759bd0636..17366f82c 100644 --- a/package.json +++ b/package.json @@ -57,7 +57,7 @@ }, "devDependencies": { "@elastic/request-converter": "8.17.0", - "@sinonjs/fake-timers": "github:sinonjs/fake-timers#48f089f", + "@sinonjs/fake-timers": "14.0.0", "@types/debug": "4.1.12", "@types/ms": "0.7.34", "@types/node": "22.10.7", @@ -89,7 +89,7 @@ "zx": "7.2.3" }, "dependencies": { - "@elastic/transport": "^8.9.1", + "@elastic/transport": "9.0.0-alpha.1", "apache-arrow": "^18.0.0", "tslib": "^2.4.0" }, diff --git a/src/client.ts b/src/client.ts index 2549a5b37..fcbc5d35b 100644 --- a/src/client.ts +++ b/src/client.ts @@ -108,14 +108,15 @@ export interface ClientOptions { * @defaultValue 3 */ maxRetries?: number /** @property requestTimeout Max request timeout in milliseconds for each request - * @defaultValue 30000 */ + * @defaultValue No timeout + * @remarks Read [the Elasticsearch docs](https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-network.html#_http_client_configuration) about HTTP client configuration for details. */ requestTimeout?: number /** @property pingTimeout Max number of milliseconds a `ClusterConnectionPool` will wait when pinging nodes before marking them dead * @defaultValue 3000 */ pingTimeout?: number /** @property sniffInterval Perform a sniff operation every `n` milliseconds * @remarks Sniffing might not be the best solution for you. Read https://www.elastic.co/blog/elasticsearch-sniffing-best-practices-what-when-why-how to learn more. - * @defaultValue */ + * @defaultValue false */ sniffInterval?: number | boolean /** @property sniffOnStart Perform a sniff once the client is started * @remarks Sniffing might not be the best solution for you. Read https://www.elastic.co/blog/elasticsearch-sniffing-best-practices-what-when-why-how to learn more. @@ -244,7 +245,6 @@ export default class Client extends API { Serializer, ConnectionPool: (opts.cloud != null) ? CloudConnectionPool : WeightedConnectionPool, maxRetries: 3, - requestTimeout: 30000, pingTimeout: 3000, sniffInterval: false, sniffOnStart: false, diff --git a/test/unit/client.test.ts b/test/unit/client.test.ts index cc9868cfe..fc2af683c 100644 --- a/test/unit/client.test.ts +++ b/test/unit/client.test.ts @@ -17,9 +17,10 @@ * under the License. */ -import * as http from 'http' +import * as http from 'node:http' +import { URL } from 'node:url' +import { setTimeout } from 'node:timers/promises' import { test } from 'tap' -import { URL } from 'url' import FakeTimers from '@sinonjs/fake-timers' import { buildServer, connection } from '../utils' import { Client, errors } from '../..' @@ -451,30 +452,37 @@ test('Ensure new client instance stores requestTimeout for each connection', t = t.end() }) -test('Ensure new client does not time out at default (30s) when client sets requestTimeout', async t => { - const clock = FakeTimers.install({ toFake: ['setTimeout', 'clearTimeout'] }) +test('No request timeout is set by default', t => { + const client = new Client({ + node: { url: new URL('http://localhost:9200') }, + }) + t.equal(client.connectionPool.connections[0].timeout, null) + t.end() +}) + +test('Ensure new client does not time out if requestTimeout is not set', async t => { + const clock = FakeTimers.install({ toFake: ['setTimeout'] }) t.teardown(() => clock.uninstall()) function handler (_req: http.IncomingMessage, res: http.ServerResponse) { - setTimeout(() => { - t.pass('timeout ended') + setTimeout(1000 * 60 * 60).then(() => { + t.ok('timeout ended') res.setHeader('content-type', 'application/json') res.end(JSON.stringify({ success: true })) - }, 31000) // default is 30000 - clock.runToLast() + }) + clock.tick(1000 * 60 * 60) } const [{ port }, server] = await buildServer(handler) const client = new Client({ node: `http://localhost:${port}`, - requestTimeout: 60000 }) try { await client.transport.request({ method: 'GET', path: '/' }) } catch (error) { - t.fail('timeout error hit') + t.fail('Error should not be thrown', error) } finally { server.stop() t.end() diff --git a/test/unit/helpers/bulk.test.ts b/test/unit/helpers/bulk.test.ts index 3871c348f..d45d2d003 100644 --- a/test/unit/helpers/bulk.test.ts +++ b/test/unit/helpers/bulk.test.ts @@ -1611,7 +1611,7 @@ test('errors', t => { test('Flush interval', t => { t.test('Slow producer', async t => { - const clock = FakeTimers.install({ toFake: ['setTimeout', 'clearTimeout'] }) + const clock = FakeTimers.install({ toFake: ['setTimeout', 'clearTimeout'], shouldClearNativeTimers: true }) t.teardown(() => clock.uninstall()) let count = 0 @@ -1663,7 +1663,7 @@ test('Flush interval', t => { }) t.test('Abort operation', async t => { - const clock = FakeTimers.install({ toFake: ['setTimeout', 'clearTimeout'] }) + const clock = FakeTimers.install({ toFake: ['setTimeout', 'clearTimeout'], shouldClearNativeTimers: true }) t.teardown(() => clock.uninstall()) let count = 0 diff --git a/test/unit/helpers/msearch.test.ts b/test/unit/helpers/msearch.test.ts index e80c5977c..ba2457587 100644 --- a/test/unit/helpers/msearch.test.ts +++ b/test/unit/helpers/msearch.test.ts @@ -583,7 +583,7 @@ test('Multiple searches (concurrency = 1)', t => { test('Flush interval', t => { t.plan(2) - const clock = FakeTimers.install({ toFake: ['setTimeout', 'clearTimeout'] }) + const clock = FakeTimers.install({ toFake: ['setTimeout', 'clearTimeout'], shouldClearNativeTimers: true }) t.teardown(() => clock.uninstall()) const MockConnection = connection.buildMockConnection({