Buffer HPACK parsing until the end of a header boundary by ctiller · Pull Request #26700 · grpc/grpc

ctiller · 2021-07-15T22:36:24Z

HTTP2 headers are sent in (potentially) many frames, but all must be
sent sequentially with no traffic intervening.

This was not clear when I wrote the HPACK parser, and still indeed quite
contentious on the HTTP2 mailing lists.

Now that matter is well settled (years ago!) take advantage of the fact
by delaying parsing until all bytes are available.

A future change will leverage this to avoid having to store and verify
partial parse state, completely eliminating indirect calls within the
parser.

HTTP2 headers are sent in (potentially) many frames, but all must be sent sequentially with no traffic intervening. This was not clear when I wrote the HPACK parser, and still indeed quite contentious on the HTTP2 mailing lists. Now that matter is well settled (years ago!) take advantage of the fact by delaying parsing until all bytes are available. A future change will leverage this to avoid having to store and verify partial parse state, completely eliminating indirect calls within the parser.

This reverts commit 258d712.

ctiller · 2021-07-19T04:23:28Z

Test changes are usually ill advised to get a change through... so here are some notes on changes I made:

duplicate_headers wants to verify that we do not segfault on illegally duplicated headers, so verifying successful completion on some ops was over specifying the test.
large_metadata had previously sent a partial header fragment; under the new scheme such a partial header will not be parsed, and consequently not trigger the condition in question

ctiller · 2021-07-19T23:03:43Z

@nicolasnoble @jtattermusch can you help me with why the tests don't seem to be running here?

ctiller · 2021-07-20T22:19:10Z

Ok, looks like the tests are running again, fixed the last breakage I know of.

ctiller · 2021-07-20T22:22:12Z

test/cpp/microbenchmarks/bm_chttp2_hpack.cc

+    return {MakeSlice({0x40, 0x0b, 0x67, 0x72, 0x70, 0x63, 0x2d, 0x73,
+                       0x74, 0x61, 0x74, 0x75, 0x73, 0x01, 0x30, 0x40,
+                       0x0c, 0x67, 0x72, 0x70, 0x63, 0x2d, 0x6d, 0x65,
+                       0x73, 0x73, 0x61, 0x67, 0x65, 0x00})};


Side note: this has been broken for years and not doing a correct benchmark: grpc_slice_from_static_string totally stops at the trailing null byte, corrupting the stream.

ctiller · 2021-07-21T14:12:46Z

Pinging on this for review

yashykt · 2021-07-26T17:13:28Z

test/core/bad_client/tests/large_metadata.cc

  "\x00\x00\x00\x04\x01\x00\x00\x00\x00"                  \
  "\x00"                                                  \
-  "5{\x01\x05\x00\x00\x00\x01"                            \
+  "\x1aM\x01\x05\x00\x00\x00\x01"                         \


can you explain this change please?

The frame length was incorrect, meaning that this test would simply buffer and wait forever due to an incomplete request.

ctiller · 2021-07-28T22:36:35Z

Known issues: #24375

…)" This reverts commit 8bab3e4.

…)" (#26825) This reverts commit 8bab3e4.

* Buffer HPACK parsing until the end of a header boundary HTTP2 headers are sent in (potentially) many frames, but all must be sent sequentially with no traffic intervening. This was not clear when I wrote the HPACK parser, and still indeed quite contentious on the HTTP2 mailing lists. Now that matter is well settled (years ago!) take advantage of the fact by delaying parsing until all bytes are available. A future change will leverage this to avoid having to store and verify partial parse state, completely eliminating indirect calls within the parser. * maybe fixes * xx * fix boundary detection * clang-format * Revert "xx" This reverts commit 258d712. * fix tests * add missed check * fixes * fix * update tests * fix benchmark * properly unref * optimize final slice refcounting * cleanup bm_chttp2_hpack * start * new parser progress * refinement * get it compiling * bug-fix * build files * clang-tidy * fixes * fixes * fixes * fix-leaks * clang-tidy * comments * fix merge error * Revert "Buffer HPACK parsing until the end of a header boundary (#26700)" This reverts commit 8bab3e4. * streaming hpack parser start * streaming parser * clang-format * Rework HPackTable into C++ * clang-tidy * fix merge * actually set the size of the entries array * better

* Buffer HPACK parsing until the end of a header boundary HTTP2 headers are sent in (potentially) many frames, but all must be sent sequentially with no traffic intervening. This was not clear when I wrote the HPACK parser, and still indeed quite contentious on the HTTP2 mailing lists. Now that matter is well settled (years ago!) take advantage of the fact by delaying parsing until all bytes are available. A future change will leverage this to avoid having to store and verify partial parse state, completely eliminating indirect calls within the parser. * maybe fixes * xx * fix boundary detection * clang-format * Revert "xx" This reverts commit 258d712. * fix tests * add missed check * fixes * fix * update tests * fix benchmark * properly unref * optimize final slice refcounting * cleanup bm_chttp2_hpack * start * new parser progress * refinement * get it compiling * bug-fix * build files * clang-tidy * fixes * fixes * fixes * fix-leaks * clang-tidy * comments * fix merge error * Revert "Buffer HPACK parsing until the end of a header boundary (grpc#26700)" This reverts commit 8bab3e4. * streaming hpack parser start * streaming parser * clang-format * Rework HPackTable into C++ * clang-tidy * fix merge * actually set the size of the entries array * better

…#26700)" (grpc#26825) This reverts commit 8bab3e4.

* Buffer HPACK parsing until the end of a header boundary HTTP2 headers are sent in (potentially) many frames, but all must be sent sequentially with no traffic intervening. This was not clear when I wrote the HPACK parser, and still indeed quite contentious on the HTTP2 mailing lists. Now that matter is well settled (years ago!) take advantage of the fact by delaying parsing until all bytes are available. A future change will leverage this to avoid having to store and verify partial parse state, completely eliminating indirect calls within the parser. * maybe fixes * xx * fix boundary detection * clang-format * Revert "xx" This reverts commit 258d712. * fix tests * add missed check * fixes * fix * update tests * fix benchmark * properly unref * optimize final slice refcounting * cleanup bm_chttp2_hpack * start * new parser progress * refinement * get it compiling * bug-fix * build files * clang-tidy * fixes * fixes * fixes * fix-leaks * clang-tidy * comments * fix merge error * Revert "Buffer HPACK parsing until the end of a header boundary (grpc#26700)" This reverts commit 8bab3e4. * streaming hpack parser start * streaming parser * clang-format * Rework HPackTable into C++ * clang-tidy * fix merge * actually set the size of the entries array * better

HTTP2 headers are sent in (potentially) many frames, but all must be sent sequentially with no traffic intervening. This was not clear when I wrote the HPACK parser, and still indeed quite contentious on the HTTP2 mailing lists. Now that matter is well settled (years ago!) take advantage of the fact by delaying parsing until all bytes are available. A future change will leverage this to avoid having to store and verify partial parse state, completely eliminating indirect calls within the parser.

…#26700)" (grpc#26825) This reverts commit 8bab3e4.

* Buffer HPACK parsing until the end of a header boundary HTTP2 headers are sent in (potentially) many frames, but all must be sent sequentially with no traffic intervening. This was not clear when I wrote the HPACK parser, and still indeed quite contentious on the HTTP2 mailing lists. Now that matter is well settled (years ago!) take advantage of the fact by delaying parsing until all bytes are available. A future change will leverage this to avoid having to store and verify partial parse state, completely eliminating indirect calls within the parser. * maybe fixes * xx * fix boundary detection * clang-format * Revert "xx" This reverts commit 258d712. * fix tests * add missed check * fixes * fix * update tests * fix benchmark * properly unref * optimize final slice refcounting * cleanup bm_chttp2_hpack * start * new parser progress * refinement * get it compiling * bug-fix * build files * clang-tidy * fixes * fixes * fixes * fix-leaks * clang-tidy * comments * fix merge error * Revert "Buffer HPACK parsing until the end of a header boundary (grpc#26700)" This reverts commit 8bab3e4. * streaming hpack parser start * streaming parser * clang-format * Rework HPackTable into C++ * clang-tidy * fix merge * actually set the size of the entries array * better

ctiller requested a review from yashykt July 15, 2021 22:36

ctiller added the release notes: no Indicates if PR should not be in release notes label Jul 15, 2021

markdroth assigned yashykt Jul 16, 2021

ctiller added 9 commits July 16, 2021 11:37

Merge branch 'master' of github.com:grpc/grpc into hpack-buffering

277002c

maybe fixes

2e698ad

xx

258d712

fix boundary detection

3c45333

clang-format

dda442f

Revert "xx"

4afbf3d

This reverts commit 258d712.

fix tests

21a4f69

add missed check

50079ba

fixes

b5e24a6

ctiller added 2 commits July 19, 2021 08:20

fix

aaf1e7f

update tests

769ab8b

ctiller added the kokoro:run label Jul 19, 2021

Merge github.com:grpc/grpc into hpack-buffering

507f5f8

grpc-kokoro removed the kokoro:run label Jul 20, 2021

fix benchmark

ba1141c

ctiller commented Jul 20, 2021

View reviewed changes

ctiller and others added 3 commits July 20, 2021 22:31

properly unref

af789d4

optimize final slice refcounting

958fd1e

cleanup bm_chttp2_hpack

47b6d54

ctiller mentioned this pull request Jul 26, 2021

Simplify HPACK parser #26787

Closed

yashykt approved these changes Jul 26, 2021

View reviewed changes

ctiller merged commit 8bab3e4 into grpc:master Jul 28, 2021

ctiller added a commit that referenced this pull request Jul 29, 2021

Revert "Buffer HPACK parsing until the end of a header boundary (#26700…

2a1d590

…)" This reverts commit 8bab3e4.

ctiller mentioned this pull request Jul 29, 2021

Revert "Buffer HPACK parsing until the end of a header boundary" #26825

Merged

ctiller added a commit that referenced this pull request Jul 29, 2021

Revert "Buffer HPACK parsing until the end of a header boundary (#26700…

613b90b

…)" (#26825) This reverts commit 8bab3e4.

Vignesh2208 pushed a commit to Vignesh2208/grpc that referenced this pull request Aug 20, 2021

Revert "Buffer HPACK parsing until the end of a header boundary (grpc…

3e68a14

…#26700)" (grpc#26825) This reverts commit 8bab3e4.

lidizheng pushed a commit to lidizheng/grpc that referenced this pull request Sep 23, 2021

Revert "Buffer HPACK parsing until the end of a header boundary (grpc…

91efcfb

…#26700)" (grpc#26825) This reverts commit 8bab3e4.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buffer HPACK parsing until the end of a header boundary#26700

Buffer HPACK parsing until the end of a header boundary#26700
ctiller merged 17 commits intogrpc:masterfrom
ctiller:hpack-buffering

ctiller commented Jul 15, 2021

Uh oh!

ctiller commented Jul 19, 2021

Uh oh!

ctiller commented Jul 19, 2021

Uh oh!

ctiller commented Jul 20, 2021

Uh oh!

ctiller Jul 20, 2021

Uh oh!

ctiller commented Jul 21, 2021

Uh oh!

yashykt Jul 26, 2021

Uh oh!

ctiller Jul 27, 2021

Uh oh!

ctiller commented Jul 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ctiller commented Jul 15, 2021

Uh oh!

ctiller commented Jul 19, 2021

Uh oh!

ctiller commented Jul 19, 2021

Uh oh!

ctiller commented Jul 20, 2021

Uh oh!

ctiller Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

ctiller commented Jul 21, 2021

Uh oh!

yashykt Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

ctiller Jul 27, 2021

Choose a reason for hiding this comment

Uh oh!

ctiller commented Jul 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants