From fb3ba941943c4a42ee8beefc9a634048b3e9b606 Mon Sep 17 00:00:00 2001 From: tsctx <91457664+tsctx@users.noreply.github.com> Date: Thu, 29 Feb 2024 23:19:29 +0900 Subject: [PATCH 1/4] perf: optimize check invalid field-vchar --- lib/core/request.js | 24 ++++++------------------ lib/core/util.js | 20 +++++++++++++++++++- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/lib/core/request.js b/lib/core/request.js index dc136408b55..cf325b54119 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -9,17 +9,6 @@ const util = require('./util') const { channels } = require('./diagnostics.js') const { headerNameLowerCasedRecord } = require('./constants') -// headerCharRegex have been lifted from -// https://github.com/nodejs/node/blob/main/lib/_http_common.js - -/** - * Matches if val contains an invalid field-vchar - * field-value = *( field-content / obs-fold ) - * field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] - * field-vchar = VCHAR / obs-text - */ -const headerCharRegex = /[^\t\x20-\x7e\x80-\xff]/ - // Verifies that a given path is valid does not contain control chars \x00 to \x20 const invalidPathRegex = /[^\u0021-\u00ff]/ @@ -166,13 +155,12 @@ class Request { if (!Array.isArray(header) || header.length !== 2) { throw new InvalidArgumentError('headers must be in key-value pair format') } - const [key, value] = header - processHeader(this, key, value) + processHeader(this, header[0], header[1]) } } else { const keys = Object.keys(headers) - for (const key of keys) { - processHeader(this, key, headers[key]) + for (let i = 0; i < keys.length; ++i) { + processHeader(this, keys[i], headers[keys[i]]) } } } else if (headers != null) { @@ -315,7 +303,7 @@ class Request { } } -function processHeader (request, key, val, skipAppend = false) { +function processHeader (request, key, val) { if (val && (typeof val === 'object' && !Array.isArray(val))) { throw new InvalidArgumentError(`invalid ${key} header`) } else if (val === undefined) { @@ -335,7 +323,7 @@ function processHeader (request, key, val, skipAppend = false) { const arr = [] for (let i = 0; i < val.length; i++) { if (typeof val[i] === 'string') { - if (headerCharRegex.exec(val[i]) !== null) { + if (!util.isValidHeaderChar(val[i])) { throw new InvalidArgumentError(`invalid ${key} header`) } arr.push(val[i]) @@ -349,7 +337,7 @@ function processHeader (request, key, val, skipAppend = false) { } val = arr } else if (typeof val === 'string') { - if (headerCharRegex.exec(val) !== null) { + if (!util.isValidHeaderChar(val)) { throw new InvalidArgumentError(`invalid ${key} header`) } } else if (val === null) { diff --git a/lib/core/util.js b/lib/core/util.js index 9789a240575..cf6fd410db0 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -495,6 +495,24 @@ function isValidHTTPToken (characters) { return true } +/** + * @param {string} characters + */ +function isValidHeaderChar (characters) { + // Validate if val is a valid field-vchar. + // field-value = *( field-content / obs-fold ) + // field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] + // field-vchar = VCHAR / obs-text + for (let i = 0; i < characters.length; ++i) { + const code = characters.charCodeAt(i) + // not \x20-\x7e, \t and \x80-\xff + if ((code < 0x20 && code !== 0x09) || code === 0x7f || code > 0xff) { + return false + } + } + return true +} + // Parsed accordingly to RFC 9110 // https://www.rfc-editor.org/rfc/rfc9110#field.content-range function parseRangeHeader (range) { @@ -516,7 +534,6 @@ kEnumerableProperty.enumerable = true module.exports = { kEnumerableProperty, nop, - isDisturbed, isErrored, isReadable, @@ -546,6 +563,7 @@ module.exports = { buildURL, addAbortListener, isValidHTTPToken, + isValidHeaderChar, isTokenCharCode, parseRangeHeader, nodeMajor, From 75fa33467db63f98ff2ff3c31dc97085a7cbd6c1 Mon Sep 17 00:00:00 2001 From: tsctx <91457664+tsctx@users.noreply.github.com> Date: Fri, 1 Mar 2024 08:06:39 +0900 Subject: [PATCH 2/4] add benchmark, use named import, fix regression --- benchmarks/core/is-valid-header-char.mjs | 57 ++++++++++++++++++++++++ lib/core/request.js | 36 ++++++++++----- lib/core/util.js | 24 +++++----- 3 files changed, 93 insertions(+), 24 deletions(-) create mode 100644 benchmarks/core/is-valid-header-char.mjs diff --git a/benchmarks/core/is-valid-header-char.mjs b/benchmarks/core/is-valid-header-char.mjs new file mode 100644 index 00000000000..7189d483335 --- /dev/null +++ b/benchmarks/core/is-valid-header-char.mjs @@ -0,0 +1,57 @@ +import { bench, group, run } from 'mitata' +import { isValidHeaderChar } from '../../lib/core/util.js' + +const html = 'text/html' +const json = 'application/json; charset=UTF-8' + +const headerCharRegex = /[^\t\x20-\x7e\x80-\xff]/ + +/** + * @param {string} characters + */ +function charCodeAtApproach (characters) { + // Validate if characters is a valid field-vchar. + // field-value = *( field-content / obs-fold ) + // field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] + // field-vchar = VCHAR / obs-text + for (let i = 0; i < characters.length; ++i) { + const code = characters.charCodeAt(i) + // not \x20-\x7e, \t and \x80-\xff + if ((code < 0x20 && code !== 0x09) || code === 0x7f || code > 0xff) { + return false + } + } + return true +} + +group(`isValidHeaderChar# ${html}`, () => { + bench('regexp.test', () => { + return !headerCharRegex.test(html) + }) + bench('regexp.exec', () => { + return headerCharRegex.exec(html) === null + }) + bench('charCodeAt', () => { + return charCodeAtApproach(html) === true + }) + bench('isValidHeaderChar', () => { + return isValidHeaderChar(html) === true + }) +}) + +group(`isValidHeaderChar ${json}`, () => { + bench('regexp.test', () => { + return !headerCharRegex.test(json) + }) + bench('regexp.exec', () => { + return headerCharRegex.exec(json) === null + }) + bench('charCodeAt', () => { + return charCodeAtApproach(json) + }) + bench('isValidHeaderChar', () => { + return isValidHeaderChar(json) + }) +}) + +await run() diff --git a/lib/core/request.js b/lib/core/request.js index cf325b54119..45f349c85c6 100644 --- a/lib/core/request.js +++ b/lib/core/request.js @@ -5,7 +5,19 @@ const { NotSupportedError } = require('./errors') const assert = require('node:assert') -const util = require('./util') +const { + isValidHTTPToken, + isValidHeaderChar, + isStream, + destroy, + isBuffer, + isFormDataLike, + isIterable, + isBlobLike, + buildURL, + validateHandler, + getServerName +} = require('./util') const { channels } = require('./diagnostics.js') const { headerNameLowerCasedRecord } = require('./constants') @@ -44,7 +56,7 @@ class Request { if (typeof method !== 'string') { throw new InvalidArgumentError('method must be a string') - } else if (!util.isValidHTTPToken(method)) { + } else if (!isValidHTTPToken(method)) { throw new InvalidArgumentError('invalid request method') } @@ -80,13 +92,13 @@ class Request { if (body == null) { this.body = null - } else if (util.isStream(body)) { + } else if (isStream(body)) { this.body = body const rState = this.body._readableState if (!rState || !rState.autoDestroy) { this.endHandler = function autoDestroy () { - util.destroy(this) + destroy(this) } this.body.on('end', this.endHandler) } @@ -99,7 +111,7 @@ class Request { } } this.body.on('error', this.errorHandler) - } else if (util.isBuffer(body)) { + } else if (isBuffer(body)) { this.body = body.byteLength ? body : null } else if (ArrayBuffer.isView(body)) { this.body = body.buffer.byteLength ? Buffer.from(body.buffer, body.byteOffset, body.byteLength) : null @@ -107,7 +119,7 @@ class Request { this.body = body.byteLength ? Buffer.from(body) : null } else if (typeof body === 'string') { this.body = body.length ? Buffer.from(body) : null - } else if (util.isFormDataLike(body) || util.isIterable(body) || util.isBlobLike(body)) { + } else if (isFormDataLike(body) || isIterable(body) || isBlobLike(body)) { this.body = body } else { throw new InvalidArgumentError('body must be a string, a Buffer, a Readable stream, an iterable, or an async iterable') @@ -119,7 +131,7 @@ class Request { this.upgrade = upgrade || null - this.path = query ? util.buildURL(path, query) : path + this.path = query ? buildURL(path, query) : path this.origin = origin @@ -167,9 +179,9 @@ class Request { throw new InvalidArgumentError('headers must be an object or an array') } - util.validateHandler(handler, method, upgrade) + validateHandler(handler, method, upgrade) - this.servername = util.getServerName(this.host) + this.servername = getServerName(this.host) this[kHandler] = handler @@ -314,7 +326,7 @@ function processHeader (request, key, val) { if (headerName === undefined) { headerName = key.toLowerCase() - if (headerNameLowerCasedRecord[headerName] === undefined && !util.isValidHTTPToken(headerName)) { + if (headerNameLowerCasedRecord[headerName] === undefined && !isValidHTTPToken(headerName)) { throw new InvalidArgumentError('invalid header key') } } @@ -323,7 +335,7 @@ function processHeader (request, key, val) { const arr = [] for (let i = 0; i < val.length; i++) { if (typeof val[i] === 'string') { - if (!util.isValidHeaderChar(val[i])) { + if (!isValidHeaderChar(val[i])) { throw new InvalidArgumentError(`invalid ${key} header`) } arr.push(val[i]) @@ -337,7 +349,7 @@ function processHeader (request, key, val) { } val = arr } else if (typeof val === 'string') { - if (!util.isValidHeaderChar(val)) { + if (!isValidHeaderChar(val)) { throw new InvalidArgumentError(`invalid ${key} header`) } } else if (val === null) { diff --git a/lib/core/util.js b/lib/core/util.js index cf6fd410db0..cbb6d7495e5 100644 --- a/lib/core/util.js +++ b/lib/core/util.js @@ -495,22 +495,22 @@ function isValidHTTPToken (characters) { return true } +// headerCharRegex have been lifted from +// https://github.com/nodejs/node/blob/main/lib/_http_common.js + +/** + * Matches if val contains an invalid field-vchar + * field-value = *( field-content / obs-fold ) + * field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] + * field-vchar = VCHAR / obs-text + */ +const headerCharRegex = /[^\t\x20-\x7e\x80-\xff]/ + /** * @param {string} characters */ function isValidHeaderChar (characters) { - // Validate if val is a valid field-vchar. - // field-value = *( field-content / obs-fold ) - // field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] - // field-vchar = VCHAR / obs-text - for (let i = 0; i < characters.length; ++i) { - const code = characters.charCodeAt(i) - // not \x20-\x7e, \t and \x80-\xff - if ((code < 0x20 && code !== 0x09) || code === 0x7f || code > 0xff) { - return false - } - } - return true + return !headerCharRegex.test(characters) } // Parsed accordingly to RFC 9110 From 1cf69281c1d235d1f298b0a1e8a60554e72430eb Mon Sep 17 00:00:00 2001 From: tsctx <91457664+tsctx@users.noreply.github.com> Date: Fri, 1 Mar 2024 08:10:21 +0900 Subject: [PATCH 3/4] fix benchmark --- benchmarks/core/is-valid-header-char.mjs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/benchmarks/core/is-valid-header-char.mjs b/benchmarks/core/is-valid-header-char.mjs index 7189d483335..1ee93fe4aaa 100644 --- a/benchmarks/core/is-valid-header-char.mjs +++ b/benchmarks/core/is-valid-header-char.mjs @@ -32,10 +32,10 @@ group(`isValidHeaderChar# ${html}`, () => { return headerCharRegex.exec(html) === null }) bench('charCodeAt', () => { - return charCodeAtApproach(html) === true + return charCodeAtApproach(html) }) bench('isValidHeaderChar', () => { - return isValidHeaderChar(html) === true + return isValidHeaderChar(html) }) }) From e73a0f35c41602821499c9131a4632777b263c5f Mon Sep 17 00:00:00 2001 From: tsctx <91457664+tsctx@users.noreply.github.com> Date: Fri, 1 Mar 2024 20:21:52 +0900 Subject: [PATCH 4/4] apply suggestions from code review --- benchmarks/core/is-valid-header-char.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchmarks/core/is-valid-header-char.mjs b/benchmarks/core/is-valid-header-char.mjs index 1ee93fe4aaa..4734d119011 100644 --- a/benchmarks/core/is-valid-header-char.mjs +++ b/benchmarks/core/is-valid-header-char.mjs @@ -39,7 +39,7 @@ group(`isValidHeaderChar# ${html}`, () => { }) }) -group(`isValidHeaderChar ${json}`, () => { +group(`isValidHeaderChar# ${json}`, () => { bench('regexp.test', () => { return !headerCharRegex.test(json) })