http, http2: remove default server timeout

ofrobots · ofrobots · commit c30ef3cbd2e4 · 2019-05-09T08:54:04.000-07:00
Timing out and closing the socket after two minutes have elapsed is surprising and problematic for users. This behavior was specific to Node.js, and doesn't seem to be common in other language runtimes. Fixes: #27556 PR-URL: #27558 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Rich Trott <rtrott@gmail.com>
diff --git a/doc/api/http.md b/doc/api/http.md
@@ -1013,9 +1013,13 @@ Limits maximum incoming headers count. If set to 0, no limit will be applied.
 ### server.setTimeout([msecs][, callback])
 <!-- YAML
 added: v0.9.12
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/27558
+    description: The default timeout changed from 120s to 0 (no timeout).
 -->
 
-* `msecs` {number} **Default:** `120000` (2 minutes)
+* `msecs` {number} **Default:** 0 (no timeout)
 * `callback` {Function}
 * Returns: {http.Server}
 
@@ -1026,16 +1030,20 @@ occurs.
 If there is a `'timeout'` event listener on the Server object, then it
 will be called with the timed-out socket as an argument.
 
-By default, the Server's timeout value is 2 minutes, and sockets are
-destroyed automatically if they time out. However, if a callback is assigned
-to the Server's `'timeout'` event, timeouts must be handled explicitly.
+By default, the Server does not timeout sockets. However, if a callback
+is assigned to the Server's `'timeout'` event, timeouts must be handled
+explicitly.
 
 ### server.timeout
 <!-- YAML
 added: v0.9.12
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/27558
+    description: The default timeout changed from 120s to 0 (no timeout).
 -->
 
-* {number} Timeout in milliseconds. **Default:** `120000` (2 minutes).
+* {number} Timeout in milliseconds. **Default:** 0 (no timeout)
 
 The number of milliseconds of inactivity before a socket is presumed
 to have timed out.
diff --git a/doc/api/http2.md b/doc/api/http2.md
@@ -1722,11 +1722,15 @@ server.on('stream', (stream, headers, flags) => {
 #### Event: 'timeout'
 <!-- YAML
 added: v8.4.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/27558
+    description: The default timeout changed from 120s to 0 (no timeout).
 -->
 
 The `'timeout'` event is emitted when there is no activity on the Server for
 a given number of milliseconds set using `http2server.setTimeout()`.
-**Default:** 2 minutes.
+**Default:** 0 (no timeout)
 
 #### server.close([callback])
 <!-- YAML
@@ -1743,9 +1747,13 @@ consider also using [`http2session.close()`] on active sessions.
 #### server.setTimeout([msecs][, callback])
 <!-- YAML
 added: v8.4.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/27558
+    description: The default timeout changed from 120s to 0 (no timeout).
 -->
 
-* `msecs` {number} **Default:** `120000` (2 minutes)
+* `msecs` {number} **Default:** 0 (no timeout)
 * `callback` {Function}
 * Returns: {Http2Server}
 
diff --git a/lib/_http_server.js b/lib/_http_server.js
@@ -315,7 +315,7 @@ function Server(options, requestListener) {
 
   this.on('connection', connectionListener);
 
-  this.timeout = 2 * 60 * 1000;
+  this.timeout = 0;
   this.keepAliveTimeout = 5000;
   this.maxHeadersCount = null;
   this.headersTimeout = 40 * 1000; // 40 seconds
diff --git a/lib/https.js b/lib/https.js
@@ -71,7 +71,7 @@ function Server(opts, requestListener) {
       conn.destroy(err);
   });
 
-  this.timeout = 2 * 60 * 1000;
+  this.timeout = 0;
   this.keepAliveTimeout = 5000;
   this.maxHeadersCount = null;
   this.headersTimeout = 40 * 1000; // 40 seconds
diff --git a/lib/internal/http2/core.js b/lib/internal/http2/core.js
@@ -171,8 +171,6 @@ const kState = Symbol('state');
 const kType = Symbol('type');
 const kWriteGeneric = Symbol('write-generic');
 
-const kDefaultSocketTimeout = 2 * 60 * 1000;
-
 const {
   paddingBuffer,
   PADDING_BUF_FRAME_LENGTH,
@@ -2680,7 +2678,7 @@ class Http2SecureServer extends TLSServer {
     options = initializeTLSOptions(options);
     super(options, connectionListener);
     this[kOptions] = options;
-    this.timeout = kDefaultSocketTimeout;
+    this.timeout = 0;
     this.on('newListener', setupCompat);
     if (typeof requestListener === 'function')
       this.on('request', requestListener);
@@ -2702,7 +2700,7 @@ class Http2Server extends NETServer {
   constructor(options, requestListener) {
     super(connectionListener);
     this[kOptions] = initializeOptions(options);
-    this.timeout = kDefaultSocketTimeout;
+    this.timeout = 0;
     this.on('newListener', setupCompat);
     if (typeof requestListener === 'function')
       this.on('request', requestListener);
diff --git a/test/async-hooks/test-graph.http.js b/test/async-hooks/test-graph.http.js
@@ -43,9 +43,6 @@ process.on('exit', function() {
       { type: 'HTTPINCOMINGMESSAGE',
         id: 'httpincomingmessage:1',
         triggerAsyncId: 'tcp:2' },
-      { type: 'Timeout',
-        id: 'timeout:2',
-        triggerAsyncId: 'tcp:2' },
       { type: 'SHUTDOWNWRAP',
         id: 'shutdown:1',
         triggerAsyncId: 'tcp:2' } ]
diff --git a/test/parallel/test-child-process-http-socket-leak.js b/test/parallel/test-child-process-http-socket-leak.js
@@ -46,7 +46,7 @@ server.listen(0, common.mustCall(() => {
     }, common.mustCall((res) => {
       res.on('data', () => {});
       res.on('end', common.mustCall(() => {
-        assert.strictEqual(socket[kTimeout]._idleTimeout, -1);
+        assert.strictEqual(socket[kTimeout], null);
         assert.strictEqual(socket.parser, null);
         assert.strictEqual(socket._httpMessage, null);
       }));
