Meta: Finch deficiencies #994

New issue

Open

opened 2025-10-24 11:02:38 +00:00 by Oneric · 0 comments

Oneric commented

2025-10-24 11:02:38 +00:00

Owner

Meta issue to track bugs or deficiencies in Finch affecting us. Spun off from and extended since from a comment in #980.

Unfortunately the especially important points are also the hardest to realise.

The only other Tesla backend with built-in connection pooling atm is Hackney and supposedly also plagued with issues.
If we try to slap our own pooling onto another backend (or wrapping some lib into a custom Tesla backend), we’ll likely run into our own race conditions, spurious-copy ineffeciencies etc making it questionable whether this is worth the effort or if fixing Finch instead would be more productive.

General

race condition in pool cleanup can kill active connections
Made less likely already by us raising the poll timeout;
ref: #880, finch#292;
was mostly (but not fully) fixed by finch#292
no way to set cacerts in default pool opts without borking all plain-HTTP connections;
fixed by finch#333
IMPORTANT: finch needs a way to actually and reliably limit received response body size
Even the proposed workaround of manually using a stream and cancelling when receiving too much doesn't actually fully work for avoiding OOMs, due to Finch (or Mint, not sure) behind the scenes eagerly preloading further received response chunks and loading them into memory.
Allowing the limit to be rounded up to a multiple of chunk sizes is probably ok.
ref finch#224 and finch#282

Required for enabling HTTP/2.0 (with ALPN alongside HTTP/1.1)

don't raise exceptions if server offers server push responses
Akkoma-side workaround possible and already employed;
ref. Finch#325;
fixed by finch#333
take “max requests per connection” value signaled by the server into account
Else some requests will just immediately fail when we already have ongoing communication with the same remote.
For us just supporting waiting for a slot with a timeout as done in HTTP1 should be good enough. More generally though, HTTP2 should prob also support multiple connections per target OR multiple pools per target with fallback to another pool.
ref finch#165
ESSENTIAL for HTTP2: respect HTTP2’s max window size even when using ALPN negotation (does not affect HTTP/2.0-only pools)
This (from Akkoma’s POV unpredictably) breaks outgoing requests with a body .
Probably needs major rework of Finch’s pooling logic; currently ALPN negotiated links are treated as if HTTP1;
ref finch#265 :\

Akkoma TODOs

after the next (> 0.20.0) Finch release (#1058):
- updating :finch version requirement in mix.exs and refresh mix.lock
- close #880
- drop own server push workaround
- if desired: set our own cacerts instead of relying on the CAStore default (as we tried before but attempt was noop)

Meta issue to track bugs or deficiencies in Finch affecting us. Spun off from and extended since from [a comment in #980](https://akkoma.dev/AkkomaGang/akkoma/pulls/980#issuecomment-14852). Unfortunately the especially important points are also the hardest to realise. The only other Tesla backend with built-in connection pooling atm is Hackney and [supposedly](https://akkoma.dev/AkkomaGang/akkoma/pulls/980#issuecomment-14917) also plagued with issues. If we try to slap our own pooling onto another backend (or wrapping some lib into a custom Tesla backend), we’ll likely run into our own race conditions, spurious-copy ineffeciencies etc making it questionable whether this is worth the effort or if fixing Finch instead would be more productive. ## General - [x] race condition in pool cleanup can kill active connections Made less likely already by us raising the poll timeout; ref: #880, [finch#292](https://github.com/sneako/finch/issues/291); was mostly (but not fully) fixed by [finch#292](https://github.com/sneako/finch/pull/292) - [x] no way to set `cacerts` in default pool opts without borking all plain-HTTP connections; fixed by [finch#333](https://github.com/sneako/finch/pull/333) - [ ] **IMPORTANT:** finch needs a way to actually and reliably limit received response body size Even the proposed workaround of manually using a stream and cancelling when receiving too much doesn't actually fully work for avoiding OOMs, due to Finch (or Mint, not sure) behind the scenes eagerly preloading further received response chunks and loading them into memory. Allowing the limit to be rounded up to a multiple of chunk sizes is probably ok. ref [finch#224](https://github.com/sneako/finch/issues/224) and [finch#282](https://github.com/sneako/finch/issues/282) ## Required for enabling HTTP/2.0 (with ALPN alongside HTTP/1.1) - [x] don't raise exceptions if server offers server push responses Akkoma-side workaround possible and already employed; ref. [Finch#325](https://github.com/sneako/finch/issues/325); fixed by [finch#333](https://github.com/sneako/finch/pull/333) - [ ] take “max requests per connection” value signaled by the server into account Else some requests will just immediately fail when we already have ongoing communication with the same remote. For us just supporting waiting for a slot with a timeout as done in HTTP1 should be good enough. More generally though, HTTP2 should prob also support multiple connections per target OR multiple pools per target *with* fallback to another pool. ref [finch#165](https://github.com/sneako/finch/issues/164) - [ ] **ESSENTIAL for HTTP2:** respect HTTP2’s max window size even when using ALPN negotation *(does not affect HTTP/2.0-only pools)* This (from Akkoma’s POV unpredictably) breaks outgoing requests with a body . Probably needs major rework of Finch’s pooling logic; currently ALPN negotiated links are treated as if HTTP1; ref [finch#265](https://github.com/sneako/finch/issues/265) :\ -------- ## Akkoma TODOs - [x] after the next (`> 0.20.0`) Finch release (#1058): - updating `:finch` version requirement in `mix.exs` and refresh `mix.lock` - close #880 - drop own server push workaround - if desired: set our own `cacerts` instead of relying on the `CAStore` default (as we tried before but attempt was noop)