Skip to content

Commit 01c1a4d

Browse files
pimterryHoonDongKang
pimterry
authored and
HoonDongKang
committed
benchmark: fix shadowing that broke h=20 on incoming_headers benchmark
Previously the inner 'headers' variable shadowed the outer, meaning that headers=20 sends the same 7 headers as headers=0. Signed-off-by: Tim Perry <pimterry@gmail.com> PR-URL: nodejs#66257 Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent 98ebbe2 commit 01c1a4d

3 files changed

Lines changed: 135 additions & 57 deletions

File tree

‎benchmark/http/incoming_headers.js‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,20 @@ const bench = common.createBenchmark(main, {
66
connections: [50], // Concurrent connections
77
headers: [20], // Number of header lines to append after the common headers
88
w: [0, 6], // Amount of trailing whitespace
9+
read: [0, 1], // Whether the handler reads req.headers
910
duration: 5,
1011
});
1112

12-
function main({ connections, headers, w, duration }) {
13+
function main({ connections, headers, w, read, duration }) {
1314
const server = http.createServer((req, res) => {
15+
if (read && req.headers.host === undefined) {
16+
throw new Error('Missing Host header');
17+
}
1418
res.end();
1519
});
1620

1721
server.listen(0, () => {
18-
const headers = {
22+
const requestHeaders = {
1923
'Content-Type': 'text/plain',
2024
'Accept': 'text/plain',
2125
'User-Agent': 'nodejs-benchmark',
@@ -28,12 +32,12 @@ function main({ connections, headers, w, duration }) {
2832
// - wrk can only send trailing OWS. This is a side-effect of wrk
2933
// processing requests with http-parser before sending them, causing
3034
// leading OWS to be stripped.
31-
headers[`foo${i}`] = `some header value ${i}${' \t'.repeat(w / 2)}`;
35+
requestHeaders[`foo${i}`] = `some header value ${i}${' \t'.repeat(w / 2)}`;
3236
}
3337
bench.http({
3438
path: '/',
3539
connections,
36-
headers,
40+
headers: requestHeaders,
3741
duration,
3842
port: server.address().port,
3943
}, () => {

‎lib/_http_incoming.js‎

Lines changed: 109 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -340,109 +340,161 @@ function _addHeaderLines(headers, n) {
340340
}
341341

342342

343-
// This function is used to help avoid the lowercasing of a field name if it
344-
// matches a 'traditional cased' version of a field name. It then returns the
345-
// lowercased name to both avoid calling toLowerCase() a second time and to
346-
// indicate whether the field was a 'no duplicates' field. If a field is not a
347-
// 'no duplicates' field, a `0` byte is prepended as a flag. The one exception
348-
// to this is the Set-Cookie header which is indicated by a `1` byte flag, since
349-
// it is an 'array' field and thus is treated differently in _addHeaderLines().
343+
// How repeated instances of a header field are combined in the headers
344+
// object. Fields that are not known are joined with ', '.
345+
const kFirstWins = 0;
346+
const kJoinComma = 1;
347+
const kJoinSemicolon = 2;
348+
const kArray = 3;
349+
350+
function knownField(name, merge) {
351+
return { name, merge };
352+
}
353+
354+
// Later values are dropped, unless joinDuplicateHeaders is set.
355+
const kAge = knownField('age', kFirstWins);
356+
const kHost = knownField('host', kFirstWins);
357+
const kFrom = knownField('from', kFirstWins);
358+
const kETag = knownField('etag', kFirstWins);
359+
const kServer = knownField('server', kFirstWins);
360+
const kReferer = knownField('referer', kFirstWins);
361+
const kExpires = knownField('expires', kFirstWins);
362+
const kLocation = knownField('location', kFirstWins);
363+
const kUserAgent = knownField('user-agent', kFirstWins);
364+
const kRetryAfter = knownField('retry-after', kFirstWins);
365+
const kContentType = knownField('content-type', kFirstWins);
366+
const kMaxForwards = knownField('max-forwards', kFirstWins);
367+
const kAuthorization = knownField('authorization', kFirstWins);
368+
const kLastModified = knownField('last-modified', kFirstWins);
369+
const kContentLength = knownField('content-length', kFirstWins);
370+
const kIfModifiedSince = knownField('if-modified-since', kFirstWins);
371+
const kProxyAuthorization = knownField('proxy-authorization', kFirstWins);
372+
const kIfUnmodifiedSince = knownField('if-unmodified-since', kFirstWins);
373+
374+
// Values are joined with ', '.
375+
const kDate = knownField('date', kJoinComma);
376+
const kVary = knownField('vary', kJoinComma);
377+
const kOrigin = knownField('origin', kJoinComma);
378+
const kExpect = knownField('expect', kJoinComma);
379+
const kAccept = knownField('accept', kJoinComma);
380+
const kUpgrade = knownField('upgrade', kJoinComma);
381+
const kIfMatch = knownField('if-match', kJoinComma);
382+
const kConnection = knownField('connection', kJoinComma);
383+
const kCacheControl = knownField('cache-control', kJoinComma);
384+
const kIfNoneMatch = knownField('if-none-match', kJoinComma);
385+
const kAcceptEncoding = knownField('accept-encoding', kJoinComma);
386+
const kAcceptLanguage = knownField('accept-language', kJoinComma);
387+
const kXForwardedFor = knownField('x-forwarded-for', kJoinComma);
388+
const kContentEncoding = knownField('content-encoding', kJoinComma);
389+
const kXForwardedHost = knownField('x-forwarded-host', kJoinComma);
390+
const kTransferEncoding = knownField('transfer-encoding', kJoinComma);
391+
const kXForwardedProto = knownField('x-forwarded-proto', kJoinComma);
392+
393+
// Values are joined with '; '.
394+
const kCookie = knownField('cookie', kJoinSemicolon);
395+
396+
// Values are collected into an array.
397+
const kSetCookie = knownField('set-cookie', kArray);
398+
399+
// Returns the descriptor of a known field, or the lowercased name of any other
400+
// field. The 'traditional cased' and lowercase spellings of known fields are
401+
// matched first to avoid calling toLowerCase() for them.
350402
// TODO: perhaps http_parser could be returning both raw and lowercased versions
351403
// of known header names to avoid us having to call toLowerCase() for those
352404
// headers.
353405
function matchKnownFields(field, lowercased) {
354406
switch (field.length) {
355407
case 3:
356-
if (field === 'Age' || field === 'age') return 'age';
408+
if (field === 'Age' || field === 'age') return kAge;
357409
break;
358410
case 4:
359-
if (field === 'Host' || field === 'host') return 'host';
360-
if (field === 'From' || field === 'from') return 'from';
361-
if (field === 'ETag' || field === 'etag') return 'etag';
362-
if (field === 'Date' || field === 'date') return '\u0000date';
363-
if (field === 'Vary' || field === 'vary') return '\u0000vary';
411+
if (field === 'Host' || field === 'host') return kHost;
412+
if (field === 'From' || field === 'from') return kFrom;
413+
if (field === 'ETag' || field === 'etag') return kETag;
414+
if (field === 'Date' || field === 'date') return kDate;
415+
if (field === 'Vary' || field === 'vary') return kVary;
364416
break;
365417
case 6:
366-
if (field === 'Server' || field === 'server') return 'server';
367-
if (field === 'Cookie' || field === 'cookie') return '\u0002cookie';
368-
if (field === 'Origin' || field === 'origin') return '\u0000origin';
369-
if (field === 'Expect' || field === 'expect') return '\u0000expect';
370-
if (field === 'Accept' || field === 'accept') return '\u0000accept';
418+
if (field === 'Server' || field === 'server') return kServer;
419+
if (field === 'Cookie' || field === 'cookie') return kCookie;
420+
if (field === 'Origin' || field === 'origin') return kOrigin;
421+
if (field === 'Expect' || field === 'expect') return kExpect;
422+
if (field === 'Accept' || field === 'accept') return kAccept;
371423
break;
372424
case 7:
373-
if (field === 'Referer' || field === 'referer') return 'referer';
374-
if (field === 'Expires' || field === 'expires') return 'expires';
375-
if (field === 'Upgrade' || field === 'upgrade') return '\u0000upgrade';
425+
if (field === 'Referer' || field === 'referer') return kReferer;
426+
if (field === 'Expires' || field === 'expires') return kExpires;
427+
if (field === 'Upgrade' || field === 'upgrade') return kUpgrade;
376428
break;
377429
case 8:
378430
if (field === 'Location' || field === 'location')
379-
return 'location';
431+
return kLocation;
380432
if (field === 'If-Match' || field === 'if-match')
381-
return '\u0000if-match';
433+
return kIfMatch;
382434
break;
383435
case 10:
384436
if (field === 'User-Agent' || field === 'user-agent')
385-
return 'user-agent';
437+
return kUserAgent;
386438
if (field === 'Set-Cookie' || field === 'set-cookie')
387-
return '\u0001';
439+
return kSetCookie;
388440
if (field === 'Connection' || field === 'connection')
389-
return '\u0000connection';
441+
return kConnection;
390442
break;
391443
case 11:
392444
if (field === 'Retry-After' || field === 'retry-after')
393-
return 'retry-after';
445+
return kRetryAfter;
394446
break;
395447
case 12:
396448
if (field === 'Content-Type' || field === 'content-type')
397-
return 'content-type';
449+
return kContentType;
398450
if (field === 'Max-Forwards' || field === 'max-forwards')
399-
return 'max-forwards';
451+
return kMaxForwards;
400452
break;
401453
case 13:
402454
if (field === 'Authorization' || field === 'authorization')
403-
return 'authorization';
455+
return kAuthorization;
404456
if (field === 'Last-Modified' || field === 'last-modified')
405-
return 'last-modified';
457+
return kLastModified;
406458
if (field === 'Cache-Control' || field === 'cache-control')
407-
return '\u0000cache-control';
459+
return kCacheControl;
408460
if (field === 'If-None-Match' || field === 'if-none-match')
409-
return '\u0000if-none-match';
461+
return kIfNoneMatch;
410462
break;
411463
case 14:
412464
if (field === 'Content-Length' || field === 'content-length')
413-
return 'content-length';
465+
return kContentLength;
414466
break;
415467
case 15:
416468
if (field === 'Accept-Encoding' || field === 'accept-encoding')
417-
return '\u0000accept-encoding';
469+
return kAcceptEncoding;
418470
if (field === 'Accept-Language' || field === 'accept-language')
419-
return '\u0000accept-language';
471+
return kAcceptLanguage;
420472
if (field === 'X-Forwarded-For' || field === 'x-forwarded-for')
421-
return '\u0000x-forwarded-for';
473+
return kXForwardedFor;
422474
break;
423475
case 16:
424476
if (field === 'Content-Encoding' || field === 'content-encoding')
425-
return '\u0000content-encoding';
477+
return kContentEncoding;
426478
if (field === 'X-Forwarded-Host' || field === 'x-forwarded-host')
427-
return '\u0000x-forwarded-host';
479+
return kXForwardedHost;
428480
break;
429481
case 17:
430482
if (field === 'If-Modified-Since' || field === 'if-modified-since')
431-
return 'if-modified-since';
483+
return kIfModifiedSince;
432484
if (field === 'Transfer-Encoding' || field === 'transfer-encoding')
433-
return '\u0000transfer-encoding';
485+
return kTransferEncoding;
434486
if (field === 'X-Forwarded-Proto' || field === 'x-forwarded-proto')
435-
return '\u0000x-forwarded-proto';
487+
return kXForwardedProto;
436488
break;
437489
case 19:
438490
if (field === 'Proxy-Authorization' || field === 'proxy-authorization')
439-
return 'proxy-authorization';
491+
return kProxyAuthorization;
440492
if (field === 'If-Unmodified-Since' || field === 'if-unmodified-since')
441-
return 'if-unmodified-since';
493+
return kIfUnmodifiedSince;
442494
break;
443495
}
444496
if (lowercased) {
445-
return '\u0000' + field;
497+
return field;
446498
}
447499
return matchKnownFields(field.toLowerCase(), true);
448500
}
@@ -453,21 +505,25 @@ function matchKnownFields(field, lowercased) {
453505
// multiple values this way. The one exception to this is the Cookie header,
454506
// which has multiple values joined with a '; ' instead. If a header's values
455507
// cannot be joined in either of these ways, we declare the first instance the
456-
// winner and drop the second. Extended header fields (those beginning with
457-
// 'x-') are always joined.
508+
// winner and drop the second. Fields that are not known are always joined.
458509
IncomingMessage.prototype._addHeaderLine = _addHeaderLine;
459510
function _addHeaderLine(field, value, dest) {
460-
field = matchKnownFields(field);
461-
const flag = field.charCodeAt(0);
462-
if (flag === 0 || flag === 2) {
463-
field = field.slice(1);
511+
const known = matchKnownFields(field);
512+
let merge = kJoinComma;
513+
if (typeof known === 'string') {
514+
field = known;
515+
} else {
516+
field = known.name;
517+
merge = known.merge;
518+
}
519+
if (merge === kJoinComma || merge === kJoinSemicolon) {
464520
// Make a delimited list
465521
if (typeof dest[field] === 'string') {
466-
dest[field] += (flag === 0 ? ', ' : '; ') + value;
522+
dest[field] += (merge === kJoinComma ? ', ' : '; ') + value;
467523
} else {
468524
dest[field] = value;
469525
}
470-
} else if (flag === 1) {
526+
} else if (merge === kArray) {
471527
// Array header -- only Set-Cookie at the moment
472528
if (dest['set-cookie'] !== undefined) {
473529
dest['set-cookie'].push(value);

‎test/parallel/test-http-incoming-matchKnownFields.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,3 +91,21 @@ checkDest('X-Forwarded-Proto', { 'x-forwarded-proto': undefined });
9191
checkDest('x-forwarded-proto', { 'x-forwarded-proto': 'test, value' }, 'value');
9292
checkDest('X-Foo', { 'x-foo': undefined });
9393
checkDest('x-foo', { 'x-foo': 'test, value' }, 'value');
94+
95+
// Known fields in other casings are merged by the same rules as their usual
96+
// spellings.
97+
checkDest('CONTENT-TYPE', { 'content-type': 'test' }, 'value');
98+
checkDest('CONNECTION', { connection: 'test, value' }, 'value');
99+
checkDest('COOKIE', { cookie: 'test; value' }, 'value');
100+
checkDest('SET-COOKIE', { 'set-cookie': ['test', 'value'] }, 'value');
101+
checkDest('X-FOO', { 'x-foo': 'test, value' }, 'value');
102+
103+
// joinDuplicateHeaders also applies to first-wins fields in other casings.
104+
{
105+
const incomingMessage = new IncomingMessage();
106+
incomingMessage.joinDuplicateHeaders = true;
107+
const dest = {};
108+
incomingMessage._addHeaderLine('AUTHORIZATION', 'a', dest);
109+
incomingMessage._addHeaderLine('Authorization', 'b', dest);
110+
assert.deepStrictEqual(dest, { authorization: 'a, b' });
111+
}

0 commit comments

Comments
 (0)