http: process headers after setting up agent

Added tests to clarify the implicit behaviour of array header setting vs
object header setting

PR-URL: #16568
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

lib/_http_client.js

@@ -164,6 +164,40 @@ function ClientRequest(options, cb) {
     this.once('response', cb);
   }
 
+  if (method === 'GET' ||
+      method === 'HEAD' ||
+      method === 'DELETE' ||
+      method === 'OPTIONS' ||
+      method === 'CONNECT') {
+    this.useChunkedEncodingByDefault = false;
+  } else {
+    this.useChunkedEncodingByDefault = true;
+  }
+
+  this._ended = false;
+  this.res = null;
+  this.aborted = undefined;
+  this.timeoutCb = null;
+  this.upgradeOrConnect = false;
+  this.parser = null;
+  this.maxHeadersCount = null;
+
+  var called = false;
+
+  if (this.agent) {
+    // If there is an agent we should default to Connection:keep-alive,
+    // but only if the Agent will actually reuse the connection!
+    // If it's not a keepAlive agent, and the maxSockets==Infinity, then
+    // there's never a case where this socket will actually be reused
+    if (!this.agent.keepAlive && !Number.isFinite(this.agent.maxSockets)) {
+      this._last = true;
+      this.shouldKeepAlive = false;
+    } else {
+      this._last = false;
+      this.shouldKeepAlive = true;
+    }
+  }
+
   var headersArray = Array.isArray(options.headers);
   if (!headersArray) {
     if (options.headers) {
@@ -173,6 +207,7 @@ function ClientRequest(options, cb) {
         this.setHeader(key, options.headers[key]);
       }
     }
+
     if (host && !this.getHeader('host') && setHost) {
       var hostHeader = host;
 
@@ -191,45 +226,25 @@ function ClientRequest(options, cb) {
       }
       this.setHeader('Host', hostHeader);
     }
-  }
 
-  if (options.auth && !this.getHeader('Authorization')) {
-    this.setHeader('Authorization', 'Basic ' +
-                   Buffer.from(options.auth).toString('base64'));
-  }
+    if (options.auth && !this.getHeader('Authorization')) {
+      this.setHeader('Authorization', 'Basic ' +
+                     Buffer.from(options.auth).toString('base64'));
+    }
 
-  if (method === 'GET' ||
-      method === 'HEAD' ||
-      method === 'DELETE' ||
-      method === 'OPTIONS' ||
-      method === 'CONNECT') {
-    this.useChunkedEncodingByDefault = false;
-  } else {
-    this.useChunkedEncodingByDefault = true;
-  }
+    if (this.getHeader('expect')) {
+      if (this._header) {
+        throw new errors.Error('ERR_HTTP_HEADERS_SENT', 'render');
+      }
 
-  if (headersArray) {
-    this._storeHeader(this.method + ' ' + this.path + ' HTTP/1.1\r\n',
-                      options.headers);
-  } else if (this.getHeader('expect')) {
-    if (this._header) {
-      throw new errors.Error('ERR_HTTP_HEADERS_SENT', 'render');
+      this._storeHeader(this.method + ' ' + this.path + ' HTTP/1.1\r\n',
+                        this[outHeadersKey]);
     }
-
+  } else {
     this._storeHeader(this.method + ' ' + this.path + ' HTTP/1.1\r\n',
-                      this[outHeadersKey]);
+                      options.headers);
   }
 
-  this._ended = false;
-  this.res = null;
-  this.aborted = undefined;
-  this.timeoutCb = null;
-  this.upgradeOrConnect = false;
-  this.parser = null;
-  this.maxHeadersCount = null;
-
-  var called = false;
-
   var oncreate = (err, socket) => {
     if (called)
       return;
@@ -242,18 +257,8 @@ function ClientRequest(options, cb) {
     this._deferToConnect(null, null, () => this._flush());
   };
 
+  // initiate connection
   if (this.agent) {
-    // If there is an agent we should default to Connection:keep-alive,
-    // but only if the Agent will actually reuse the connection!
-    // If it's not a keepAlive agent, and the maxSockets==Infinity, then
-    // there's never a case where this socket will actually be reused
-    if (!this.agent.keepAlive && !Number.isFinite(this.agent.maxSockets)) {
-      this._last = true;
-      this.shouldKeepAlive = false;
-    } else {
-      this._last = false;
-      this.shouldKeepAlive = true;
-    }
     this.agent.addRequest(this, options);
   } else {
     // No agent, default to Connection:close.

test/parallel/test-http-client-headers-array.js

@@ -0,0 +1,60 @@
+'use strict';
+
+require('../common');
+
+const assert = require('assert');
+const http = require('http');
+
+function execute(options) {
+  http.createServer(function(req, res) {
+    const expectHeaders = {
+      'x-foo': 'boom',
+      cookie: 'a=1; b=2; c=3',
+      connection: 'close'
+    };
+
+    // no Host header when you set headers an array
+    if (!Array.isArray(options.headers)) {
+      expectHeaders.host = `localhost:${this.address().port}`;
+    }
+
+    // no Authorization header when you set headers an array
+    if (options.auth && !Array.isArray(options.headers)) {
+      expectHeaders.authorization =
+          `Basic ${Buffer.from(options.auth).toString('base64')}`;
+    }
+
+    this.close();
+
+    assert.deepStrictEqual(req.headers, expectHeaders);
+
+    res.end();
+  }).listen(0, function() {
+    options = Object.assign(options, {
+      port: this.address().port,
+      path: '/'
+    });
+    const req = http.request(options);
+    req.end();
+  });
+}
+
+// should be the same except for implicit Host header on the first two
+execute({ headers: { 'x-foo': 'boom', 'cookie': 'a=1; b=2; c=3' } });
+execute({ headers: { 'x-foo': 'boom', 'cookie': [ 'a=1', 'b=2', 'c=3' ] } });
+execute({ headers: [[ 'x-foo', 'boom' ], [ 'cookie', 'a=1; b=2; c=3' ]] });
+execute({ headers: [
+  [ 'x-foo', 'boom' ], [ 'cookie', [ 'a=1', 'b=2', 'c=3' ]]
+] });
+execute({ headers: [
+  [ 'x-foo', 'boom' ], [ 'cookie', 'a=1' ],
+  [ 'cookie', 'b=2' ], [ 'cookie', 'c=3']
+] });
+
+// Authorization and Host header both missing from the second
+execute({ auth: 'foo:bar', headers:
+  { 'x-foo': 'boom', 'cookie': 'a=1; b=2; c=3' } });
+execute({ auth: 'foo:bar', headers: [
+  [ 'x-foo', 'boom' ], [ 'cookie', 'a=1' ],
+  [ 'cookie', 'b=2' ], [ 'cookie', 'c=3']
+] });