Skip to content

http: fix error check in Execute()#25863

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

http: fix error check in Execute()#25863
mscdex wants to merge 1 commit intonodejs:v10.x-stagingfrom
mscdex:v10-http-fix-parser-execute-error-check

Conversation

@mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 1, 2019 •
edited
Loading

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

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

nodejs-github-bot commented Feb 1, 2019

@mscdex build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2477/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. v10.x labels Feb 1, 2019
@mscdex
Copy link
Contributor Author

mscdex commented Feb 1, 2019

/cc @indutny

@indutny
Copy link
Member

indutny commented Feb 1, 2019

@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

indutny commented Feb 1, 2019

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

indutny
indutny approved these changes Feb 1, 2019
View reviewed changes
Copy link
Member

@indutny indutny 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
Contributor Author

mscdex commented Feb 5, 2019

/cc @nodejs/collaborators

mcollina
mcollina approved these changes Feb 5, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina 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
Contributor Author

mscdex commented Feb 5, 2019

@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
Contributor Author

mscdex commented Feb 5, 2019

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

BethGriggs pushed a commit that referenced this pull request Feb 5, 2019
@mscdex @BethGriggs
http: fix error check in Execute()
741c5ef 
Refs: #24738
Fixes: #25858

PR-URL: #25863
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BethGriggs
Copy link
Member

BethGriggs commented Feb 5, 2019

Landed on v10.x-staging in 741c5ef

@BethGriggs BethGriggs closed this Feb 5, 2019
@BethGriggs BethGriggs mentioned this pull request Feb 12, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@mscdex @rvagg
http: fix error check in Execute()
b9204db 
Refs: #24738
Fixes: #25858

PR-URL: #25863
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs added a commit that referenced this pull request Mar 4, 2019
@BethGriggs
2019-03-05, Version 10.15.3 'Dubnium' (LTS)
1a96d83 
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
@BethGriggs
2019-03-05, Version 10.15.3 'Dubnium' (LTS)
a52c1ea 
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
@snyk-bot snyk-bot mentioned this pull request Aug 11, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

+1 more reviewer

@indutny indutny indutny approved these changes

Reviewers whose approvals may not affect merge requirements

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.

5 participants

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