Skip to content
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

HTTP/2 Parser implementation (#309). #1368

Merged
merged 69 commits into from
Mar 5, 2020

Conversation

aleksostapenko
Copy link
Contributor

No description provided.

Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

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

This is just an intermediate review - a lot of things unfinished and not fixed since #1338 . Also I didn't dive too deeply into the code though.

tempesta_fw/hpack.c Show resolved Hide resolved
tempesta_fw/hpack.c Show resolved Hide resolved
tempesta_fw/hpack.c Show resolved Hide resolved
tempesta_fw/cache.c Outdated Show resolved Hide resolved
tempesta_fw/http_frame.c Outdated Show resolved Hide resolved
tempesta_fw/http_parser.c Show resolved Hide resolved
tempesta_fw/hpack.c Outdated Show resolved Hide resolved
tempesta_fw/http_msg.c Outdated Show resolved Hide resolved
tempesta_fw/hpack.c Outdated Show resolved Hide resolved
tempesta_fw/http.c Outdated Show resolved Hide resolved
1. Changes in HPACK decoder to copy only Huffman-decoded and dynamically indexed headers;
2. Appropriate changes in HPACK-decoder/parser unit-tests.
Corrections as a result of HPACK decoder/encoder/parser unit-tests debugging.
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

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

I'm still in progress for the review. Besides not fixed issues from my previous PR and this PR, there are new issues with the new code which must be fixed.

tempesta_fw/http_parser.c Outdated Show resolved Hide resolved
tempesta_fw/hpack.h Outdated Show resolved Hide resolved
tempesta_fw/http_msg.c Show resolved Hide resolved
tempesta_fw/http.c Show resolved Hide resolved
tempesta_fw/http_parser.c Show resolved Hide resolved
tempesta_fw/http.c Show resolved Hide resolved
tempesta_fw/http.c Outdated Show resolved Hide resolved
tempesta_fw/http.c Outdated Show resolved Hide resolved
tempesta_fw/http_msg.c Outdated Show resolved Hide resolved
tempesta_fw/http.c Show resolved Hide resolved
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

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

I completely dislike the multiple HTTP header strings processing.

tempesta_fw/http_msg.h Outdated Show resolved Hide resolved
tempesta_fw/http_msg.c Show resolved Hide resolved
tempesta_fw/http_msg.c Outdated Show resolved Hide resolved
@krizhanovsky
Copy link
Contributor

While HTTP/2 usage is still less than HTTP/1.1, it actually means that site owners don't deploy HTTP/2 and just stay on current (old) configurations. Meantime, it seems the most modern web clients do support HTTP/2, so

  1. it's safe to make proto attribute of listen option h2 by default. Please patch etc/tempesta_fw.conf and adjust the Wiki
  2. basically there is no sense to care about HTTP/1 performance on client side (only backend side matters for compatibility with the legacy backends).

TfwStrs often go in arrays, and there is a 1-byte alignment gap between
TfwStr neighbours. `eolen` is always [0,2], 14-bits are enough for
hpack index.
# Conflicts:
#	tempesta_fw/http_parser.c
#	tempesta_fw/http_sess.c
#	tempesta_fw/t/unit/test_hpack.c
#	tempesta_fw/t/unit/test_http_sticky.c
Copy link
Contributor

@krizhanovsky krizhanovsky left a comment

Choose a reason for hiding this comment

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

More notes to make the HTTP/2 parser faster, required for #1207.

tempesta_fw/http_parser.c Outdated Show resolved Hide resolved
tempesta_fw/http_parser.c Outdated Show resolved Hide resolved
tempesta_fw/http_parser.c Outdated Show resolved Hide resolved
tempesta_fw/http_parser.c Outdated Show resolved Hide resolved
tempesta_fw/http_parser.c Outdated Show resolved Hide resolved
tempesta_fw/http_parser.c Outdated Show resolved Hide resolved
@krizhanovsky krizhanovsky mentioned this pull request Dec 7, 2019
2 tasks
vankoven and others added 13 commits January 11, 2020 02:21
Don't switch fsm state on processing frame header, update the
fsm state only after the full frame is processed. Otherwise the
fsm is triggered twice and fsm closes the connection when a new
fragment of frame is expected.
Parse name, colon, LWS, value and RWS of HTTP/1.1-response headers into
separate chunks to facilitate the name/value splitting and colon/OWS
eviction during HTTP/1.1=>HTTP/2 response transformation.
@vankoven vankoven dismissed krizhanovsky’s stale review March 5, 2020 12:05

@krizhanovsky is out for conference and the PR should be merged before the conference

Copy link
Contributor

@vankoven vankoven left a comment

Choose a reason for hiding this comment

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

Looks good to me. Any new issues found in the PR or caused by the PR will be fixed in separate PRs

@aleksostapenko aleksostapenko merged commit 8d90875 into master Mar 5, 2020
@aleksostapenko aleksostapenko deleted the ao-309-parser-transformation branch March 5, 2020 12:27
This was referenced Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants