Proxy: Use connection pools for images #4326
Conversation
|
Can't test as I have an external proxy for that. But sounds good |
| return pool | ||
| else | ||
| LOGGER.info("ytimg_pool: Creating a new HTTP pool for \"https://#{subdomain}.ytimg.com\"") | ||
| pool = YoutubeConnectionPool.new(URI.parse("https://#{subdomain}.ytimg.com"), capacity: CONFIG.pool_size) |
There was a problem hiding this comment.
I'm concerned about keeping that many open connections on small instances. Maybe we could bypass the pooling system if CONFIG.pool_size == 0?
There was a problem hiding this comment.
Does the pool not automatically clean up connections?
There was a problem hiding this comment.
I don't think so? It seems that released connections are pushed to the idle list, but I'm not sure what causes them to be closed:
https://github.com/crystal-lang/crystal-db/blob/3eaac85a5d4b7bee565b55dcb584e84e29fc5567/src/db/pool.cr#L151-L185
There was a problem hiding this comment.
Well that's a problem...
Regarding the bypass though, shouldn't that already be what DB:Pool does when pool_size equals 0? Invidious seems to be able to handle requests fine when CONFIG.pool_size is set to zero at least
There was a problem hiding this comment.
YoutubeConnectionPool sets the max_pool_size and max_idle_pool_size to the value of capacity. So meaning the pool will start out with 0 clients, and create more up to capacity, but then it'll keep all of them around forever.
There is also not currently a way to set a timeout on idle connections. I.e. that would expire idle connections after some period of time. Is some discussion related to this in crystal-lang/crystal-db#47.
In the meantime it might be worth allowing to customize the max_idle_pool_size vs always assuming it should be CONFIG.pool_size big. E.g. have a higher max_pool_size to handle bursts, but then keep max_idle_pool_size set to something smaller for normal traffic. 1 might be a good starting point, could run some benchmarks to figure out a good value.
|
I wonder if something similar to |
It would have to be somewhat different. There are so many video servers that keeping connections pools to every single one would be wasteful. But keeping the connection open per client (= almost for each video) would be nice, though I have no idea how to do that efficiently.... |
Regression from iv-org#2364
SamantazFox
added
the
need-testing
SamantazFox
added
ready
and removed
need-testing
SamantazFox
changed the title
|
Thanks for your work :) |
Theoretically this should improve memory usage and performance by quite a bit as we aren't creating a new
HTTP::Clientand in a turn a new connection for every image we request from YouTube.This should be tested extensively, especially in IPv6 only environment.
Closes #4009