Skip to content

Commit

Permalink
http: process headers after setting up agent
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
rvagg authored and MylesBorins committed Feb 21, 2018
1 parent 6b74064 commit 8b2a272
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 43 deletions.
91 changes: 48 additions & 43 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -173,6 +207,7 @@ function ClientRequest(options, cb) {
this.setHeader(key, options.headers[key]);
}
}

if (host && !this.getHeader('host') && setHost) {
var hostHeader = host;

Expand All @@ -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;
Expand All @@ -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.
Expand Down
60 changes: 60 additions & 0 deletions test/parallel/test-http-client-headers-array.js
Original file line number Diff line number Diff line change
@@ -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']
] });

0 comments on commit 8b2a272

Please sign in to comment.