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

Already on GitHub? Sign in to your account

header event (patch.js) seems not able to take care writeHead(status, headers) #683

Closed
yukinying opened this Issue Nov 2, 2012 · 7 comments

Comments

Projects
None yet
4 participants

I am debugging a scenario when using connect with http-proxy. And I realized that I could not remove headers. With some investigation, I believe there was an assumption that code are calling response.writeHead(status) only.

I am not able to setup more tests to justify the case, but it looks like, from the code of patch.js:

  res.writeHead = function(){
    if (!this._emittedHeader) this.emit('header');
    this._emittedHeader = true;
    return writeHead.apply(this, arguments);
  };

If arguments contain headers, then one could not remove headers in the event "header" since writeHead.apply already got that in the arguments. Furthermore, header event couldn't be possibly getting the headers inside the arguments as well.

I may have overlooked some parts since I could not perform more troubleshooting and debugging on the connect core code. Perhaps I was mistaken and I wish this tracker may be useful if this is a real issue.

Thanks,
Albert

Member

tj commented Nov 2, 2012

yeah, node internally is a mess right now you can't even really reliably hook into when headers are set, I strongly recommend never using .writeHead() it is a mistake of an api

Perhaps connect should patch the writeHead in a way that it will revoke the mechanism to "setHeaders", set StatusCode, set Reasons, etc emit "header" event and then just call writeHead.apply(this) ? In that sense it will eliminate potential misuse.

The fact is that there are a lot of middlewares that are using writeHead and chaining them in connect is generating unexpected behavior now.

Do you think this makes sense? If so I may create a patch and send a pull request.

Thanks!

Member

tj commented Nov 3, 2012

yeah that might be a nicer way to fix it for now, I dont think writeHead is going anywhere for legacy reasons unfortunately

It seems someone has proposed for the same 7 months ago - #524 .

Contributor

jonathanong commented Oct 22, 2013

is this still an issue? are people actually using writeHead?

Contributor

dougwilson commented Oct 22, 2013

It's probably still an issue and sadly I see people using writeHead all the time, due to copy-paste coding :(

Contributor

dougwilson commented Oct 22, 2013

Nevermind, I believe this was fixed in commit b6d76b7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment