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

Refactor H2 #286

Merged
merged 46 commits into from
Feb 9, 2024
Merged

Refactor H2 #286

merged 46 commits into from
Feb 9, 2024

Conversation

mtrudel
Copy link
Owner

@mtrudel mtrudel commented Jan 6, 2024

This is a long-term working branch to handle a fairly major refactor of the H2 code. Goals:

  • Move all stream-specific logic into the stream process itself. All stream state transitions, error handling, and request validation should take place on the stream process so as to block the connection process as little as possible

  • Reduce the scope of the connection process to encompass only connection-level concerns (pings, continuation frame validation, etc), sending and receiving bare frames the stream processes, and managing a list of the processes that are handling active streams on the connection. It essentially becomes a switchboard with the client on the one side and the stream processes on the other

  • Split the stream into transport concerns and HTTP semantic concerns. Put the transport concerns (roughly, RFC9113 concerns) into Bandit.HTTP2.Stream, and HTTP semantic concerns (roughly, RFC9110 concerns) into Bandit.HTTP2.Adapter. This provides a foundation for two workups that will come in future PRs:

    • Refactor HTTP/1 #297 will make this same distinction between transport and semantic concerns for the HTTP/1 stack (ie: splitting out RFC9112 concerns from Bandit.HTTP1.Adapter). At this point, the two adapters should be identical (or at least close to it), and we can factor them together into a single implementation. This will prevent having to implement all HTTP semantic concerns separately in both stacks.
    • A future PR will refactor Bandit's WebSocket implementation to use these lower level transports, which will enable the use of WebSockets on HTTP/2 (at least once a connection is established). The effort required for Add support for HTTP/2 originated Websockets (RFC 8441) #91 will then reduce to that required to accomplish the HTTP/2 specific upgrade mechanism.

@mtrudel
Copy link
Owner Author

mtrudel commented Jan 26, 2024

This is ready for review; short of huge issues raised on review, I'm planning to merge this on or near 31 Jan so I can get moving on subsequent workups.

This is a big workup, and is likely best considered as a complete rewrite rather than piecemeal changes. The ONLY scope here is HTTP/2; nothing else has been changed here. A provisional CHANGELOG:

  • Improved process model is MUCH easier to understand and yields about a 10% performance boost to HTTP/2 requests
  • BREAKING CHANGE The HTTP/2 header size options have been deprecated, and have been replaced with a single max_header_block_size (the setting defaults to 50k bytes). This is in keeping with the intent of these checks to be as early of a protection as possible against oversize requests, and changing to this definition allows us to do that before we even attempt to decompress them.
  • We no longer do the right thing if processes that are linked to an HTTP/2 stream process terminate unexpectedly. No logs are produced, and the request's telemetry span will also remain unclosed. Note that this has always been unsupported behaviour so I'm not considering it as a breaking change (though it is unfortunate)
  • We now strictly validate the that all Plug.Conn function calls for an HTTP/2 connection be from the stream process. Again this has ~always been undefined behaviour, see Plug.Conn.read_body fails inside separate process #215 and others
  • Reading the body of an HTTP/2 request after it has already been read will return {:ok, "'} instead of raising a Bandit.BodyAlreadyReadError (I'm not sure about this; it may come back after the HTTP/1 workup once I see how a shared semantic layer shakes out).
  • We now send RST_STREAM frames if we complete a stream and the remote end is still open. This optimizes cases where the client may still be sending a body that we never consumed and don't care about
  • We no longer explicitly close the connection when we receive a GOAWAY frame

@mtrudel mtrudel marked this pull request as ready for review January 26, 2024 17:09
@mtrudel mtrudel force-pushed the refactor_http2 branch 4 times, most recently from 3b34925 to 8a5ab3f Compare January 27, 2024 16:00
@mtrudel mtrudel force-pushed the refactor_http2 branch 2 times, most recently from 59f5390 to 589f7a0 Compare February 8, 2024 19:21
…o take http1

Define HTTPTransport behaviour to model the two transport implementations
@mtrudel mtrudel merged commit ba755c0 into main Feb 9, 2024
27 checks passed
@mtrudel mtrudel deleted the refactor_http2 branch February 9, 2024 01:49
@mtrudel mtrudel mentioned this pull request Feb 9, 2024
mtrudel added a commit that referenced this pull request Feb 16, 2024
mtrudel added a commit that referenced this pull request Feb 16, 2024
mtrudel added a commit that referenced this pull request Mar 14, 2024
mtrudel added a commit that referenced this pull request Mar 14, 2024
mtrudel added a commit that referenced this pull request Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Assign this to a PR to have the benchmark CI suite run
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant