CrowdHailer / Ace

HTTP web server and client, supports http1 and http2
https://hex.pm/packages/ace
MIT License
305 stars 26 forks source link

Add HTTP/1 backpressure #115

Closed nietaki closed 6 years ago

nietaki commented 6 years ago

Should fix #114.

It's going to be good if the :ack logic gets some more eyes on it, but it should be ok and with this patch my code from the related issue has a very steady message queue size of 10 instead of a perpetually increasing one.

The @max_pending_request_part_count 10 value was chosen relatively arbitrarily. Any positive value would work and I think any small integer greater or equal to 2 would be good. If we set it to 1, the Endpoint won't ask the socket for more data before the Worker receives the last chunk(s) of data, so it might slow down the request streaming. We can probably decrease it to something like 4.

Instead of sending the acks conditionally only if the request is HTTP/1 I chose to send them regardless and have them be a no-op in HTTP/2. The performance penalty should be irrelevant, it keeps the code simpler and provides a starting point for implementing a similar mechanism for HTTP/2.

I tried to keep the code style consistent with the rest of it, let me know what you think.

CrowdHailer commented 6 years ago

Looks good. :whale:

When we merge should probably make an issue for backpressure in HTTP/2 case. also needs a changelog entry