Skip to content

Conversation

@mscdex
Copy link
Contributor

@mscdexmscdex commented Feb 1, 2019

Refs: #24738
Fixes: #25858

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

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

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

@nodejs-github-botnodejs-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. v10.x labels Feb 1, 2019
@mscdex
Copy link
ContributorAuthor

/cc @indutny

@indutny
Copy link
Member

@mscdex There're differences in return values for llhttp and http_parser. I don't think that this PR as it is will work well for llhttp.

cc @addaleax

@indutny
Copy link
Member

Wait, I just realized that this is for 10.x branch. nvm.

Copy link
Member

@indutnyindutny left a comment

Choose a reason for hiding this comment

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

LGTM

@mscdex
Copy link
ContributorAuthor

/cc @nodejs/collaborators

Copy link
Member

@mcollinamcollina left a comment

Choose a reason for hiding this comment

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

LGTM. Does this apply only on Node 10?

@mscdex
Copy link
ContributorAuthor

@mcollina It has to be done for v6.x and v8.x as well if that's what you mean, I can do that yet. master already has the fix though.

@mscdex
Copy link
ContributorAuthor

PR for v6.x: #25939
PR for v8.x: #25938

BethGriggs pushed a commit that referenced this pull request Feb 5, 2019
Refs: #24738Fixes: #25858 PR-URL: #25863 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@BethGriggs
Copy link
Member

Landed on v10.x-staging in 741c5ef

@BethGriggsBethGriggs mentioned this pull request Feb 12, 2019
rvagg pushed a commit that referenced this pull request Feb 28, 2019
Refs: #24738Fixes: #25858 PR-URL: #25863 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
BethGriggs added a commit that referenced this pull request Mar 4, 2019
Notable Changes * **doc** * add antsmartian to collaborators (Anto Aravinth) [#24655](#24655) * **http** * fix error check in Execute() (Brian White) [#25863](#25863) * **stream** * fix end-of-stream for HTTP/2 (Anna Henningsen) [#24926](#24926) PR-URL: #26063
BethGriggs added a commit that referenced this pull request Mar 5, 2019
Notable Changes * **doc** * add antsmartian to collaborators (Anto Aravinth) [#24655](#24655) * **http** * fix error check in Execute() (Brian White) [#25863](#25863) * **stream** * fix end-of-stream for HTTP/2 (Anna Henningsen) [#24926](#24926) PR-URL: #26063
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.http_parserIssues and PRs related to the HTTP Parser dependency or the http_parser binding.httpIssues or PRs related to the http subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@mscdex@nodejs-github-bot@indutny@BethGriggs@mcollina