http: enable setHeader for call chaining

[PoojaDurgad]

Make response.setHeader return the response object itself
so that multiple header setting can be chained.

Fixes: #33148

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

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[mscdex]

The targeted subsystem can just be http in the commit message instead of lib,http since http is the only thing being changed here.

[lpinca]

Can you please add a test?

[lpinca]

I'm fine with the test file as is but it could be simplified to something like

'use strict';require('../common');constassert=require('assert');const{ ServerResponse }=require('http');constresponse=newServerResponse({method: 'GET',httpVersionMajor: 1,httpVersionMinor: 1});assert.strictEqual(response.setHeader('foo','bar'),response);

or just a single assertion like the one above in an already existing test where response.setHeader() is used.

There is no need to start a server and chaining is a logical consequence of returning the same object.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Trott]

Nit on the commit message. Maybe this is a little more clear?: http: enable call chaining with setHeader() Or if you don't like that, maybe this?: http: enable setHeader() call chaining

[nodejs-github-bot]

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

[Trott]

Will this automatically add this for https as well? If so, should we add a test? If not, should we also add it to https?

[Trott]

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/680/

[PoojaDurgad]

Will this automatically add this for https as well? If so, should we add a test? If not, should we also add it to https?

@Trott - This behavior will be added to the https and need a test I guess . I think this feature can also be added to the http2 module too!!

[Trott]

Benchmark results show a small performance hit in some cases. Probably acceptable? @nodejs/http

07:04:12 confidence improvement accuracy (*) (**) (***) 07:04:12 http/set-header.js duration=5 res='normal' benchmarker='wrk' -2.36 % ±4.51% ±6.01% ±7.84% 07:04:12 http/set-header.js duration=5 res='setHeader' benchmarker='wrk' * -3.48 % ±3.17% ±4.22% ±5.49% 07:04:12 http/set-header.js duration=5 res='setHeaderWH' benchmarker='wrk' -2.83 % ±3.86% ±5.14% ±6.71% 07:04:12 http/set_header.js n=1000000 value='Connection' -2.96 % ±4.18% ±5.62% ±7.44% 07:04:12 http/set_header.js n=1000000 value='Content-Length' -0.92 % ±2.31% ±3.08% ±4.03% 07:04:12 http/set_header.js n=1000000 value='Content-Type' 0.05 % ±1.31% ±1.74% ±2.27% 07:04:12 http/set_header.js n=1000000 value='Set-Cookie' 0.75 % ±1.57% ±2.09% ±2.72% 07:04:12 http/set_header.js n=1000000 value='Transfer-Encoding' -0.75 % ±3.16% ±4.22% ±5.54% 07:04:12 http/set_header.js n=1000000 value='Vary' * -2.19 % ±1.95% ±2.61% ±3.41% 07:04:12 http/set_header.js n=1000000 value='X-Powered-By' 0.95 % ±1.86% ±2.48% ±3.23% 07:04:12 07:04:12 Be aware that when doing many comparisons the risk of a false-positive 07:04:12 result increases. In this case there are 10 comparisons, you can thus 07:04:12 expect the following amount of false-positive results: 07:04:12 0.50 false positives, when considering a 5% risk acceptance (*, **, ***), 07:04:12 0.10 false positives, when considering a 1% risk acceptance (**, ***), 07:04:12 0.01 false positives, when considering a 0.1% risk acceptance (***) 

[PoojaDurgad]

is there anything pending on this PR? if yes let me know .

[nodejs-github-bot]

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

[Trott]

Landed in dedd061