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.

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

[v8.x] http: fix error check in Execute() #25938

Closed
wants to merge 1 commit into from
Closed

[v8.x] http: fix error check in Execute() #25938

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 5, 2019 •
edited
Loading

Refs: #24738
Fixes: #25858

CI: https://ci.nodejs.org/job/node-test-pull-request/20577/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Feb 5, 2019
@nodejs-github-bot
Copy link
Collaborator

@mscdex build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2511/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. v8.x labels Feb 5, 2019
@mscdex mscdex mentioned this pull request Feb 5, 2019
Closed
3 tasks
@addaleax addaleax changed the title http: fix error check in Execute() [v8.x] http: fix error check in Execute() Feb 9, 2019
@mscdex mscdex force-pushed the v8-http-fix-parser-execute-error-check branch from eac405d to 4ceb467 Compare February 28, 2019 16:57
@mscdex
Copy link
Contributor Author

mscdex commented Feb 28, 2019

Can this and the v6.x backport be merged yet or do we need another sign off on each first?

@BridgeAR
Copy link
Member

@mscdex the LTS @nodejs/releasers normally merge these when preparing a new release for the specific release line.

@mscdex
Copy link
Contributor Author

mscdex commented Mar 1, 2019

@BridgeAR If that's the case, why do we have version-specific staging branches? I thought the process still was: version-specific PRs are merged into the staging branch after sufficient approval (like a typical PR), then releasers pull new commits from the staging branch into the release branch when making a new release?

@mscdex
Copy link
Contributor Author

mscdex commented Mar 6, 2019

ping @nodejs/http

@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2019

@mscdex we do not distinguish semver-minor and patch releases on those branches (at least for now). If a semver-minor PR would be pulled in early, it would have to be pulled out again when releasing a patch release.

That is off course not the case here but it is AFAIK the way we dealt with these things so far. I am not part of the @nodejs/lts team so they might have further insight.

I am fine to land the PR.

@richardlau
Copy link
Member

@BridgeAR If that's the case, why do we have version-specific staging branches? I thought the process still was: version-specific PRs are merged into the staging branch after sufficient approval (like a typical PR), then releasers pull new commits from the staging branch into the release branch when making a new release?

The current process is https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#how-are-lts-branches-managed

Only the members of the @nodejs/backporters team should land commits onto LTS staging branches.

This is up for discussion in nodejs/Release#388.

@BridgeAR BridgeAR requested review from cjihrig and lpinca March 6, 2019 18:36
@lpinca
Copy link
Member

lpinca commented Mar 9, 2019

This is an important fix especially on LTS release lines.

@lpinca
Copy link
Member

lpinca commented Mar 11, 2019

@nodejs/lts @MylesBorins

@lpinca lpinca mentioned this pull request Mar 19, 2019
Closed
@BethGriggs
Copy link
Member

BethGriggs commented Mar 19, 2019 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/21668/ ✅

BethGriggs pushed a commit that referenced this pull request Mar 20, 2019
@mscdex @BethGriggs
http: fix error check in Execute()
ac9b8f7 
`http_parser_execute(..., nullptr, 0)` returns either `0` or `1`. The
expectation is that no error must be returned if it is `0`, and if
it is `1` - a `Error` object must be returned back to user.

The introduction of `llhttp` and the refactor that happened during it
accidentally removed the error-returning code. This commit reverts it
back to its original state.

Backport-PR-URL: #25938
Fix: #24585
PR-URL: #24738
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BethGriggs
Copy link
Member

Landed on v8.x-staging

@BethGriggs BethGriggs closed this Mar 20, 2019
@mscdex mscdex deleted the v8-http-fix-parser-execute-error-check branch March 20, 2019 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BethGriggs BethGriggs BethGriggs approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig Awaiting requested review from cjihrig

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http_parser Issues and PRs related to the HTTP Parser dependency or the http_parser binding. 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
@mscdex @nodejs-github-bot @BridgeAR @richardlau @lpinca @BethGriggs @jasnell