http: edit _storeHeader to check for Trailer header by arturgvieira-zz · Pull Request #12990 · nodejs/node · GitHub

Test non-chunked message does not have trailer header set,
message will be terminated by the first empty line after the
header fields, regardless of the header fields present in the
message, and thus cannot contain a message body or 'trailers'.

Ref: #2842

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

http

Please avoid hasOwnProperty() for performance reasons.

What would be a better choice?

Just try accessing the property and check if it is set to anything.

Thank you, will make the changes shortly.

This comment doesn't really describe the code below it well. The code doesn't even check for HEAD requests or 1xx/204 responses.

I will revise, thanks for the review.

@mscdex I made edits to the comment, it is more specific to the actual code.

Actually, now that I think more about it, just checking for 'Trailer' won't be enough because the letters could be in any case. I wonder if a better solution would be to just check for statusCode === 304 && state.trailer inside _storeHeader() in _http_outgoing.js? Perhaps it could be merged with the existing 204/304 check that's already being done in there.

I also don't know if we should throw or just discard the trailer header...
Thoughts @nodejs/http ?

All good insights. #2842 had a mention about throwing. If it is preferred to discard I'll make the change.

@mscdex All done. I updated the location of the check, thank you for pointing out _storeHeader.

Should the ? be a $?

Yes, I'll correct it. Thank you

common.mustCall() is not needed here.

I will update it. Thanks

@cjihrig All done, I corrected the errors. Thanks for the review.

@cjihrig Hi, if you have time, could you check if I need to make any further changes?

LGTM. I'd like feedback from @nodejs/http though.

Thank you.

I've read RFC 7230 a bit more on this and my understanding/interpretation is that a Trailer header should be fine for 304 as it could indicate that a trailer would have been sent if the request was an unconditional GET (the RFC gives a similar example/explanation for a Transfer-Encoding header sent in a 304). Node already doesn't send the actual trailer for such status codes.

That's my understanding as well @mscdex but the spec is rather vague on the topic.

I was actually interested in closing the issue. I read over the RFC as well and I am fine with whatever you guys decide. The Bluemix story may be a concern? Thoughts.

I think the original linked issue seems to be suggesting that any Trailer header should be removed when Transfer-Encoding: chunked is not set since Trailer is a component of a chunk encoded response, regardless of status code. I guess that makes sense to me, but it'd definitely be a semver-major change IMHO.

The Bluemix story may be a concern? Thoughts.

@arturgvieira could you elaborate? Not sure what the Bluemix story is.

