Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getHeader() returns undefined after using writeHead() in a certain circumstance #10354

Closed
DevAndrewGeorge opened this issue Dec 20, 2016 · 9 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem.

Comments

@DevAndrewGeorge
Copy link

DevAndrewGeorge commented Dec 20, 2016

  • Version: 6.9.1
  • Platform: Linux 4.4.0-21-generic logo ideas #37-Ubuntu SMP x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: http

In short, calling writeHead() with a ServerResponse object which has no headers already set causes getHeader() to return undefined even if the header was set in the writeHead() function call. I am unfamiliar with the project, so I'm unsure if this is defined behavior or not.

//Case 1: using setHeader, getHeader works as intended
res.setHeader("Location", "index.html");
res.getHeader("Location"); //index.html

//Case 2: using setHeader then writeHead, getHeader works as intended
res.setHeader("Set-Cookie", "hello=world");
res.writeHead(303, {Location: "index.html"});
res.getHeader("Location"); //index.html

//Case 3: using writeHead when res._headers is null, getHeader does not work as intended
res.writeHead(303, {Location: "index.html"});
res.getHeader("Location"); //undefined
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Dec 20, 2016
@Fishrock123 Fishrock123 added the confirmed-bug Issues with confirmed bugs. label Dec 20, 2016
@Fishrock123
Copy link
Contributor

Fishrock123 commented Dec 20, 2016

Oh haha, one of these "bug"s. Related to #1873

IIRC there are a number of performance reason this is difficult to fix because? Implicit headers... stuff?

Maybe when it's null we can just assign it to that object...

@DevAndrewGeorge
Copy link
Author

Seems when req._headers is null that writeHead() skips calling setHeader() a bunch of times and instead goes straight to writing the final header. Unfortunately, I don't think we could assign the passed object to req._headers because of the way keys are handled in setHeader().

@Fishrock123
Copy link
Contributor

@DevAndrewGeorge I think (but I have not checked) that it should be possible to invoke the same mechanism as setHeader() uses on each of the keys in the object passed to writeHead() if req._headers is null. It could have a performance impact though. :/

However it should be mentioned that you are still missing implicit headers and... I think you can still get poorly defined behavior when mixing writeHead() and setHeader(). Until the API is re-written (probably via HTTP/2) headers is just always going to have loopholes and strange behavior. :(

That being said I'd review a PR if you are willing to make one that covers what I've mentioned in the first paragraph. :D

@Trott
Copy link
Member

Trott commented Jul 15, 2017

@nodejs/http

@gireeshpunathil
Copy link
Member

A reasonable explanation to this behavior is optimization as opposed to a bug. This block explains the logic. setHeader is for a progressive supply of header fields (iteratively set the headers as the program advances) and writeHead is for a one shot write (all the header values available to write at a single program point) in which case you don't need to reflect on the written data and hence the storing of headers for future retrieval through getHeader is short-circuited.

The same is also explained in #13825 (comment) and a PR is being worked upon here : #19902

@gireeshpunathil
Copy link
Member

@Fishrock123 - can we have a consensus on this as to:

  • Is it a bug or no bug?
  • Is it a need fix, or a need doc?

My assertion is that use setHeader for progressive supply of headers for future modification potential (and hence can be retrieved through getHeader) and use writeHead for committing fields that are confirmed to be final (cannot be retrieved through getHeader), and a doc to this effect would be good enough.

@bnoordhuis
Copy link
Member

Not a bug, an optimization. Could maybe use a note in the docs.

If we want uniform behavior, we could set this[outHeadersKey] = null in writeHead(). There's no reason to hang on to them after the headers have been sent and it saves some memory.

@gireeshpunathil gireeshpunathil added doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. and removed confirmed-bug Issues with confirmed bugs. labels May 20, 2018
@gireeshpunathil
Copy link
Member

thanks. Added good first issue to attract contributions.

@DevAndrewGeorge
Copy link
Author

I should actually block away a few hours and actually get this done. It's been literal years.... :P

gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Jun 13, 2018
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
targos pushed a commit that referenced this issue Jun 22, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants