Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor connection pool #5081

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion config/config.example.yml
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ https_only: false
#disable_proxy: false

##
## Size of the HTTP pool used to connect to youtube. Each
## Max size of the HTTP pool used to connect to youtube. Each
## domain ('youtube.com', 'ytimg.com', ...) has its own pool.
##
## Accepted values: a positive integer
Expand All @@ -154,6 +154,28 @@ https_only: false
#pool_size: 100


##
## Max idle size of the HTTP pool used to connect to youtube. Each
## domain ('youtube.com', 'ytimg.com', ...) has its own pool.
##
## When unset this value has the same value as pool_size
##
## Accepted values: a positive integer
## Default: <none> (internally this means that it has the same value as pool_size)
##
#idle_pool_size:

##
## Amount of seconds to wait for a client to be free from the pool
## before raising an error
##
##
## Accepted values: a positive integer
## Default: 5
##
#pool_checkout_timeout: 5


##
## Additional cookies to be sent when requesting the youtube API.
##
Expand Down
15 changes: 13 additions & 2 deletions src/invidious.cr
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ require "protodec/utils"

require "./invidious/database/*"
require "./invidious/database/migrations/*"
require "./invidious/connection/*"
require "./invidious/http_server/*"
require "./invidious/helpers/*"
require "./invidious/yt_backend/*"
Expand Down Expand Up @@ -91,11 +92,21 @@ SOFTWARE = {
"branch" => "#{CURRENT_BRANCH}",
}

YT_POOL = YoutubeConnectionPool.new(YT_URL, capacity: CONFIG.pool_size)
YT_POOL = Invidious::ConnectionPool::Pool.new(
YT_URL,
max_capacity: CONFIG.pool_size,
idle_capacity: CONFIG.idle_pool_size,
timeout: CONFIG.pool_checkout_timeout
)

# Image request pool

GGPHT_POOL = YoutubeConnectionPool.new(URI.parse("https://yt3.ggpht.com"), capacity: CONFIG.pool_size)
GGPHT_POOL = Invidious::ConnectionPool::Pool.new(
URI.parse("https://yt3.ggpht.com"),
max_capacity: CONFIG.pool_size,
idle_capacity: CONFIG.idle_pool_size,
timeout: CONFIG.pool_checkout_timeout
)

# CLI
Kemal.config.extra_options do |parser|
Expand Down
9 changes: 8 additions & 1 deletion src/invidious/config.cr
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,15 @@ class Config
property port : Int32 = 3000
# Host to bind (overridden by command line argument)
property host_binding : String = "0.0.0.0"
# Pool size for HTTP requests to youtube.com and ytimg.com (each domain has a separate pool of `pool_size`)
# Max pool size for HTTP requests to youtube.com and ytimg.com (each domain has a separate pool)
property pool_size : Int32 = 100

# Idle pool size for HTTP requests to youtube.com and ytimg.com (each domain has a separate pool)
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

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

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.


# HTTP Proxy configuration
property http_proxy : HTTPProxyConfig? = nil

Expand Down
Original file line number Diff line number Diff line change
@@ -1,51 +1,3 @@
# Mapping of subdomain => YoutubeConnectionPool
# This is needed as we may need to access arbitrary subdomains of ytimg
private YTIMG_POOLS = {} of String => YoutubeConnectionPool

struct YoutubeConnectionPool
property! url : URI
property! capacity : Int32
property! timeout : Float64
property pool : DB::Pool(HTTP::Client)

def initialize(url : URI, @capacity = 5, @timeout = 5.0)
@url = url
@pool = build_pool()
end

def client(&)
conn = pool.checkout
# Proxy needs to be reinstated every time we get a client from the pool
conn.proxy = make_configured_http_proxy_client() if CONFIG.http_proxy

begin
response = yield conn
rescue ex
conn.close
conn = make_client(url, force_resolve: true)

response = yield conn
ensure
pool.release(conn)
end

response
end

private def build_pool
options = DB::Pool::Options.new(
initial_pool_size: 0,
max_pool_size: capacity,
max_idle_pool_size: capacity,
checkout_timeout: timeout
)

DB::Pool(HTTP::Client).new(options) do
next make_client(url, force_resolve: true)
end
end
end

def add_yt_headers(request)
request.headers.delete("User-Agent") if request.headers["User-Agent"] == "Crystal"
request.headers["User-Agent"] ||= "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/128.0.0.0 Safari/537.36"
Expand Down Expand Up @@ -99,18 +51,3 @@ def make_configured_http_proxy_client
password: config_proxy.password,
)
end

# Fetches a HTTP pool for the specified subdomain of ytimg.com
#
# Creates a new one when the specified pool for the subdomain does not exist
def get_ytimg_pool(subdomain)
if pool = YTIMG_POOLS[subdomain]?
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)
YTIMG_POOLS[subdomain] = pool

