elixir-mint / mint

Functional HTTP client for Elixir with support for HTTP/1 and HTTP/2 🌱
Apache License 2.0
1.36k stars 112 forks source link

Support queue of streamed HTTP1 requests #299

Closed howleysv closed 3 years ago

howleysv commented 3 years ago

This PR allows request body streaming to be used to send multiple requests on the same conn.

The current behaviour is to raise a FunctionClauseError if multiple streamed requests are sent on the same conn by the following steps:

{:ok, conn, ref1} = HTTP1.request(conn, "GET", "/", [], :stream)
{:ok, conn} = HTTP1.stream_request_body(conn, ref1, body1)
{:ok, conn} = HTTP1.stream_request_body(conn, ref1, :eof)

{:ok, conn, ref2} = HTTP1.request(conn, "GET", "/", [], :stream)
{:ok, conn} = HTTP1.stream_request_body(conn, ref2, body2)

The last statement will raise the error because it attempts to stream based on the state of conn.request, which refers to ref1, rather than the request with ref2.

The change in this PR is to make state & ref checks against the last queued request rather than the head of the queue conn.request. This allows multiple streamed requests (and a mix of streamed and non-streamed requests) to coexist on the same conn, while still enforcing that requests will error if an existing request is currently streaming a body.

ericmj commented 3 years ago

I think this is a good change overall and it was definitely an oversight that stream_request_body/3 does not work for pipelined requests.

But I am bit worried about the O(n) worst case performance of the reverse queue _r functions. I don't know if it would be an issue in practice or what a better solution would be. What are your thoughts?

howleysv commented 3 years ago

Maybe I could add another field to the conn struct to hold the (if any) actively streaming request. Then this can be enqueued once the request has finished sending. That should avoid the need to update already queued requests.

howleysv commented 3 years ago

@ericmj Thanks for the feedback, I've implemented the changes we discussed.

I'm not sure what happened with CI - it looks like hex was down when it tried to run.

ericmj commented 3 years ago

Please rebase against master to fix the CI issue.

whatyouhide commented 3 years ago

Thanks @howleysv! 💟