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.

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

fix multiple begin message cbs when response starts with CR/LF #432

Merged
merged 2 commits into from Jul 19, 2018

Conversation

Projects
None yet
4 participants
@mattklein123
Copy link
Contributor

mattklein123 commented Jun 5, 2018

Signed-off-by: Matt Klein mklein@lyft.com

fix multiple begin message cbs when response starts with CR/LF
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123

This comment has been minimized.

Copy link
Contributor Author

mattklein123 commented Jun 5, 2018

It's not completely clear to me that the RFC allows CR/LF before response start, but this mirrors the logic in the other start states (request and request or response) and I think it's a clear bug to invoke the message begin callback multiple times. Please let me know if you think a different fix would be better.

Lukasa added a commit to Lukasa/swift-nio that referenced this pull request Jun 5, 2018

Update http_parser
Motivation:

Our copy of http_parser has become almost a year old now, and there are
both some bugfixes that improve correctness and some performance
improvements in http_parser that we should pick up.

Modifications:

- Updated http_parser
- Added tests to confirm we don't suffer from
    nodejs/http-parser#432
- Added tests that we didn't get broken by SOURCE verb addition.

Result:

Shiny new http_parser!

@Lukasa Lukasa referenced this pull request Jun 5, 2018

Merged

Update http_parser #471

normanmaurer added a commit to apple/swift-nio that referenced this pull request Jun 10, 2018

Update http_parser (#471)
Motivation:

Our copy of http_parser has become almost a year old now, and there are
both some bugfixes that improve correctness and some performance
improvements in http_parser that we should pick up.

Modifications:

- Updated http_parser
- Added tests to confirm we don't suffer from
    nodejs/http-parser#432
- Added tests that we didn't get broken by SOURCE verb addition.

Result:

Shiny new http_parser!
@mattklein123

This comment has been minimized.

Copy link
Contributor Author

mattklein123 commented Jun 12, 2018

@bnoordhuis any chance of getting this looked at this week? Thank you in advance.

@bnoordhuis
Copy link
Member

bnoordhuis left a comment

LGTM with a request.

It's not completely clear to me that the RFC allows CR/LF before response start

I don't think it does, strictly speaking, but you're right that we're already allowing that for other start states so it'd be inconsistent to disallow it here.

case CR:
case LF:
break;

This comment has been minimized.

@bnoordhuis

bnoordhuis Jun 15, 2018

Member

Now that the switch has only one case (well, two if you count the default) can you turn it into an if statement?

comments
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123

This comment has been minimized.

Copy link
Contributor Author

mattklein123 commented Jun 15, 2018

@bnoordhuis thanks for the review. Updated.

@bnoordhuis
Copy link
Member

bnoordhuis left a comment

I'll leave this open for a few more days in case other maintainers want to chime in.

@mattklein123

This comment has been minimized.

Copy link
Contributor Author

mattklein123 commented Jun 20, 2018

@bnoordhuis friendly ping. Can we merge?

@mattklein123

This comment has been minimized.

Copy link
Contributor Author

mattklein123 commented Jun 27, 2018

@bnoordhuis friendly ping again. Can we please merge? Thank you!

@htuch

This comment has been minimized.

Copy link

htuch commented Jul 9, 2018

@bnoordhuis any chance we can get a merge here?

@maclover7
Copy link
Member

maclover7 left a comment

esttorhe added a commit to esttorhe/swift-nio that referenced this pull request Jul 11, 2018

Update http_parser (apple#471)
Motivation:

Our copy of http_parser has become almost a year old now, and there are
both some bugfixes that improve correctness and some performance
improvements in http_parser that we should pick up.

Modifications:

- Updated http_parser
- Added tests to confirm we don't suffer from
    nodejs/http-parser#432
- Added tests that we didn't get broken by SOURCE verb addition.

Result:

Shiny new http_parser!
@mattklein123

This comment has been minimized.

Copy link
Contributor Author

mattklein123 commented Jul 17, 2018

@bnoordhuis @maclover7 do we expect this PR to merge any time soon? If not I will fork http-parser into the envoy proxy org. Thank you!

@maclover7 maclover7 merged commit 25de6ed into nodejs:master Jul 19, 2018

1 check passed

test tests passed
Details
@maclover7

This comment has been minimized.

Copy link
Member

maclover7 commented Jul 19, 2018

Landed in 25de6ed

@mattklein123

This comment has been minimized.

Copy link
Contributor Author

mattklein123 commented Jul 19, 2018

Thank you for merging!

@mattklein123 mattklein123 deleted the mattklein123:fix_response_start branch Jul 19, 2018

ploxiln added a commit to ploxiln/http-parser that referenced this pull request Jan 4, 2019

Fix multiple begin message cbs when response starts with CR/LF
PR-URL: nodejs#432
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment