Skip to content

Refactor connection pool#5081

Open
syeopite wants to merge 25 commits intoiv-org:masterfrom
syeopite:connection-pool-improvements
Open

Refactor connection pool#5081
syeopite wants to merge 25 commits intoiv-org:masterfrom
syeopite:connection-pool-improvements

Conversation

@syeopite
Copy link
Member

@syeopite syeopite commented Nov 12, 2024

Brings improvements from https://github.com/jgaskins/http_client which is a shard that implements a HTTP connection pool through DB::Pool like Invidious.

The most noticeable change is that connection timeout errors will no longer say DB::PoolTimeout but rather Invidious::ConnectionPool::Error. Full backtrace here.

Backtrace 
Title: `Could not check out a connection in 5.0 seconds (Invidious::ConnectionPool::Error)`
Date: `2024-11-12T00:15:28Z`
Route: `/search?q=test`
Version: `2024.11.11-c026a7b1 @ connection-pool-improvements`

<details>
<summary>Backtrace</summary>
<p>
   
```
Could not check out a connection in 5.0 seconds (Invidious::ConnectionPool::Error)
  from src/invidious/connection/pool.cr:45:7 in '_post_json'
  from src/invidious/yt_backend/youtube_api.cr:569:12 in 'search'
  from src/invidious/search/processors.cr:10:7 in 'regular'
  from src/invidious/search/query.cr:127:9 in 'process'
  from src/invidious/routes/search.cr:61:17 in 'search'
  from src/invidious/routing.cr:184:20 in '->'
  from lib/kemal/src/kemal/route.cr:12:26 in '->'
  from src/invidious/helpers/handlers.cr:30:37 in 'process_request'
  from lib/kemal/src/kemal/route_handler.cr:17:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/websocket_handler.cr:13:14 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/filter_handler.cr:21:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:157:5 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:94:12 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:146:5 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:70:5 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/ext/kemal_static_file_handler.cr:162:16 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/exception_handler.cr:8:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/logger.cr:21:35 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/init_handler.cr:12:7 in 'call'
  from /usr/share/crystal/src/http/server/request_processor.cr:51:11 in 'process'
  from /usr/share/crystal/src/http/server.cr:521:5 in 'handle_client'
  from /usr/share/crystal/src/http/server.cr:451:5 in '->'
  from /usr/share/crystal/src/fiber.cr:143:11 in 'run'
  from /usr/share/crystal/src/fiber.cr:95:34 in '->'
  from ???
Caused by: Could not check out a connection in 5.0 seconds (DB::PoolTimeout)
  from lib/db/src/db/pool.cr:261:7 in 'wait_for_available'
  from lib/db/src/db/pool.cr:127:35 in 'checkout'
  from lib/db/src/db/pool.cr:152:20 in '_post_json'
  from src/invidious/yt_backend/youtube_api.cr:569:12 in 'search'
  from src/invidious/search/processors.cr:10:7 in 'regular'
  from src/invidious/search/query.cr:127:9 in 'process'
  from src/invidious/routes/search.cr:61:17 in 'search'
  from src/invidious/routing.cr:184:20 in '->'
  from lib/kemal/src/kemal/route.cr:12:26 in '->'
  from src/invidious/helpers/handlers.cr:30:37 in 'process_request'
  from lib/kemal/src/kemal/route_handler.cr:17:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/websocket_handler.cr:13:14 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/filter_handler.cr:21:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:157:5 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:94:12 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:146:5 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/handlers.cr:70:5 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/ext/kemal_static_file_handler.cr:162:16 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/exception_handler.cr:8:7 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from src/invidious/helpers/logger.cr:21:35 in 'call'
  from /usr/share/crystal/src/http/server/handler.cr:30:7 in 'call_next'
  from lib/kemal/src/kemal/init_handler.cr:12:7 in 'call'
  from /usr/share/crystal/src/http/server/request_processor.cr:51:11 in 'process'
  from /usr/share/crystal/src/http/server.cr:521:5 in 'handle_client'
  from /usr/share/crystal/src/http/server.cr:451:5 in '->'
  from /usr/share/crystal/src/fiber.cr:143:11 in 'run'
  from /usr/share/crystal/src/fiber.cr:95:34 in '->'
  from ???
```
</p>
</details>

I've also added a configuration option to customize max idle pool size and the checkout timeout

@syeopite syeopite requested review from a team, SamantazFox and unixfox as code owners November 12, 2024 00:27
syeopite
syeopite commented Nov 12, 2024
View reviewed changes
@syeopite
Copy link
Member Author

syeopite commented Nov 12, 2024

TODO: Add connection pool stats to /api/v1/stats

https://crystal-lang.github.io/crystal-db/api/0.13.1/DB/Pool.html#stats-instance-method

https://github.com/crystal-lang/crystal-db/blob/892c25675634fcdf59780055c872be8468b2c204/src/db/pool.cr#L103-L110

SamantazFox
SamantazFox reviewed Nov 14, 2024
View reviewed changes
src/invidious/config.cr Outdated
property idle_pool_size : Int32? = nil

# Amount of seconds to wait for a client to be free from the pool before rasing an error
property pool_checkout_timeout : Int32 = 5
Copy link
Member

@SamantazFox SamantazFox Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oddly, the Crystal compiler didn't complain: there is a type mismatch between this property (Int32) and Pool.timeout (Float64).

Copy link
Member Author

@syeopite syeopite Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops I didn't notice that

But I think that's an intended feature of Crystal. The compiler allows you to use an integer in place of a float but not the other way around.

syeopite
syeopite commented Dec 17, 2024
View reviewed changes
SamantazFox
SamantazFox requested changes Apr 6, 2025
View reviewed changes
syeopite added 22 commits April 9, 2025 13:07
@syeopite
Add support for setting max idle http pool size
78b7db1
@syeopite
Refactor connection pooling logic
247254b 
- Remove duplication between standard and companion pool
- Raises a wrapped exception on any DB:Error
- Don't use a non-pool client when client fails
- Ensure that client is always released
- Add documentation to various pool methods
@syeopite
Move client logic file to connection subfolder
27b66d2
@syeopite
Move ytimg pool logic to Invidious::ConnectionPool
347273d
@syeopite
Add config to set connection pool checkout timeout
ef07f78
@syeopite
Typo
739c350
@syeopite
Remove redundant pool.release
4fd6f79 
pool.checkout(&block) already ensures that the checked out item
will be released back into the pool
@syeopite
Delete broken clients from the pool explicitly
0d5887c
@syeopite
Improve documentation of idle pool size
4ce0f77
@syeopite
Connection pool: ensure response is fully read
e9b2db4 
The streaming API of HTTP::Client has an internal buffer
that will continue to persist onto the next request unless
the response is fully read.

This commit privatizes the #client method of Pool and instead
expose various HTTP request methods that will call and yield
the underlying request and response.

This way, we can ensure that the resposne is fully read before
the client is passed back into the pool for another request.
@syeopite
Release client only when it still exists
35ddc67 
@pool.release should not be called when the client has already been
deleted from the pool.
@syeopite
Pool: Refactor logic for request methods
734c84c 
Make non-block request method internally call
the block based request method.
@syeopite
Pool: remove redundant properties
4c44601
@syeopite
Simplify namespace calls to ConnectionPool
692df7d
@syeopite
Update comment on reiniting proxy of pooled client
cdb6eef
@syeopite
Use non-streaming api when not invoked with block
a3864e6 
Defaulting to the streaming api of `HTTP::Client` causes some issues
since the streaming respone content needs to be accessed
through #body_io rather than #body
@syeopite
Add tests for connection pool
ebefa5e
@syeopite
Pool: Make Pool#client method public
f32c954
@syeopite
Pool: raise custom error when DB::PoolTimeout
fbccb6a
@syeopite
Merge companion and standard pool into one
7901906
@syeopite
Fix ameba complaints
8be9d85
@syeopite
Remove extraneous space
24a6c31
@syeopite
Remove idle_pool_size config
0865bc5 
Clients created when idle_capacity is reached but not max_capacity
are discarded as soon as the client is checked back into the pool,
not when the connection is closed.

This means that allowing idle_capacity to be lower than max_capacity
essentially just makes the remaining clients a checkout timeout
deterrent that gets thrown away as soon as it is used. Not useful
for reusing connections whatsoever during peak load times
@syeopite syeopite force-pushed the connection-pool-improvements branch from bdcd9af to 0865bc5 Compare April 10, 2025 07:47
@syeopite syeopite mentioned this pull request Apr 10, 2025
Draft
syeopite
syeopite commented Apr 10, 2025
View reviewed changes
config/config.example.yml
## before raising an error
##
##
## Accepted values: a positive integer
Copy link
Member Author

@syeopite syeopite Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Accepted values: a positive integer
## Accepted values: a positive floating point number

@syeopite syeopite mentioned this pull request Jun 2, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SamantazFox SamantazFox SamantazFox requested changes

@unixfox unixfox Awaiting requested review from unixfox unixfox is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@syeopite @SamantazFox