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

Fix for Reason-Phrase being overwritten on proxy response. #1051

Merged
merged 2 commits into from
Sep 13, 2016

Conversation

cchamberlain
Copy link
Contributor

Calling res.writeHead has the side effect of setting the Reason-Phrase back to node defaults regardless of what is in the response status line. I'm using Reason-Phrase codes to sub-route api responses and they were all being reset. This change only sets the statusMessage (Reason-Phrase) if it exists on the proxyRes and is successfully passing it through in my tests.

from docs:

image

So if just calling writeHead and not passing the optional statusMessage second argument, I believe it is getting reset to undefined and then defaulting it. I'm not sure if there is a good reason for using writeHead in general here since it seems like setting statusCode / statusMessage properties directly accomplishes the same with less side effects, could definitely be overlooking something.

What I see before change:
image

What I see after change:
image

Calling res.writeHead has the side effect of defaulting the Reason-Phrase to default ones.  I'm using Reason-Phrase codes to sub-route api responses and they were all being reset.  This change only sets the statusMessage (Reason-Phrase) if it exists on the proxyRes and is successfully passing it through in my tests.
res.writeHead(proxyRes.statusCode);
res.statusCode = proxyRes.statusCode;
if(proxyRes.statusMessage)
res.statusMessage = proxyRes.statusMessage;
Copy link
Contributor

Choose a reason for hiding this comment

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

2 space tabs

@cchamberlain
Copy link
Contributor Author

Sorry for the slow response this dropped off my radar for a bit.

@cchamberlain
Copy link
Contributor Author

@jcrugzz - Any time frame on when / if this will get merged? Not trying to rush, I just got some prod dependencies going out this week that are reliant on the status reason being passed through so I will have to fork if it does not get accepted soon.

@jcrugzz jcrugzz merged commit d8fb344 into http-party:master Sep 13, 2016
@jcrugzz
Copy link
Contributor

jcrugzz commented Sep 13, 2016

@cchamberlain sorry it took so long. Thanks!

@cchamberlain cchamberlain deleted the patch-2 branch September 13, 2016 23:08
@ArcanoxDragon
Copy link
Contributor

ArcanoxDragon commented Sep 14, 2016

This breaks plugins such as harmon that patch the writeHead function. Node supports writeHead( statusCode, statusMessage, headers ), which should be used instead of manually setting the status code and status message here.

(edit: although it looks like harmon doesn't properly account for that overload, so it would need to be patched in order for this method to work)

@cchamberlain
Copy link
Contributor Author

@briman0094 - I considered that solution as well but being unaware of harmon's dependency, went with this one since it seemed lighter weight (setting the fields directly has less of this to worry about) -

image

I can modify / open a new PR? Should we implement the headers parameter to allow upstream headers to be passed through or is this already implemented elsewhere?

@ArcanoxDragon
Copy link
Contributor

@cchamberlain I don't think we need to implement headers because the previous version of http-proxy that called writeHead didn't implement it. There's another pass in web-outgoing.js that writes the headers from proxyRes.headers into the response by calling res.setHeader(...). I had a PR that got merged in a few days ago which fixes the issue and it seems to be working fine.

@cchamberlain
Copy link
Contributor Author

@briman0094 - Awesome, guess my work here is done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants