fix: replace keepAlive option with pipelining 0 (#459)

nodejs · Oct 26, 2020 · 64d582c · 64d582c
1 parent 1dc8328
commit 64d582c
Show file tree

Hide file tree

Showing 4 changed files with 8 additions and 63 deletions.
diff --git a/README.md b/README.md
@@ -58,9 +58,6 @@ Options:
  This value may be overriden by *keep-alive* hints from the server.
  Default: `4e3` milliseconds (4s).
 
-- `keepAlive: Boolean`, enable or disable keep alive connections.
- Default: `true`.
-
 - `keepAliveMaxTimeout: Number`, the maximum allowed `idleTimeout` when overriden by
  *keep-alive* hints from the server.
  Default: `600e3` milliseconds (10min).
@@ -75,6 +72,7 @@ Options:
  Carefully consider your workload and environment before enabling concurrent requests
  as pipelining may reduce performance if used incorrectly. Pipelining is sensitive 
  to network stack settings as well as head of line blocking caused by e.g. long running requests.
+ Set to `0` to disable keep-alive connections.
  Default: `1`.
 
 - `tls: Object|Null`, an options object which in the case of `https` will be passed to

diff --git a/lib/core/client.js b/lib/core/client.js
@@ -53,7 +53,6 @@ const {
  kHeadersTimeout,
  kKeepAliveMaxTimeout,
  kKeepAliveTimeoutThreshold,
- kKeepAlive,
  kTLSSession
 } = require('./symbols')
 
@@ -124,10 +123,6 @@ class Client extends EventEmitter {
  throw new InvalidArgumentError('invalid keepAliveMaxTimeout')
  }
 
- if (keepAlive != null && typeof keepAlive !== 'boolean') {
- throw new InvalidArgumentError('invalid keepAlive')
- }
-
  if (keepAliveTimeoutThreshold != null && !Number.isFinite(keepAliveTimeoutThreshold)) {
  throw new InvalidArgumentError('invalid keepAliveTimeoutThreshold')
  }
@@ -138,7 +133,7 @@ class Client extends EventEmitter {
 
  this[kSocket] = null
  this[kReset] = false
- this[kPipelining] = pipelining || 1
+ this[kPipelining] = pipelining != null ? pipelining : 1
  this[kMaxHeadersSize] = maxHeaderSize || 16384
  this[kHeadersTimeout] = headersTimeout == null ? 30e3 : headersTimeout
  this[kUrl] = url
@@ -148,7 +143,6 @@ class Client extends EventEmitter {
  this[kKeepAliveMaxTimeout] = keepAliveMaxTimeout == null ? 600e3 : keepAliveMaxTimeout
  this[kKeepAliveTimeoutThreshold] = keepAliveTimeoutThreshold == null ? 1e3 : keepAliveTimeoutThreshold
  this[kKeepAliveTimeout] = this[kIdleTimeout]
- this[kKeepAlive] = keepAlive == null ? true : keepAlive
  this[kClosed] = false
  this[kDestroyed] = false
  this[kTLSServerName] = (tls && tls.servername) || null
@@ -530,7 +524,7 @@ class Parser extends HTTPParser {
  this.headers = null
  this.trailers = trailers ? trailers.toLowerCase().split(/,\s*/) : null
 
- if (shouldKeepAlive && client[kKeepAlive]) {
+ if (shouldKeepAlive && client[kPipelining]) {
  const keepAliveTimeout = keepAlive ? util.parseKeepAliveTimeout(keepAlive) : null
 
  if (keepAliveTimeout != null) {
@@ -904,7 +898,7 @@ function _resume (client, sync) {
  client[kNeedDrain] = 2
  }
 
- if (client.running >= client[kPipelining]) {
+ if (client.running >= (client[kPipelining] || 1)) {
  return
  }
 
@@ -956,11 +950,6 @@ function _resume (client, sync) {
  return
  }
 
- if (client.running && !client[kKeepAlive]) {
- // Don't schedule more if we know connection will reset.
- return
- }
-
  if (client.running && !request.idempotent) {
  // Non-idempotent request cannot be retried.
  // Ensure that no other requests are inflight and
@@ -1091,7 +1080,7 @@ function write (client, request) {
 
  if (upgrade) {
  header = `${method} ${path} HTTP/1.1\r\nconnection: upgrade\r\nupgrade: ${upgrade}\r\n`
- } else if (client[kKeepAlive]) {
+ } else if (client[kPipelining]) {
  header = `${method} ${path} HTTP/1.1\r\nconnection: keep-alive\r\n`
  } else {
  header = `${method} ${path} HTTP/1.1\r\nconnection: close\r\n`

diff --git a/test/client-keep-alive.js b/test/client-keep-alive.js
@@ -30,7 +30,7 @@ test('keep-alive header', (t) => {
  body.on('end', () => {
  const timeout = setTimeout(() => {
  t.fail()
- }, 3e3)
+ }, 2e3)
  client.on('disconnect', () => {
  t.pass()
  clearTimeout(timeout)
@@ -257,7 +257,7 @@ test('Disable keep alive', (t) => {
  t.teardown(server.close.bind(server))
 
  server.listen(0, () => {
- const client = new Client(`http:https://localhost:${server.address().port}`, { keepAlive: false })
+ const client = new Client(`http:https://localhost:${server.address().port}`, { pipelining: 0 })
  t.teardown(client.destroy.bind(client))
 
  client.request({
@@ -279,45 +279,3 @@ test('Disable keep alive', (t) => {
  })
  })
 })
-
-test('Disable keep alive concurrency 1', (t) => {
- t.plan(8)
-
- const ports = []
- const server = http.createServer((req, res) => {
- t.false(ports.includes(req.socket.remotePort))
- ports.push(req.socket.remotePort)
- t.match(req.headers, { connection: 'close' })
- res.writeHead(200, { connection: 'close' })
- res.end()
- })
- t.teardown(server.close.bind(server))
-
- server.listen(0, () => {
- const client = new Client(`http:https://localhost:${server.address().port}`, {
- keepAlive: false,
- pipelining: 2
- })
- t.teardown(client.destroy.bind(client))
-
- client.request({
- path: '/',
- method: 'GET'
- }, (err, { body }) => {
- t.error(err)
- body.on('end', () => {
- t.pass()
- }).resume()
- })
-
- client.request({
- path: '/',
- method: 'GET'
- }, (err, { body }) => {
- t.error(err)
- body.on('end', () => {
- t.pass()
- }).resume()
- })
- })
-})
diff --git a/test/tls-session-reuse.js b/test/tls-session-reuse.js
@@ -32,7 +32,7 @@ test('TLS should reuse sessions', { skip: nodeMajor < 11 }, t => {
 
  server.listen(0, function () {
  const client = new Client(`https://localhost:${server.address().port}`, {
- keepAlive: false,
+ pipelining: 0,
  tls: {
  ca,
  rejectUnauthorized: false,