doc: explain HTTP writeHead()'s fast path behavior by gireeshpunathil · Pull Request #21289 · nodejs/node

gireeshpunathil · 2018-06-12T14:50:22Z

calling writeHead() into a Response object that has no headers already
set causes getHeader() to return undefined, even if the header was set
in the writeHead() function call. Explain this behavior as an
optimiation as opposed to a bug.

Fixes: #10354

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 · 2018-06-12T14:50:23Z

@gireeshpunathil sadly an error occured when I tried to trigger a build :(

vsemozhetbyt · 2018-06-12T15:33:59Z

Is this intended to be in the request.setHeader() section, not in the response.setHeader() section?

vsemozhetbyt · 2018-06-12T15:22:49Z

doc/api/http.md

This long line seems to break linting.

lpinca

LGTM with @vsemozhetbyt's nit addressed.

gireeshpunathil · 2018-06-13T07:45:55Z

that is interesting. I ran lint and lint-ci targets locally, and it does not throw this error.

$ make lint
make[1]: Entering directory `/home/node'
Running JS linter...
make[1]: Leaving directory `/home/node'
make[1]: Entering directory `/home/node'
Running C++ linter...
Total errors found: 0
make[1]: Leaving directory `/home/node'
make[1]: Entering directory `/home/node'
Running C++ linter on addon docs...
Total errors found: 0
make[1]: Leaving directory `/home/node'
make[1]: Entering directory `/home/node'
The markdown linter is not installed.
To install (requires internet access) run: make lint-md-build
make[1]: Leaving directory `/home/node'

$ make lint-ci

Running JS linter...
The markdown linter is not installed.
To install (requires internet access) run: make lint-md-build
Running C++ linter on addon docs...
Total errors found: 0

any idea why?

Nevertheless, made the changes and pushed, please have a look.

gireeshpunathil · 2018-06-13T07:48:09Z

@vsemozhetbyt

Is this intended to be in the request.setHeader() section, not in the response.setHeader() section?

No, it is meant at response.setHeader() and response.writeHead() sites itself, because those are the APIs whose usage context and behavior were under question, and hence I clarified it under both the APIs. Please let me know if there is an issue with that.

richardlau · 2018-06-13T08:05:05Z

@gireeshpunathil The lint and lint-ci targets will skip linting docs unless dependencies are installed. As the output says, run: make lint-md-build and then try again.

vsemozhetbyt · 2018-06-13T15:11:28Z

@gireeshpunathil Currently, the first fragment is placed in the request.setHeader() section)

apapirovski

This needs to be fixed up before it can land

apapirovski · 2018-06-13T15:11:06Z

doc/api/http.md

This is inserted in an incorrect area or has the wrong links.

fixed, ptal.

vsemozhetbyt · 2018-06-13T15:20:31Z

doc/api/http.md

are desired -> is desired?

vsemozhetbyt · 2018-06-13T15:20:41Z

doc/api/http.md

are desired -> is desired?

fixed, thanks.

calling writeHead() into a Response object that has no headers already set causes getHeader() to return undefined, even if the header was set in the writeHead() function call. Explain this behavior as an optimiation as opposed to a bug. Fixes: nodejs#10354

TimothyGu · 2018-06-13T16:11:32Z

doc/api/http.md

+called, it will directly write the supplied header values onto the network
+channel without caching internally, and the [`response.getHeader()`][] on the
+header will not yield the expected result. If progressive population of headers
+is desired with potential future retrieval and modification, use


Is modification still possible after writeHead? Sounds to me like only retrieval is possible. Ditto below.

@TimothyGu - yes, that is right: headers written through writeHead are not modifiable, and attempt to do so results in error Cannot set headers after they are sent to the client. Hence the suggestion to use setHeader() API for such cases. If the text is not conveying that let me know where and how can I fix it, thanks.

Yeah, you wrote

If progressive population of headers is desired with potential future retrieval and modification, use …

