Skip to content

http: flush stored header #1695

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.

Sign up for GitHub

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

Jump to bottom
Closed
vkurchatkin wants to merge 1 commit into from
Closed

http: flush stored header #1695

vkurchatkin wants to merge 1 commit into from

Conversation

@vkurchatkin
Copy link
Contributor

@vkurchatkin vkurchatkin commented May 13, 2015

flushHeaders should work for header written with writeHead.

R=@bnoordhuis

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label May 13, 2015
mscdex
mscdex reviewed May 13, 2015
View reviewed changes
test/parallel/test-http-flush-headers.js Outdated
Copy link
Contributor

@mscdex mscdex May 13, 2015

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

@vkurchatkin vkurchatkin May 13, 2015

Choose a reason for hiding this comment

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

just an unrelated fix. bar was used as a message, not as expected value

Copy link
Contributor

@mscdex mscdex May 13, 2015

Choose a reason for hiding this comment

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

Ah I see now. Maybe it would be better to change this to assert.equal() instead, that way if the assertion should fail, it will show the two non-matching values in the message instead of just "false."

Copy link
Contributor Author

@vkurchatkin vkurchatkin May 13, 2015

Choose a reason for hiding this comment

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

yep, it is better!

brendanashworth
brendanashworth reviewed May 13, 2015
View reviewed changes
test/parallel/test-http-flush-response-headers.js Outdated
Copy link
Contributor

@brendanashworth brendanashworth May 13, 2015

Choose a reason for hiding this comment

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

sorry, style: space in (req, res) {

@vkurchatkin vkurchatkin mentioned this pull request May 14, 2015
Closed
rlidwka
rlidwka reviewed May 14, 2015
View reviewed changes
test/parallel/test-http-flush-response-headers.js Outdated
Copy link
Contributor

@rlidwka rlidwka May 14, 2015

Choose a reason for hiding this comment

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

what the second flushHeaders() call is for?

Copy link
Contributor Author

@vkurchatkin vkurchatkin May 14, 2015

Choose a reason for hiding this comment

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

Saw another test doing this. flushHeaders should be idempotent, this validates that nothing fatal happens if it is called twice

Copy link
Member

@ChALkeR ChALkeR May 17, 2015

Choose a reason for hiding this comment

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

Could you add that as a comment to the second flushHeaders call?

@bnoordhuis
Copy link
Member

bnoordhuis commented May 17, 2015

LGTM with a comment. Is the CI happy?

@Fishrock123
Copy link
Contributor

Fishrock123 commented May 27, 2015

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/718/

@Fishrock123
Copy link
Contributor

Fishrock123 commented May 27, 2015

CI is happy, @vkurchatkin are you able to update this?

@vkurchatkin
http: flush stored header
01b7a80 
`flushHeaders` should work for header written
with `writeHead`.
@vkurchatkin
Copy link
Contributor Author

vkurchatkin commented May 28, 2015

updated! @Fishrock123 can you start CI, it's a shame but I have no idea how do it

@bnoordhuis
Copy link
Member

bnoordhuis commented May 28, 2015

@vkurchatkin https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/723/

Ping me on IRC if you want instructions on how to start a job (assuming you have an account.)

vkurchatkin added a commit that referenced this pull request May 29, 2015
@vkurchatkin
http: flush stored header
2c686fd 
`flushHeaders` should work for header written
with `writeHead`.

PR-URL: #1695
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@vkurchatkin
Copy link
Contributor Author

vkurchatkin commented May 29, 2015

All is green! this is really satisfying :-). Landed in 2c686fd

@rvagg rvagg mentioned this pull request May 30, 2015
Merged
andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
@vkurchatkin @andrewdeandrade
http: flush stored header
25b8aeb 
`flushHeaders` should work for header written
with `writeHead`.

PR-URL: nodejs/node#1695
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

http Issues or PRs related to the http subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@vkurchatkin @bnoordhuis @Fishrock123 @mscdex @ChALkeR @rlidwka @brendanashworth