This repository has been archived by the owner on May 21, 2022. It is now read-only.
HTTP/2 improvements #68
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andyleclair
reviewed
Jul 8, 2020
samgaw
pushed a commit
to samgaw-archive/mojito
that referenced
this pull request
Oct 27, 2021
Picked from appcues#87 --- This avoids using chunked transfer encoding for HTTP/1.1. This also ensures the content-length headers is set for HTTP/2. The idea is if the connection is HTTP/1.1 then we just do the original behaviour before appcues#68. However, for HTTP/2 we still need to handle requests that violate either the maximum frame size or the stream window, or connection window size. So we check if the data can safely fit, and if it does then we do the original pre-fix behaviour. Otherwise, we perform the new post-fix behaviour. We do this to avoid sending a DATA frame which is empty. This part of the patch is probably the most controversial because it is just an optimisation and makes the code a bit more complicated and also reads some mint-internals that we are probably not meant to touch in order to work. See: https://github.com/elixir-mint/mint/blob/master/lib/mint/http2.ex#L1321 ``` defp add_default_content_length_header(headers, body) when body in [nil, :stream] do headers end defp add_default_content_length_header(headers, body) do Util.put_new_header_lazy(headers, "content-length", fn -> body |> IO.iodata_length() |> Integer.to_string() end) ``` So this content-length header adding behaviour should now be the same as pre appcues#68 fix. I'm not sure if it is correct in regards to people sending GET requests and using "" as the body instead of nil. However, this seems to be what mint does. I think this whole thing is a bit of a mess and I think optimally it should be all handled within Mint. However, I think the problem is Mint is so low level it has no concept of consuming packets from the socket so it can't possibly handle windowing itself. Also, I'm not sure if the mojito streaming correctly works in all scenarios. It should be possible to return an empty window size and if this happens we should block until the window increases. But I think in this scenario we send an empty data frame which is kind of odd. It also looks like we always block after sending a frame but this is not always necessary because we may have been splitting the data because of max frame size and not the window size. Also, there does not seem to be any timeouts when blocking. Additionally, we are using a catch all receive which does not compose correctly with other users of the process message queue. We should be explicitly matching for tcp_* and ssl_* messages with the correct socket.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes a bug where HTTP/2 window size was not being respected at request time. The bug resulted in e.g. POST requests with bodies over 64KB to fail.
Note that this PR breaks Dialyzer.
Mint.HTTP1.t()andMint.HTTP2.t()are both defined as opaque types, and the Mint library does not seem to provide a way to distinguish one from another without angering Dialyzer (The attempt to match a term of type 'Elixir.Mint.HTTP':t() against the pattern #{'__struct__' := 'Elixir.Mint.HTTP1'} breaks the opacity of the term).Fixes #54, implements #6.