FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v8.x] http: fix error check in `Execute()` by mscdex · Pull Request #25938 · nodejs/node · GitHub

/ node Public

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

Closed
mscdex wants to merge 1 commit into
nodejs:v8.x-stagingfrom
mscdex:v8-http-fix-parser-execute-error-check
ConversationCommits (1)ChecksFiles changed
Closed

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

Conversation

mscdex commented Feb 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 added the http Issues or PRs related to the http subsystem. label Feb 5, 2019

nodejs-github-bot commented Feb 5, 2019

Copy link
Copy Markdown
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 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 mentioned this pull request Feb 5, 2019
Closed
3 tasks
addaleax changed the title http: fix error check in Execute() [v8.x] http: fix error check in Execute() Feb 9, 2019
mscdex force-pushed the v8-http-fix-parser-execute-error-check branch from eac405d to 4ceb467 Compare February 28, 2019 16:57

mscdex commented Feb 28, 2019

Copy link
Copy Markdown
Contributor Author

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

BridgeAR commented Feb 28, 2019

Copy link
Copy Markdown
Member

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

mscdex commented Mar 1, 2019

Copy link
Copy Markdown
Contributor Author

@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 commented Mar 6, 2019

Copy link
Copy Markdown
Contributor Author

ping @nodejs/http

BridgeAR commented Mar 6, 2019

Copy link
Copy Markdown
Member

@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 commented Mar 6, 2019

Copy link
Copy Markdown
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 requested review from cjihrig and lpinca March 6, 2019 18:36

lpinca commented Mar 9, 2019

Copy link
Copy Markdown
Member

This is an important fix especially on LTS release lines.

lpinca commented Mar 11, 2019

Copy link
Copy Markdown
Member

@nodejs/lts @MylesBorins

lpinca mentioned this pull request Mar 19, 2019
Closed

BethGriggs commented Mar 19, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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

BethGriggs pushed a commit that referenced this pull request Mar 20, 2019
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 commented Mar 20, 2019

Copy link
Copy Markdown
Member

Landed on v8.x-staging

BethGriggs closed this Mar 20, 2019
mscdex deleted the v8-http-fix-parser-execute-error-check branch March 20, 2019 20:44
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
lpinca lpinca approved these changes
BethGriggs BethGriggs approved these changes
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

Back | FazBrowse Home | New Git URL