-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Upgrade to llhttp 9.2.1 #8292
Upgrade to llhttp 9.2.1 #8292
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8292 +/- ##
==========================================
+ Coverage 97.49% 97.57% +0.07%
==========================================
Files 107 107
Lines 32987 33085 +98
Branches 3853 3868 +15
==========================================
+ Hits 32162 32282 +120
+ Misses 606 587 -19
+ Partials 219 216 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I'll give it a test now |
I think the only change is disabling obsolete line folding by default. |
It looks quite safe. Not expecting any issues (but would rather not found out later) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on production. Everything seems ok.
tests/test_http_parser.py
Outdated
assert msg.method == "GET" | ||
assert msg.path == "/test" | ||
assert msg.code == 200 | ||
assert msg.reason == "Ok" | ||
assert msg.version == (1, 1) | ||
assert msg.headers == CIMultiDict({"data": "test " + value.decode()}) | ||
assert msg.raw_headers == ((b"data", b"test " + value),) | ||
assert not msg.should_close |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would appear the C parser is setting this to True while the Python parser is setting it to False. I'm not sure of the significance of that... (This was probably already like that before this release, as I've switched the test to use the response parser).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a bug nodejs/llhttp#354
Will just comment out the check to get this merged.
Backport to 3.9: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 4d72dca on top of patchback/backports/3.9/4d72dca6869072fb073621f8b752225e216a92d9/pr-8292 Backporting merged PR #8292 into master
🤖 @patchback |
Backport to 3.10: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 4d72dca on top of patchback/backports/3.10/4d72dca6869072fb073621f8b752225e216a92d9/pr-8292 Backporting merged PR #8292 into master
🤖 @patchback |
Fixes #8291.