Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

http: server code refactor #6533

Merged
merged 8 commits into from
Dec 29, 2016
Merged

http: server code refactor #6533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
175ed52
http: reuse existing headers array for raw values
mscdex Dec 18, 2016
14c76f8
http: misc cleanup and minor optimizations
mscdex Dec 18, 2016
832271c
http: refactor server connection handling
mscdex Dec 19, 2016
a54972c
http: improve validation performance
mscdex Dec 19, 2016
b6ea857
lib: avoid recompilation of anonymous functions
mscdex Dec 19, 2016
e8b809a
http: extract validation functions
mscdex Dec 20, 2016
af74e72
http: simplify boolean checks
mscdex Dec 20, 2016
4d7531d
http: optimize headers iteration
mscdex Dec 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 59 additions & 42 deletions lib/_http_common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,6 @@ function freeParser(parser, req, socket) {
parser[kOnExecute] = null;
if (parsers.free(parser) === false)
parser.close();
parser = null;
}
if (req) {
req.parser = null;
Expand Down Expand Up @@ -247,44 +246,44 @@ exports.httpSocketSetup = httpSocketSetup;
* so take care when making changes to the implementation so that the source
* code size does not exceed v8's default max_inlined_source_size setting.
**/
function isValidTokenChar(ch) {
if (ch >= 94 && ch <= 122)
return true;
if (ch >= 65 && ch <= 90)
return true;
if (ch === 45)
return true;
if (ch >= 48 && ch <= 57)
return true;
if (ch === 34 || ch === 40 || ch === 41 || ch === 44)
var validTokens = [
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 0 - 15
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 16 - 31
0, 1, 0, 1, 1, 1, 1, 1, 0, 0, 1, 1, 0, 1, 1, 0, // 32 - 47
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, // 48 - 63
0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // 64 - 79
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 1, 1, // 80 - 95
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // 96 - 111
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 0, 1, 0, // 112 - 127
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 128 ...
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 // ... 255
];
function checkIsHttpToken(val) {
if (typeof val !== 'string' || val.length === 0)
return false;
if (!validTokens[val.charCodeAt(0)])
return false;
if (ch >= 33 && ch <= 46)
if (val.length < 2)
return true;
if (ch === 124 || ch === 126)
if (!validTokens[val.charCodeAt(1)])
return false;
if (val.length < 3)
return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you might want to check if setting a value and having a single return improves things.

Copy link
Contributor Author

@mscdex mscdex Dec 22, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt it, but the early returns are what keeps the function length small enough (it avoids the increasing indentation) and allows for more unwinding.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, great. Than it's perfect.

return false;
}
function checkIsHttpToken(val) {
if (typeof val !== 'string' || val.length === 0)
if (!validTokens[val.charCodeAt(2)])
return false;
if (!isValidTokenChar(val.charCodeAt(0)))
if (val.length < 4)
return true;
if (!validTokens[val.charCodeAt(3)])
return false;
const len = val.length;
if (len > 1) {
if (!isValidTokenChar(val.charCodeAt(1)))
for (var i = 4; i < val.length; ++i) {
if (!validTokens[val.charCodeAt(i)])
return false;
if (len > 2) {
if (!isValidTokenChar(val.charCodeAt(2)))
return false;
if (len > 3) {
if (!isValidTokenChar(val.charCodeAt(3)))
return false;
for (var i = 4; i < len; i++) {
if (!isValidTokenChar(val.charCodeAt(i)))
return false;
}
}
}
}
return true;
}
Expand All @@ -300,26 +299,44 @@ exports._checkIsHttpToken = checkIsHttpToken;
* so take care when making changes to the implementation so that the source
* code size does not exceed v8's default max_inlined_source_size setting.
**/
var validHdrChars = [
0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, // 0 - 15
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // 16 - 31
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // 32 - 47
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // 48 - 63
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // 64 - 79
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // 80 - 95
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // 96 - 111
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, // 112 - 127
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, // 128 ...
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 // ... 255
];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you have any reference mat'l on the choice of int vs bool here? just interested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance-wise? Not really. At the very least it's more compact. I wouldn't be surprised though if V8 internally treated booleans as SMIs with values of 0 or 1.

function checkInvalidHeaderChar(val) {
val += '';
if (val.length < 1)
return false;
var c = val.charCodeAt(0);
if ((c <= 31 && c !== 9) || c > 255 || c === 127)
if (!validHdrChars[val.charCodeAt(0)])
return true;
if (val.length < 2)
return false;
c = val.charCodeAt(1);
if ((c <= 31 && c !== 9) || c > 255 || c === 127)
if (!validHdrChars[val.charCodeAt(1)])
return true;
if (val.length < 3)
return false;
c = val.charCodeAt(2);
if ((c <= 31 && c !== 9) || c > 255 || c === 127)
if (!validHdrChars[val.charCodeAt(2)])
return true;
if (val.length < 4)
return false;
if (!validHdrChars[val.charCodeAt(3)])
return true;
for (var i = 3; i < val.length; ++i) {
c = val.charCodeAt(i);
if ((c <= 31 && c !== 9) || c > 255 || c === 127)
for (var i = 4; i < val.length; ++i) {
if (!validHdrChars[val.charCodeAt(i)])
return true;
}
return false;
Expand Down
12 changes: 4 additions & 8 deletions lib/_http_incoming.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,21 +103,17 @@ IncomingMessage.prototype.destroy = function destroy(error) {
IncomingMessage.prototype._addHeaderLines = _addHeaderLines;
function _addHeaderLines(headers, n) {
if (headers && headers.length) {
var raw, dest;
var dest;
if (this.complete) {
raw = this.rawTrailers;
this.rawTrailers = headers;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this change, why was a separate array needed before? Can we assume this won't cause issues with the array being modified from the outside?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why a separate array was being created in this function. A new array is already created for every parse of an incoming message, so this particular change just holds onto that reference instead of having it go unused and being GC'ed after parsing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

dest = this.trailers;
} else {
raw = this.rawHeaders;
this.rawHeaders = headers;
dest = this.headers;
}

for (var i = 0; i < n; i += 2) {
var k = headers[i];
var v = headers[i + 1];
raw.push(k);
raw.push(v);
this._addHeaderLine(k, v, dest);
this._addHeaderLine(headers[i], headers[i + 1], dest);
}
}
}
Expand Down
75 changes: 41 additions & 34 deletions lib/_http_outgoing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ const util = require('util');
const internalUtil = require('internal/util');
const Buffer = require('buffer').Buffer;
const common = require('_http_common');
const checkIsHttpToken = common._checkIsHttpToken;
const checkInvalidHeaderChar = common._checkInvalidHeaderChar;

const CRLF = common.CRLF;
const trfrEncChunkExpression = common.chunkExpression;
Expand Down Expand Up @@ -207,23 +209,31 @@ function _storeHeader(firstLine, headers) {
messageHeader: firstLine
};

if (headers) {
var keys = Object.keys(headers);
var isArray = Array.isArray(headers);
var field, value;

for (var i = 0, l = keys.length; i < l; i++) {
var key = keys[i];
if (isArray) {
field = headers[key][0];
value = headers[key][1];
var i;
var j;
var field;
var value;
if (headers instanceof Array) {
for (i = 0; i < headers.length; ++i) {
field = headers[i][0];
value = headers[i][1];

if (value instanceof Array) {
for (j = 0; j < value.length; j++) {
storeHeader(this, state, field, value[j]);
}
} else {
field = key;
value = headers[key];
storeHeader(this, state, field, value);
}
}
} else if (headers) {
var keys = Object.keys(headers);
for (i = 0; i < keys.length; ++i) {
field = keys[i];
value = headers[field];

if (Array.isArray(value)) {
for (var j = 0; j < value.length; j++) {
if (value instanceof Array) {
for (j = 0; j < value.length; j++) {
storeHeader(this, state, field, value[j]);
}
} else {
Expand All @@ -237,7 +247,7 @@ function _storeHeader(firstLine, headers) {
this.upgrading = true;

// Date header
if (this.sendDate === true && state.sentDateHeader === false) {
if (this.sendDate && !state.sentDateHeader) {
state.messageHeader += 'Date: ' + utcDate() + CRLF;
}

Expand All @@ -253,8 +263,7 @@ function _storeHeader(firstLine, headers) {
// of creating security liabilities, so suppress the zero chunk and force
// the connection to close.
var statusCode = this.statusCode;
if ((statusCode === 204 || statusCode === 304) &&
this.chunkedEncoding === true) {
if ((statusCode === 204 || statusCode === 304) && this.chunkedEncoding) {
debug(statusCode + ' response should not use chunked encoding,' +
' closing connection.');
this.chunkedEncoding = false;
Expand All @@ -265,7 +274,7 @@ function _storeHeader(firstLine, headers) {
if (this._removedHeader.connection) {
this._last = true;
this.shouldKeepAlive = false;
} else if (state.sentConnectionHeader === false) {
} else if (!state.sentConnectionHeader) {
var shouldSendKeepAlive = this.shouldKeepAlive &&
(state.sentContentLengthHeader ||
this.useChunkedEncodingByDefault ||
Expand All @@ -278,8 +287,7 @@ function _storeHeader(firstLine, headers) {
}
}

if (state.sentContentLengthHeader === false &&
state.sentTransferEncodingHeader === false) {
if (!state.sentContentLengthHeader && !state.sentTransferEncodingHeader) {
if (!this._hasBody) {
// Make sure we don't end the 0\r\n\r\n at the end of the message.
this.chunkedEncoding = false;
Expand Down Expand Up @@ -312,11 +320,11 @@ function _storeHeader(firstLine, headers) {
}

function storeHeader(self, state, field, value) {
if (!common._checkIsHttpToken(field)) {
if (!checkIsHttpToken(field)) {
throw new TypeError(
'Header name must be a valid HTTP Token ["' + field + '"]');
}
if (common._checkInvalidHeaderChar(value) === true) {
if (checkInvalidHeaderChar(value)) {
debug('Header "%s" contains invalid characters', field);
throw new TypeError('The header content contains invalid characters');
}
Expand Down Expand Up @@ -350,14 +358,14 @@ function storeHeader(self, state, field, value) {


OutgoingMessage.prototype.setHeader = function setHeader(name, value) {
if (!common._checkIsHttpToken(name))
if (!checkIsHttpToken(name))
throw new TypeError(
'Header name must be a valid HTTP Token ["' + name + '"]');
if (value === undefined)
throw new Error('"value" required in setHeader("' + name + '", value)');
if (this._header)
throw new Error('Can\'t set headers after they are sent.');
if (common._checkInvalidHeaderChar(value) === true) {
if (checkInvalidHeaderChar(value)) {
debug('Header "%s" contains invalid characters', name);
throw new TypeError('The header content contains invalid characters');
}
Expand All @@ -380,8 +388,7 @@ OutgoingMessage.prototype.getHeader = function getHeader(name) {

if (!this._headers) return;

var key = name.toLowerCase();
return this._headers[key];
return this._headers[name.toLowerCase()];
};


Expand Down Expand Up @@ -531,11 +538,11 @@ OutgoingMessage.prototype.addTrailers = function addTrailers(headers) {
field = key;
value = headers[key];
}
if (!common._checkIsHttpToken(field)) {
if (!checkIsHttpToken(field)) {
throw new TypeError(
'Trailer name must be a valid HTTP Token ["' + field + '"]');
}
if (common._checkInvalidHeaderChar(value) === true) {
if (checkInvalidHeaderChar(value)) {
debug('Trailer "%s" contains invalid characters', field);
throw new TypeError('The trailer content contains invalid characters');
}
Expand All @@ -546,6 +553,9 @@ OutgoingMessage.prototype.addTrailers = function addTrailers(headers) {

const crlf_buf = Buffer.from('\r\n');

function onFinish(outmsg) {
outmsg.emit('finish');
}

OutgoingMessage.prototype.end = function end(data, encoding, callback) {
if (typeof data === 'function') {
Expand Down Expand Up @@ -585,7 +595,6 @@ OutgoingMessage.prototype.end = function end(data, encoding, callback) {
if (this.connection && data)
this.connection.cork();

var ret;
if (data) {
// Normal body write.
this.write(data, encoding);
Expand All @@ -594,10 +603,9 @@ OutgoingMessage.prototype.end = function end(data, encoding, callback) {
if (typeof callback === 'function')
this.once('finish', callback);

const finish = () => {
this.emit('finish');
};
var finish = onFinish.bind(undefined, this);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this use of bind is awesome. There might be more areas in which we could improve things.


var ret;
if (this._hasBody && this.chunkedEncoding) {
ret = this._send('0\r\n' + this._trailer + '\r\n', 'latin1', finish);
} else {
Expand Down Expand Up @@ -677,8 +685,7 @@ OutgoingMessage.prototype._flushOutput = function _flushOutput(socket) {
var outputCallbacks = this.outputCallbacks;
socket.cork();
for (var i = 0; i < outputLength; i++) {
ret = socket.write(output[i], outputEncodings[i],
outputCallbacks[i]);
ret = socket.write(output[i], outputEncodings[i], outputCallbacks[i]);
}
socket.uncork();

Expand Down
Loading