It seemed to me that the bolded bit should perhaps be removed.

sorry, missed this update. Sorry, I am missing your point:
#cat 10354.js

const h = require('http') h.createServer((q, r) => { r.setHeader('foo', 'bar') r.setHeader('foo', 'new') r.end() }).listen(12000, () => { const c = h.get('http://localhost:12000', (m) => { console.log(m.headers) }) })

#node 10354.js

{ foo: 'new', date: 'Thu, 21 Jun 2018 14:42:33 GMT', connection: 'close', 'content-length': '0' }

setHeader() is indeed used for modification, and hence the text. Can you please clarify your point?

Never mind, I see what this is trying to say now.

vsemozhetbyt

Doc format LGTM)

gireeshpunathil · 2018-06-22T04:18:44Z

In the light of recent changes to the build process, what is the CI recommendation for doc changes? (sorry, I was not keeping track of the conversations) full CI (manual trigger) / lite CI (manual trigger) / do nothing - as the auto-CI said all checks have passed.

gireeshpunathil · 2018-06-22T09:44:29Z

answering my own question, I guess it is node-test-pull-request-lite-pipeline for docs:
https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/119/

calling writeHead() into a Response object that has no headers already set causes getHeader() to return undefined, even if the header was set in the writeHead() function call. Explain this behavior as an optimiation as opposed to a bug. Fixes: #10354 PR-URL: #21289 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

gireeshpunathil · 2018-06-22T10:04:07Z

landed as cbe307d

calling writeHead() into a Response object that has no headers already set causes getHeader() to return undefined, even if the header was set in the writeHead() function call. Explain this behavior as an optimiation as opposed to a bug. Fixes: #10354 PR-URL: #21289 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

nodejs-github-bot added doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem. labels Jun 12, 2018

vsemozhetbyt reviewed Jun 12, 2018

View reviewed changes

doc/api/http.md Outdated

Copy link

Contributor

vsemozhetbyt Jun 12, 2018

Choose a reason for hiding this comment

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

This long line seems to break linting.

lpinca approved these changes Jun 12, 2018

View reviewed changes

gireeshpunathil force-pushed the header-persistence branch from 2526761 to f429d40 Compare June 13, 2018 07:44

apapirovski suggested changes Jun 13, 2018

View reviewed changes

vsemozhetbyt reviewed Jun 13, 2018

View reviewed changes

gireeshpunathil force-pushed the header-persistence branch from 749e155 to c86df32 Compare June 13, 2018 15:53

TimothyGu reviewed Jun 13, 2018

View reviewed changes

vsemozhetbyt approved these changes Jun 13, 2018

View reviewed changes

apapirovski approved these changes Jun 14, 2018

View reviewed changes

trivikr approved these changes Jun 14, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 14, 2018

TimothyGu approved these changes Jun 21, 2018

View reviewed changes

jasnell approved these changes Jun 21, 2018

View reviewed changes

gireeshpunathil closed this Jun 22, 2018

gireeshpunathil mentioned this pull request Jun 25, 2018

Duplicated response.writeHead calling is not checked in specific case #20932

Closed

targos mentioned this pull request Jul 3, 2018

v10.6.0 proposal #21629

Merged

Uh oh!

Conversation

gireeshpunathil commented Jun 12, 2018

Checklist

Uh oh!

nodejs-github-bot commented Jun 12, 2018

Uh oh!

vsemozhetbyt commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil commented Jun 13, 2018

Uh oh!

gireeshpunathil commented Jun 13, 2018

Uh oh!

richardlau commented Jun 13, 2018

Uh oh!

vsemozhetbyt commented Jun 13, 2018

Uh oh!

apapirovski left a comment • 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!

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!

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!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil commented Jun 22, 2018

Uh oh!

gireeshpunathil commented Jun 22, 2018

Uh oh!

gireeshpunathil commented Jun 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

apapirovski left a comment •

edited

Loading