Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

better output handler response header check #1264

Merged
merged 1 commit into from
Oct 17, 2013

Conversation

jarnoux
Copy link
Contributor

@jarnoux jarnoux commented Oct 16, 2013

The output handler was maintaining its own guard to prevent writing headers when they are already sent. This is not an efficient solution as the headers can potentially have been written by virtually anyone that has access to the response. This was causing our app to crash and stop when we had two errors.

@caridy
Copy link
Contributor

caridy commented Oct 17, 2013

LGTM, although the functional tests are failing, but might not be related to this PR. /cc @lzhan can you take care of this?

@lzhan
Copy link
Contributor

lzhan commented Oct 17, 2013

@caridy LGTM too. Failed functional tests are due to existing YQL/flickr issue. I have run this pr with the branch with fixed functional app/tests, everything is fine.
@jarnoux would you please provide a unit or functional test for this pr?

@caridy
Copy link
Contributor

caridy commented Oct 17, 2013

@lzhan, a unit test for this is going to be difficult because @jarnoux removed the piece of logic that we controlled in favor of something controlled by express directly. What we might need to do is to review if we were doing any unit test around that manual verification, and remove them.

In the other hand, if we want to really test this, we might want to use supertest which allow us to fake an express request, but I think for now we are good.

@lzhan
Copy link
Contributor

lzhan commented Oct 17, 2013

@caridy Got it. Thanks. I will update HISTORY file and merge this pr.

lzhan pushed a commit that referenced this pull request Oct 17, 2013
better output handler response header check
@lzhan lzhan merged commit efa639f into YahooArchive:develop Oct 17, 2013
lzhan pushed a commit that referenced this pull request Oct 18, 2013
update history file, add entry for pr #1264
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants