Refactor connection pool #5081

syeopite · 2024-11-12T00:27:47Z

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

- Raises a wrapped exception on any DB:Error - Don't use a non-pool client when client fails - Ensure that client is always released

src/invidious/connection/pool.cr

pool.checkout(&block) already ensures that the checked out item will be released back into the pool

syeopite · 2024-11-12T17:21:53Z

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

src/invidious/connection/pool.cr

src/invidious/routes/images.cr

src/invidious/connection/pool.cr

SamantazFox · 2024-11-14T12:52:14Z

src/invidious/config.cr

+  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


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

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.

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.

@pool.release should not be called when the client has already been deleted from the pool.

Make non-block request method internally call the block based request method.

syeopite added 5 commits November 11, 2024 15:24

Add support for setting max idle http pool size

6ff4024

Refactor pool checkout logic

7116603

- Raises a wrapped exception on any DB:Error - Don't use a non-pool client when client fails - Ensure that client is always released

Move client logic file to connection subfolder

b044727

Move ytimg pool logic to Invidious::ConnectionPool

c026a7b

Add config to set connection pool checkout timeout

07caf93

syeopite requested review from SamantazFox, unixfox and a team as code owners November 12, 2024 00:27

Typo

5d60356

syeopite commented Nov 12, 2024

View reviewed changes

src/invidious/connection/pool.cr Outdated Show resolved Hide resolved

src/invidious/connection/pool.cr Outdated Show resolved Hide resolved

syeopite added 3 commits November 12, 2024 08:57

Remove redundant pool.release

e3313d3

pool.checkout(&block) already ensures that the checked out item will be released back into the pool

Delete broken clients from the pool explicitly

39afb33

Improve documentation of idle pool size

540dfe2

SamantazFox reviewed Nov 14, 2024

View reviewed changes

syeopite added 6 commits November 14, 2024 16:20

Release client only when it still exists

621c6ab

@pool.release should not be called when the client has already been deleted from the pool.

Pool: Refactor logic for request methods

aab9719

Make non-block request method internally call the block based request method.

Pool: remove redundant properties

6b3f665

Simplify namespace calls to ConnectionPool

4c1db6e

Update comment on reiniting proxy of pooled client

bdcd9af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor connection pool #5081

Refactor connection pool #5081

syeopite commented Nov 12, 2024

syeopite commented Nov 12, 2024

SamantazFox Nov 14, 2024

syeopite Nov 15, 2024

Refactor connection pool #5081

Are you sure you want to change the base?

Refactor connection pool #5081

Conversation

syeopite commented Nov 12, 2024

syeopite commented Nov 12, 2024

SamantazFox Nov 14, 2024

Choose a reason for hiding this comment

syeopite Nov 15, 2024

Choose a reason for hiding this comment