Ahh, the linked issue (#2842), got it!

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/831/
CI: https://ci.nodejs.org/job/node-test-commit/10164/

I think the original linked issue seems to be suggesting that any Trailer header should be removed when Transfer-Encoding: chunked is not set since Trailer is a component of a chunk encoded response, regardless of status code. I guess that makes sense to me, but it'd definitely be a semver-major change IMHO.

@mscdex @jasnell Just to be clear, this case (304 only) is considered semver-patch?

@refack I think I'd rather just have one change for this particular issue and currently, discarding the trailer on a non-chunked response seems like the best option to me (and that would be semver-major).

@refack I think I'd rather just have one change for this particular issue and currently, discarding the trailer on a non-chunked response seems like the best option to me (and that would be semver-major).

So I'm blocking this.

@arturgvieira go for the gold... i.e. discard the trailer for all non-chunked responses.
P.S. since headers can be added in any order I'm not sure you can do the trick you did here...

Sorry, I meant the Trailer header, actual trailer content should already be discarded for non-chunked responses.

I'm checking for lint errors.

All lint errors have been corrected.

@arturgvieira we don't have consensus on this fix. @mscdex and I think that you should solve the general case #12990 (comment).
That is:

move the test to around L434(https://github.com/nodejs/node/pull/12990/files#diff-286202fdbdd74ede6f5f5334b6176b5cR434)
Check for this.chunkedEncoding !== true && state.trailer
Evaluate if response.addTrailers need modifications
Document in https://github.com/nodejs/node/blob/master/doc/api/http.md#responsesetheadername-value and response.addTrailers.
Expand the test.

I don't think addTrailers() needs any changes, since that content is already being ignored properly. All we should care about is skipping the Trailer header when chunked encoding is not being used. I don't think there is going to be a straight-forward change because a default chunked encoding header could be appended after the header is already generated in _storeHeader().

All done, I added the additional information for the error as requested.

💯

CI: https://ci.nodejs.org/job/node-test-commit/10506/

/cc @nodejs/ctc

LGTM

I think this can be simplified to just throw new errors.Error(...)?

Perhaps move the object literal into a separate variable and pass that to common.expectsError() to make this more readable?:

const trailerInvalidErr = {
  code: 'ERR_HTTP_TRAILER_INVALID',
  message: 'Trailers are invalid with this transfer encoding',
  type: Error
};
assert.throws(() => res.writeHead(200, {'Content-Length': '24'}),
              common.expectsError(trailerInvalidErr));

This description does not agree with the code below.

All done, made the changes requested

Minor nit: you might add a little extra code here to verify that the response is what you expect (both statusCode and body content). In doing so, you'd need to make sure to adjust the Content-Length value to 2.

Ok, will do.

Made the requested changes. Thanks @mscdex

This should just be .strictEqual().

Ok

Technically it would be better to just buffer data in here (without common.mustCall()) and then assert.strictEqual(buffer, 'ok') in a common.mustCall()-wrapped 'end' event handler:

var buf = '';
res.on('data', (chunk) => {
  buf += chunk;
}).on('end', common.mustCall(() => {
  assert.strictEqual(buf, 'ok');
}));

The reason being in general you shouldn't assume anything about the number of 'data' events for a stream.

@mscdex Take a look, I think this is what you meant.

IMO this is less readable, I think it's best to just always append to a variable that is initially an empty string. Also you will be able to drop the explicit buffer.toString().

Ok, I just saw your edit. Changing now.

Test non-chunked message does not have trailer header set, message will be terminated by the first empty line after the header fields, regardless of the header fields present in the message, and thus cannot contain a message body or 'trailers'. Ref: #2842

@mscdex Thank you for the review.

CI: https://ci.nodejs.org/job/node-test-pull-request/8620/

LGTM

@jasnell PTAL (even though we have 3 CTC LGTMs)

Test non-chunked message does not have trailer header set, message will be terminated by the first empty line after the header fields, regardless of the header fields present in the message, and thus cannot contain a message body or 'trailers'. PR-URL: #12990 Ref: #2842 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Landed in 80c9ef0

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label May 12, 2017

arturgvieira-zz changed the title Server header branch http: edit writeHead to check for 304 and Trailer May 12, 2017

mscdex reviewed May 12, 2017

View reviewed changes

arturgvieira-zz changed the title http: edit writeHead to check for 304 and Trailer http: edit _storeHeader to check for 304 and Trailer May 12, 2017

cjihrig requested changes May 12, 2017

View reviewed changes

cjihrig approved these changes May 17, 2017

View reviewed changes

refack self-assigned this May 25, 2017

refack added the blocked PRs that are blocked by other issues or PRs. label May 25, 2017

refack approved these changes Jun 11, 2017

View reviewed changes

mcollina approved these changes Jun 12, 2017

View reviewed changes

refack removed the wip Issues and PRs that are still a work in progress. label Jun 12, 2017

mscdex reviewed Jun 12, 2017

View reviewed changes

mscdex reviewed Jun 13, 2017

View reviewed changes

hiroppy approved these changes Jun 13, 2017

View reviewed changes

jasnell approved these changes Jun 13, 2017

View reviewed changes

jasnell closed this Jun 13, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

refack removed their assignment Oct 20, 2018

bjohansebas mentioned this pull request Mar 27, 2025

Trailer header field included with 304 expressjs/express#2749

Closed

coderabbitai Bot mentioned this pull request Jul 9, 2026

node:http: emit writeHead raw-array headers in the caller's order oven-sh/bun#33870

Open

http: edit _storeHeader to check for Trailer header by arturgvieira-zz · Pull Request #12990 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: edit _storeHeader to check for Trailer header - #12990

http: edit _storeHeader to check for Trailer header#12990arturgvieira-zz wants to merge 1 commit intonodejs:masternodejs/node:masterfrom arturgvieira-zz:serverHeader-branchCopy head branch name to clipboard

Conversation

arturgvieira-zz commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arturgvieira-zz commented May 12, 2017

Uh oh!

mscdex May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arturgvieira-zz May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arturgvieira-zz commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arturgvieira-zz commented May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arturgvieira-zz commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

arturgvieira-zz commented May 17, 2017

Uh oh!

mscdex commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 17, 2017

Uh oh!

arturgvieira-zz commented May 17, 2017

Uh oh!

mscdex commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented May 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented May 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented May 25, 2017

Uh oh!

mscdex commented May 25, 2017

Uh oh!

refack commented May 25, 2017

Uh oh!

refack commented May 25, 2017

Uh oh!

mscdex commented May 25, 2017

Uh oh!

http: edit _storeHeader to check for Trailer header#12990
arturgvieira-zz wants to merge 1 commit into
nodejs:masterfrom
arturgvieira-zz:serverHeader-branch

arturgvieira-zz commented May 12, 2017 •
edited
Loading

mscdex May 12, 2017 •
edited
Loading

arturgvieira-zz May 12, 2017 •
edited
Loading

arturgvieira-zz commented May 12, 2017 •
edited
Loading

arturgvieira-zz commented May 17, 2017 •
edited
Loading

mscdex commented May 17, 2017 •
edited
Loading

mscdex commented May 17, 2017 •
edited
Loading

gibfahn commented May 20, 2017 •
edited
Loading

refack commented May 25, 2017 •
edited
Loading

refack commented May 25, 2017 •
edited
Loading

mscdex commented May 26, 2017 •
edited
Loading

arturgvieira-zz Jun 13, 2017 •
edited
Loading

mscdex Jun 13, 2017 •
edited
Loading

mscdex Jun 13, 2017 •
edited
Loading