Skip to content

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

Closed
mscdex wants to merge 1 commit intonodejs:v8.x-stagingfrom
mscdex:v8-http-fix-parser-execute-error-check
Closed

[v8.x] http: fix error check in Execute()#25938
mscdex wants to merge 1 commit intonodejs:v8.x-stagingfrom
mscdex:v8-http-fix-parser-execute-error-check

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

nodejs-github-bot commented Feb 5, 2019

@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

BridgeAR commented Feb 28, 2019

@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

richardlau commented Mar 6, 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?

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

BethGriggs commented Mar 20, 2019

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

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@BethGriggs BethGriggs BethGriggs 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