Drop default 30-second timeout (#2573)

Author: JoshMock

docs/basic-config.asciidoc

@@ -13,7 +13,6 @@ const client = new Client({
   cloud: { id: '<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. +

docs/changelog.asciidoc

@@ -12,6 +12,11 @@
 
 In 8.0, the top-level `body` parameter that was available on all API functions <<remove-body-key,was deprecated>>. 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 <<timeout-best-practices>> for more information.
+
 [discrete]
 === 8.17.0
 

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)
-----
+----

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.

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.

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"
   },

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,

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()

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

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({