return pool
end
end
90 changes: 90 additions & 0 deletions src/invidious/connection/pool.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
module Invidious::ConnectionPool
struct Pool
property! url : URI
property! max_capacity : Int32
property! idle_capacity : Int32
property! timeout : Float64
syeopite marked this conversation as resolved.
Show resolved Hide resolved
property pool : DB::Pool(HTTP::Client)

def initialize(
url : URI,
*,
@max_capacity : Int32 = 5,
idle_capacity : Int32? = nil,
@timeout : Float64 = 5.0
)
if idle_capacity.nil?
@idle_capacity = @max_capacity
else
@idle_capacity = idle_capacity
end

@url = url

@pool = build_pool()
end

# Checks out a client in the pool
def client(&)
pool.checkout do |http_client|
# Proxy needs to be reinstated every time we get a client from the pool
http_client.proxy = make_configured_http_proxy_client() if CONFIG.http_proxy

response = yield http_client

return response
rescue ex : DB::Error
# Prevent broken client from being checked back into the pool
http_client.close
syeopite marked this conversation as resolved.
Show resolved Hide resolved
raise ConnectionPool::Error.new(ex.message, cause: ex)
ensure
pool.release(http_client)
syeopite marked this conversation as resolved.
Show resolved Hide resolved
end
rescue ex : DB::PoolTimeout
# Failed to checkout a client
raise ConnectionPool::Error.new(ex.message, cause: ex)
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it even useful for us to add the cause of the ConnectionPool exception here when we know that the original error is a DB::PoolTimeout? All it does really is produce a lengthy backtrace that can potentially confuse users

end

private def build_pool
# We call the getter for the instance variables instead of using them directly
# because the getters defined by property! ensures that the value is not a nil
options = DB::Pool::Options.new(
initial_pool_size: 0,
max_pool_size: max_capacity,
max_idle_pool_size: idle_capacity,
checkout_timeout: timeout
)

DB::Pool(HTTP::Client).new(options) do
next make_client(url, force_resolve: true)
end
end
end

class Error < Exception
end

# Mapping of subdomain => Invidious::ConnectionPool::Pool
# This is needed as we may need to access arbitrary subdomains of ytimg
private YTIMG_POOLS = {} of String => Invidious::ConnectionPool::Pool

# Fetches a HTTP pool for the specified subdomain of ytimg.com
#
# Creates a new one when the specified pool for the subdomain does not exist
def self.get_ytimg_pool(subdomain)
if pool = YTIMG_POOLS[subdomain]?
return pool
else
LOGGER.info("ytimg_pool: Creating a new HTTP pool for \"https://#{subdomain}.ytimg.com\"")
pool = Invidious::ConnectionPool::Pool.new(
URI.parse("https://#{subdomain}.ytimg.com"),
max_capacity: CONFIG.pool_size,
idle_capacity: CONFIG.idle_pool_size,
timeout: CONFIG.pool_checkout_timeout
)
YTIMG_POOLS[subdomain] = pool

return pool
end
end
end
8 changes: 4 additions & 4 deletions src/invidious/routes/images.cr
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ module Invidious::Routes::Images
end

begin
get_ytimg_pool(authority).client &.get(url, headers) do |resp|
Invidious::ConnectionPool.get_ytimg_pool(authority).client &.get(url, headers) do |resp|
syeopite marked this conversation as resolved.
Show resolved Hide resolved
env.response.headers["Connection"] = "close"
return self.proxy_image(env, resp)
end
Expand All @@ -65,7 +65,7 @@ module Invidious::Routes::Images
end

begin
get_ytimg_pool("i9").client &.get(url, headers) do |resp|
Invidious::ConnectionPool.get_ytimg_pool("i9").client &.get(url, headers) do |resp|
return self.proxy_image(env, resp)
end
rescue ex
Expand Down Expand Up @@ -111,7 +111,7 @@ module Invidious::Routes::Images
if name == "maxres.jpg"
build_thumbnails(id).each do |thumb|
thumbnail_resource_path = "/vi/#{id}/#{thumb[:url]}.jpg"
if get_ytimg_pool("i9").client &.head(thumbnail_resource_path, headers).status_code == 200
if Invidious::ConnectionPool.get_ytimg_pool("i9").client &.head(thumbnail_resource_path, headers).status_code == 200
name = thumb[:url] + ".jpg"
break
end
Expand All @@ -127,7 +127,7 @@ module Invidious::Routes::Images
end

begin
get_ytimg_pool("i").client &.get(url, headers) do |resp|
Invidious::ConnectionPool.get_ytimg_pool("i").client &.get(url, headers) do |resp|
return self.proxy_image(env, resp)
end
rescue ex
Expand Down
Loading