Increasing the max number of headers allowed in HTTP requests

[dmu1313]

This is just sharing the results of a discussion that occurred elsewhere for others to potentially view in the future. Also, we can track the final result of this issue here.

While using pushpin, which uses condure, I observed that pushpin was closing some connections due to error. The specific error message was this: [DEBUG] 2022-04-11 08:19:38.945 [condure] [condure::connection] conn 0-2-76c: error: Http(ParseError(TooManyHeaders))

From this message, the error appears to be coming from a max header limit that is set in condure's code.

Here, the number of headers allowed in an HTTP request is limited to 32: https://github.com/fanout/condure/blob/373bf5679b538dfe729cf922a5bf09e8a925cb4f/src/http1.rs#L25 https://github.com/fanout/condure/blob/373bf5679b538dfe729cf922a5bf09e8a925cb4f/src/http1.rs#L371

Here the number of headers in the HTTP response is also limited to 32: https://github.com/fanout/condure/blob/373bf5679b538dfe729cf922a5bf09e8a925cb4f/src/zhttppacket.rs#L27 https://github.com/fanout/condure/blob/373bf5679b538dfe729cf922a5bf09e8a925cb4f/src/zhttppacket.rs#L535-L547 https://github.com/fanout/condure/blob/373bf5679b538dfe729cf922a5bf09e8a925cb4f/src/zhttppacket.rs#L779-L781

I propose that we increase the limit. I believe 32 headers is relatively easy to reach because in my testing I found that Firefox on my computer was sending 16 headers by itself, and applications using CloudFlare (such as mine) will have a few more added by CloudFlare, and then finally custom headers added in nginx or other places will easily push it past 32.

I also had a couple other potential ideas, although, I do not know if these fit into the design of condure:

1. The arrays holding the headers are set to be exactly of size 32, but it may be worth converting the headers to a dynamically sized array so that any HTTP request is supported.
2. Maybe condure could allow users to pass in a configuration param that specifies the max number of headers and let users override a default of 32. This means only users who need the increase can increase it.

[dmu1313]

An initial PR to simply increase the limit was made here: https://github.com/fanout/condure/pull/8

However, it appears this increases memory usage by more than expected, so it requires more thorough investigation by @jkarneges

[jkarneges]

The general memory usage issue has been fixed in 839919251e6e042cecaa415d5968329baeeb291c, which means changing HEADERS_MAX now has only modest effect on memory usage. I've gone ahead and increased it to 64 in a4af3624af36643d5cbe2545ae91e85ba46e9919.

I always knew there was something fishy with the way our async functions were laying out memory, but until now I wasn't sure why. Fiddling with HEADERS_MAX helped illuminate what was going on. The fix resulted in a pretty dramatic reduction in memory usage. Stream connection tasks now use around 50% less memory. Your feature request had some nice side effects. :)

[jkarneges]

I don't think we'd want to allow an arbitrary number of headers, as one of the design goals of condure is to ensure the amount of memory per-connection is predictable.

Making the limit configurable is a reasonable idea.

It occurs to me that headers are also limited by the buffer size, which means there's already an implicit max. For example, the default buffer size is 8192 bytes, and if we assume a parsable header requires at least 4 bytes (just a guess), then there'd be an implicit max of around 2048 even if no other limit was enforced. That'd be too big of a default though.