Skip to content
This repository has been archived by the owner on May 21, 2022. It is now read-only.

Ensure content-length header is sent for HTTP/1.1 and HTTP/2 #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DunyaKokoschka
Copy link

This avoids using chunked transfer encoding for HTTP/1.1. This
also ensures the content-length headers is set for HTTP/2.

This is a fix for #80

The idea is if the connection is HTTP/1.1 then we just do the original behaviour before #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.

If we need to stream the body we also ensure the content-length header is set. The mint code did this automatically but with the fix in #68 this no longer happened. 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 #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.

This avoids using chunked transfer encoding for HTTP/1.1. This
also ensures the content-length headers is set for HTTP/2.
@andyleclair
Copy link
Member

@DunyaKokoschka I think I understand what this is doing. Initially I think I was worried about pattern matching on the %Mint.HTTP1{} or %Mint.HTTP2{} struct, but I don't really see another option. I wish mint provided a way to get what type of connection you had without breaking the opacity of the type (see also elixir-mint/mint#263) but without that, I guess we have to pattern match the type.

Do you mind rebasing on master? there have been a few changes there that should get reflected here

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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants