Fixes #39291 - Add persistent HTTP connection pooling for Candlepin#11726
Fixes #39291 - Add persistent HTTP connection pooling for Candlepin#11726pablomh wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAdds an optional persistent HTTP layer for Candlepin using Net::HTTP::Persistent, integrates it into ChangesPersistent HTTP Connection
Sequence DiagramsequenceDiagram
participant Client
participant CandlepinResource
participant NetHTTPPersistent
participant CandlepinServer
Client->>CandlepinResource: call .issue_request(method, path, headers, payload)
CandlepinResource->>CandlepinResource: build & OAuth-sign HTTP request
CandlepinResource->>NetHTTPPersistent: execute signed request
NetHTTPPersistent->>CandlepinServer: HTTP request
CandlepinServer-->>NetHTTPPersistent: HTTP response
NetHTTPPersistent-->>CandlepinResource: raw response
CandlepinResource->>CandlepinResource: wrap as PooledResponse / translate errors
CandlepinResource-->>Client: return PooledResponse or raise mapped error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/katello/resources/candlepin/persistent_connection.rb`:
- Line 1: The file uses OpenStruct inside the PooledResponse class but doesn't
require 'ostruct', which causes a NameError on Ruby 3.x; add an explicit require
'ostruct' near the top of the file (above or alongside the existing require
'net/http/persistent') so PooledResponse can instantiate OpenStruct reliably.
Ensure the require precedes any use of OpenStruct in class PooledResponse.
- Around line 77-84: Headers with underscores must be normalized to proper HTTP
header names before assignment so request['Content-Type'] checks work; in
persistent_connection.rb, when iterating (headers || {}).each do |k, v|,
transform the key (k) to replace underscores with hyphens and canonicalize
capitalization (e.g. "content_type" -> "Content-Type") then use that normalized
key when setting request[...] = CONTENT_TYPE_MAP[v] || v.to_s; this ensures
request['Content-Type'] is present when payload is set and the existing
payload/content_type logic continues to work.
In `@app/lib/katello/resources/candlepin/proxy.rb`:
- Around line 11-12: The proxied calls currently pass inline header hashes which
bypass CandlepinResource.default_headers and one use of merge! mutates caller
state; update each CandlepinResource.post/get invocation (the calls using
path_with_cp_prefix(path) and similar at the noted locations) to build headers
by starting from CandlepinResource.default_headers and merging in
User.cp_oauth_header plus the content/accept entries (e.g. :accept => :json,
:content_type => :json) without using merge! so accept-language and
X-Correlation-ID are preserved and no external hash is mutated; ensure all
occurrences (including the get call that currently does merge!) use this
pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74e13eb3-e443-4110-8673-ad62c28ac9db
📒 Files selected for processing (5)
app/lib/katello/resources/candlepin.rbapp/lib/katello/resources/candlepin/persistent_connection.rbapp/lib/katello/resources/candlepin/proxy.rbkatello.gemspectest/lib/resources/persistent_connection_test.rb
| (headers || {}).each do |k, v| | ||
| request[k.to_s] = CONTENT_TYPE_MAP[v] || v.to_s | ||
| end | ||
|
|
||
| if payload | ||
| request.body = payload | ||
| request.content_type = 'application/json' unless request['Content-Type'] | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Ruby's Net::HTTP, if a header is assigned with request['content_type'] = 'application/json', is it preserved as content_type/Content_typeon the wire, or normalized toContent-Type?
💡 Result:
In Ruby's Net::HTTP, when you assign a header using request['content_type'] = 'application/json', the key is stored in lowercase as 'content_type' in the internal @header hash because Net::HTTPHeader#[]=(key, val) does key.downcase before storing [1][2][3][4][5]. When the request is sent on the wire, Net::HTTP uses each_header, which iterates over @header.each do |k,va| yield k, va.join(', ') end, sending the header names in their stored lowercase form (e.g., "content_type: application/json") [3]. There is a separate capitalize method in Net::HTTPHeader that title-cases header names (e.g., "Content-Type") by splitting on '-' and capitalizing each word [6], but it is used in iteration methods like each_capitalized_name (deprecated) or each_capitalized_name, not in the actual request writing process [3][7][4][6]. Examples confirm lowercase transmission unless custom classes override to preserve case [8][1]. Thus, it is preserved as 'content_type' on the wire, not normalized to 'Content-Type'. HTTP header names are case-insensitive per RFC, so this is valid [9].
Citations:
- 1: https://stackoverflow.com/questions/2710373/preserving-case-in-http-headers-with-rubys-nethttp
- 2: https://docs.ruby-lang.org/en/2.2.0/Net/HTTPHeader.html
- 3: https://github.com/ruby/net-http/blob/master/lib/net/http/header.rb
- 4: https://github.com/ruby/ruby/blob/v3_0_7/lib/net/http/header.rb
- 5: https://ruby-doc.org/stdlib-2.6/libdoc/net/http/rdoc/Net/HTTPHeader.html
- 6: https://github.com/wycats/net-http/blob/master/lib/net2/http/header.rb
- 7: https://git.ruby-lang.org/ruby.git/tree/lib/net/http/header.rb?h=ruby_2_7&id=1f4d4558484b370999954f3ede7e3aa3a3a01ef3
- 8: Issue with Content Type ruby/net-http#148
- 9: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Type
🏁 Script executed:
# First, find the exact file and read the relevant section
find . -type f -name "persistent_connection.rb" | head -5Repository: Katello/katello
Length of output: 122
🏁 Script executed:
# Get context around lines 77-84 to understand the full picture
if [ -f "app/lib/katello/resources/candlepin/persistent_connection.rb" ]; then
wc -l "app/lib/katello/resources/candlepin/persistent_connection.rb"
fiRepository: Katello/katello
Length of output: 124
🏁 Script executed:
# Read the file to see full method context
cat -n "app/lib/katello/resources/candlepin/persistent_connection.rb" | sed -n '60,100p'Repository: Katello/katello
Length of output: 1922
🏁 Script executed:
# Look for tests related to persistent_connection
find . -type f -name "*persistent_connection*test*.rb" -o -name "*persistent_connection*spec*.rb" 2>/dev/nullRepository: Katello/katello
Length of output: 110
🏁 Script executed:
# Read the test file to understand what's being tested
cat -n "./test/lib/resources/persistent_connection_test.rb" | head -100Repository: Katello/katello
Length of output: 4753
🏁 Script executed:
# Search for CONTENT_TYPE_MAP definition
rg "CONTENT_TYPE_MAP" -B 2 -A 5Repository: Katello/katello
Length of output: 1666
🏁 Script executed:
# Search for how headers are passed in tests or throughout codebase
rg "issue_request.*headers" -B 1 -A 3Repository: Katello/katello
Length of output: 4304
🏁 Script executed:
# Search for actual calls to issue_request with non-empty headers
rg "issue_request.*headers.*:" -A 2 | head -40Repository: Katello/katello
Length of output: 2781
🏁 Script executed:
# Check if headers are ever passed with underscore style (like :accept, :content_type)
rg "headers\s*[:=].*\{.*[:_]" | head -20Repository: Katello/katello
Length of output: 2244
🏁 Script executed:
# Look at RestClient usage and how it converts headers
grep -r "RestClient" app/lib/katello/resources/candlepin/ | grep -v ".pyc" | head -10Repository: Katello/katello
Length of output: 1216
🏁 Script executed:
# Search for how headers are constructed or passed before issue_request
rg "accept|content_type|content-type" app/lib/katello/resources/candlepin/ -B 2 -A 2Repository: Katello/katello
Length of output: 6775
Normalize header key underscores to hyphens for proper HTTP header formatting.
Headers passed with underscore symbols (:accept, :content_type) are converted to lowercase strings ("accept", "content_type") and sent on the wire in that form, not as proper HTTP headers ("Accept", "Content-Type"). This breaks the condition at line 83, which checks request['Content-Type'] (title-case) but won't match request['content_type'] (underscore) set by line 78. GET/DELETE requests won't have the Content-Type header set even when intended.
Suggested change
(headers || {}).each do |k, v|
- request[k.to_s] = CONTENT_TYPE_MAP[v] || v.to_s
+ request[k.to_s.tr('_', '-')] = CONTENT_TYPE_MAP[v] || v.to_s
end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/lib/katello/resources/candlepin/persistent_connection.rb` around lines 77
- 84, Headers with underscores must be normalized to proper HTTP header names
before assignment so request['Content-Type'] checks work; in
persistent_connection.rb, when iterating (headers || {}).each do |k, v|,
transform the key (k) to replace underscores with hyphens and canonicalize
capitalization (e.g. "content_type" -> "Content-Type") then use that normalized
key when setting request[...] = CONTENT_TYPE_MAP[v] || v.to_s; this ensures
request['Content-Type'] is present when payload is set and the existing
payload/content_type logic continues to work.
| CandlepinResource.post(path_with_cp_prefix(path), body, | ||
| {:accept => :json, :content_type => :json}.merge(User.cp_oauth_header)) |
There was a problem hiding this comment.
Reuse CandlepinResource.default_headers for these proxied calls.
These inline hashes bypass CandlepinResource.default_headers, so proxied requests stop sending accept-language and X-Correlation-ID. get also mutates the caller’s extra_headers via merge!. Reusing the shared header builder preserves the previous request contract and avoids the mutation.
Suggested change
def self.post(path, body)
logger.debug "Sending POST request to Candlepin: #{path}"
- CandlepinResource.post(path_with_cp_prefix(path), body,
- {:accept => :json, :content_type => :json}.merge(User.cp_oauth_header))
+ CandlepinResource.post(path_with_cp_prefix(path), body, CandlepinResource.default_headers)
end
def self.delete(path, body = nil)
logger.debug "Sending DELETE request to Candlepin: #{path}"
- headers = {:accept => :json, :content_type => :json}.merge(User.cp_oauth_header)
+ headers = CandlepinResource.default_headers
if body
CandlepinResource.delete(path_with_cp_prefix(path), body, headers)
else
CandlepinResource.delete(path_with_cp_prefix(path), headers)
end
@@
def self.get(path, extra_headers = {})
logger.debug "Sending GET request to Candlepin: #{path}"
- CandlepinResource.get(path_with_cp_prefix(path),
- extra_headers.merge!(default_request_headers))
+ CandlepinResource.get(
+ path_with_cp_prefix(path),
+ CandlepinResource.default_headers.merge(extra_headers)
+ )
rescue RestClient::NotModified => e
e.response
end
def self.put(path, body)
logger.debug "Sending PUT request to Candlepin: #{path}"
- CandlepinResource.put(path_with_cp_prefix(path), body,
- {:accept => :json, :content_type => :json}.merge(User.cp_oauth_header))
+ CandlepinResource.put(path_with_cp_prefix(path), body, CandlepinResource.default_headers)
endAlso applies to: 17-21, 27-28, 35-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/lib/katello/resources/candlepin/proxy.rb` around lines 11 - 12, The
proxied calls currently pass inline header hashes which bypass
CandlepinResource.default_headers and one use of merge! mutates caller state;
update each CandlepinResource.post/get invocation (the calls using
path_with_cp_prefix(path) and similar at the noted locations) to build headers
by starting from CandlepinResource.default_headers and merging in
User.cp_oauth_header plus the content/accept entries (e.g. :accept => :json,
:content_type => :json) without using merge! so accept-language and
X-Correlation-ID are preserved and no external hash is mutated; ensure all
occurrences (including the get call that currently does merge!) use this
pattern.
Each Candlepin request currently creates a new TCP+TLS connection via RestClient, then tears it down. Under concurrent registration, this generates thousands of TIME_WAIT sockets and wastes ~30ms per TLS handshake. Add a Net::HTTP::Persistent connection pool that maintains one TCP+TLS connection per Puma thread, reused across all Candlepin calls within that thread. Stale connections are detected and re-established automatically by the library. Key changes: - New PersistentConnection module included in CandlepinResource - PooledResponse wrapper preserving RestClient::Response contract - Proxy routes through CandlepinResource.get/post/put/delete - UpstreamCandlepinResource opts out (different server) - Requires net-http-persistent gem (MIT, connection_pool already in foreman-packaging) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pablomh
force-pushed
the
registration/candlepin-connection-pooling
branch
from
4ef0d58 to
59bc505
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
test/lib/resources/persistent_connection_test.rb (1)
124-135:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest assertion validates the wrong header key — it should check
content-type(hyphen), notcontent_type(underscore).Line 127 asserts
req['content_type'] == 'application/json', which confirms the current underscore behavior rather than catching the bug. If the header normalization fix (tr('_', '-')) is applied topersistent_connection.rb, this test would need to be updated to checkreq['content-type']instead.💡 Suggested update (once header normalization is fixed)
mock_http.expects(:request).with do |_uri, req| - req['accept'] == 'application/json' && req['content_type'] == 'application/json' + req['accept'] == 'application/json' && req['content-type'] == 'application/json' end.returns(`@success_response`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/lib/resources/persistent_connection_test.rb` around lines 124 - 135, The test test_symbol_header_values_are_translated currently asserts the wrong header key; update the expectation inside the mock for persistent_http (the block passed to mock_http.expects(:request)) to check req['content-type'] == 'application/json' instead of req['content_type'] so it will validate the hyphenated header name after the header normalization change in persistent_connection.rb; keep the accept assertion as req['accept'] == 'application/json' and ensure CandlepinResource.issue_request call (method: :get, path: '/candlepin/status', headers: {accept: :json, content_type: :json}) remains the same so the test verifies translation from symbol/underscore to hyphenated header keys.app/lib/katello/resources/candlepin/proxy.rb (1)
11-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
accept-language/X-Correlation-IDstill missing from proxied requests;merge!on line 28 still mutates the caller's hash.These issues were raised in the previous review round and are still unresolved.
CandlepinResource.default_headersbuildsaccept-language(for Candlepin error localization) andX-Correlation-ID(for distributed tracing), neither of which appear in the inline header hashes used bypost/put/delete.getadditionally callsextra_headers.merge!(default_request_headers), which mutates the hash the controller passed in.💡 Suggested fix (same as prior review)
def self.post(path, body) logger.debug "Sending POST request to Candlepin: #{path}" - CandlepinResource.post(path_with_cp_prefix(path), body, - {:accept => :json, :content_type => :json}.merge(User.cp_oauth_header)) + CandlepinResource.post(path_with_cp_prefix(path), body, CandlepinResource.default_headers) end def self.delete(path, body = nil) logger.debug "Sending DELETE request to Candlepin: #{path}" - headers = {:accept => :json, :content_type => :json}.merge(User.cp_oauth_header) + headers = CandlepinResource.default_headers if body CandlepinResource.delete(path_with_cp_prefix(path), body, headers) else CandlepinResource.delete(path_with_cp_prefix(path), headers) end end def self.get(path, extra_headers = {}) logger.debug "Sending GET request to Candlepin: #{path}" - CandlepinResource.get(path_with_cp_prefix(path), - extra_headers.merge!(default_request_headers)) + CandlepinResource.get( + path_with_cp_prefix(path), + CandlepinResource.default_headers.merge(extra_headers) + ) rescue RestClient::NotModified => e e.response end def self.put(path, body) logger.debug "Sending PUT request to Candlepin: #{path}" - CandlepinResource.put(path_with_cp_prefix(path), body, - {:accept => :json, :content_type => :json}.merge(User.cp_oauth_header)) + CandlepinResource.put(path_with_cp_prefix(path), body, CandlepinResource.default_headers) endAlso applies to: 17-18, 27-28, 35-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/katello/resources/candlepin/proxy.rb` around lines 11 - 12, The proxied requests in proxy.rb (methods calling CandlepinResource.post/put/delete/get that currently merge User.cp_oauth_header into inline header hashes) must include CandlepinResource.default_headers (to add accept-language and X-Correlation-ID) and must never mutate the caller's headers; replace any use of merge! (e.g. extra_headers.merge!(default_request_headers)) with non-destructive merge and chain merges so headers = (extra_headers || {}).merge(CandlepinResource.default_headers).merge(User.cp_oauth_header).merge(your_content_type_accept) before calling CandlepinResource.post/put/delete/get, and update all occurrences referenced in the diff (the get, post, put, delete call sites and any use of default_request_headers or merge!) accordingly.app/lib/katello/resources/candlepin/persistent_connection.rb (1)
78-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHeader keys with underscores are sent as non-standard wire names (
content_typeinstead ofContent-Type).
k.to_sconverts:content_type→"content_type", whichNet::HTTPstores and sends on the wire as-is. HTTP servers typically accept it (RFC 7230 mandates case-insensitivity), but Candlepin or a proxy may reject the_-separated form. This also makes the guard at line 84 (request['Content-Type']) unreliable:Net::HTTP::[]lowercases but doesn't de-underscore, so it never finds"content_type", causingrequest.content_type =to always fire, resulting in two separate Content-Type-like headers on the wire.This was flagged in the previous review round and is still unresolved.
💡 Suggested fix
(headers || {}).each do |k, v| - request[k.to_s] = CONTENT_TYPE_MAP[v] || v.to_s + request[k.to_s.tr('_', '-')] = CONTENT_TYPE_MAP[v] || v.to_s end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/katello/resources/candlepin/persistent_connection.rb` around lines 78 - 80, Header keys are being sent with underscores (e.g. "content_type") because the code uses k.to_s directly; change the header name normalization so keys are converted to standard HTTP wire form before setting on the Net::HTTP request—e.g. derive header_name = k.to_s.tr('_','-').split('-').map(&:capitalize).join('-') (or otherwise replace underscores with hyphens and use canonical case), then do request[header_name] = CONTENT_TYPE_MAP[v] || v.to_s and use the same canonical "Content-Type" header_name when checking request['Content-Type'] to avoid creating duplicate Content-Type-like headers (affecting the block that later sets request.content_type).
🧹 Nitpick comments (1)
app/lib/katello/resources/candlepin/persistent_connection.rb (1)
50-60: ⚡ Quick win
@persistent_http ||=is not thread-safe for multi-threaded Puma — consider a Mutex guard.This is the primary use-case (high-concurrency registration), and the
||=pattern on a class instance variable is not atomic. Two threads can both observe@persistent_httpasniland both execute the initializer block, leaving oneNet::HTTP::Persistentinstance orphaned. Since the library stores the actual TCP connections inThread.current, the orphaned instance itself doesn't create incorrect behavior, but it does leak open connections until the idle timeout fires.🔒 Suggested fix — Mutex-guarded initialization
+ PERSISTENT_HTTP_MUTEX = Mutex.new + def persistent_http - `@persistent_http` ||= begin - http = Net::HTTP::Persistent.new(name: 'candlepin') - http.idle_timeout = 30 - http.max_requests = 500 - http.ca_file = ssl_ca_file if ssl_ca_file - http.open_timeout = SETTINGS[:katello][:rest_client_timeout] - http.read_timeout = SETTINGS[:katello][:rest_client_timeout] - http - end + return `@persistent_http` if `@persistent_http` + PERSISTENT_HTTP_MUTEX.synchronize do + `@persistent_http` ||= begin + http = Net::HTTP::Persistent.new(name: 'candlepin') + http.idle_timeout = 30 + http.max_requests = 500 + http.ca_file = ssl_ca_file if ssl_ca_file + http.open_timeout = SETTINGS[:katello][:rest_client_timeout] + http.read_timeout = SETTINGS[:katello][:rest_client_timeout] + http + end + end end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/katello/resources/candlepin/persistent_connection.rb` around lines 50 - 60, The lazy initialization of `@persistent_http` in persistent_http is not thread-safe and can create leaked Net::HTTP::Persistent instances under Puma; change it to acquire a Mutex (e.g., define a class- or instance-level mutex like PERSISTENT_HTTP_MUTEX or `@persistent_http_mutex`) around the ||= initialization so only one thread runs the initializer (inside persistent_http) and all others return the same instance; ensure the Mutex is instantiated once and use synchronize to guard creation of `@persistent_http` while leaving subsequent reads lock-free.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@app/lib/katello/resources/candlepin/persistent_connection.rb`:
- Around line 78-80: Header keys are being sent with underscores (e.g.
"content_type") because the code uses k.to_s directly; change the header name
normalization so keys are converted to standard HTTP wire form before setting on
the Net::HTTP request—e.g. derive header_name =
k.to_s.tr('_','-').split('-').map(&:capitalize).join('-') (or otherwise replace
underscores with hyphens and use canonical case), then do request[header_name] =
CONTENT_TYPE_MAP[v] || v.to_s and use the same canonical "Content-Type"
header_name when checking request['Content-Type'] to avoid creating duplicate
Content-Type-like headers (affecting the block that later sets
request.content_type).
In `@app/lib/katello/resources/candlepin/proxy.rb`:
- Around line 11-12: The proxied requests in proxy.rb (methods calling
CandlepinResource.post/put/delete/get that currently merge User.cp_oauth_header
into inline header hashes) must include CandlepinResource.default_headers (to
add accept-language and X-Correlation-ID) and must never mutate the caller's
headers; replace any use of merge! (e.g.
extra_headers.merge!(default_request_headers)) with non-destructive merge and
chain merges so headers = (extra_headers ||
{}).merge(CandlepinResource.default_headers).merge(User.cp_oauth_header).merge(your_content_type_accept)
before calling CandlepinResource.post/put/delete/get, and update all occurrences
referenced in the diff (the get, post, put, delete call sites and any use of
default_request_headers or merge!) accordingly.
In `@test/lib/resources/persistent_connection_test.rb`:
- Around line 124-135: The test test_symbol_header_values_are_translated
currently asserts the wrong header key; update the expectation inside the mock
for persistent_http (the block passed to mock_http.expects(:request)) to check
req['content-type'] == 'application/json' instead of req['content_type'] so it
will validate the hyphenated header name after the header normalization change
in persistent_connection.rb; keep the accept assertion as req['accept'] ==
'application/json' and ensure CandlepinResource.issue_request call (method:
:get, path: '/candlepin/status', headers: {accept: :json, content_type: :json})
remains the same so the test verifies translation from symbol/underscore to
hyphenated header keys.
---
Nitpick comments:
In `@app/lib/katello/resources/candlepin/persistent_connection.rb`:
- Around line 50-60: The lazy initialization of `@persistent_http` in
persistent_http is not thread-safe and can create leaked Net::HTTP::Persistent
instances under Puma; change it to acquire a Mutex (e.g., define a class- or
instance-level mutex like PERSISTENT_HTTP_MUTEX or `@persistent_http_mutex`)
around the ||= initialization so only one thread runs the initializer (inside
persistent_http) and all others return the same instance; ensure the Mutex is
instantiated once and use synchronize to guard creation of `@persistent_http`
while leaving subsequent reads lock-free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e2b9cc0-946e-47dc-9872-373176dc374f
📒 Files selected for processing (5)
app/lib/katello/resources/candlepin.rbapp/lib/katello/resources/candlepin/persistent_connection.rbapp/lib/katello/resources/candlepin/proxy.rbkatello.gemspectest/lib/resources/persistent_connection_test.rb
|
I feel the project should look at replacing rest-client with an actively maintained gem that supports this out of the box. For example, Faraday works well and it's already brought into the project by the pulp client dependencies. Not saying that should happen on this PR but I think the effort would be better spent by migrating the CandlepinResource layer to a more modern HTTP client. |
|
Agreed that migrating CandlepinResource to Faraday would be the right long-term move — rest-client is effectively unmaintained and Faraday is already in the dependency tree via Pulp client. We went with Would you prefer we:
If (2), any pointers on how the Pulp client integration uses Faraday? That would give us a head start on matching the existing pattern. |
|
Personally (not speaking for Katello as a whole) I'd prefer option two since this is already tech debt as it sits. The project has gone a long time without having persistent connections, not to say they shouldn't be a goal. It'd be better to resolve it in tandem or following the move to a more modern HTTP client. Faraday was only an example, although I think it's a good one. The decision would be appropriate for a discourse (community.theforeman.org) thread - I'm sure other devs would have good input on the direction to go, and this would be a good one to have alignment on as a project. Here is another project that did their own migration if you'd like to take a look. |
|
We did a thorough investigation into the migration options. Here's what we found: The Faraday version blocker Katello pins Faraday to Migrating the Candlepin layer to Faraday today would mean migrating to a dead 1.x branch. The gate is resolving the rest-client is also unmaintained — last release was August 2019. The repo itself says "this library is unmaintained". So the Candlepin layer currently sits on two unmaintained libraries. Real-world migration references We looked at several projects that made this migration:
Scope of migration for Katello
Our take We agree this is the right long-term direction. The question is sequencing: the |
2 participants
Problem
Each Candlepin request creates a new TCP+TLS connection via RestClient, then tears it down. Under concurrent registration (500+ hosts), this generates thousands of TIME_WAIT sockets and wastes ~30ms per TLS handshake.
Solution
Add a
Net::HTTP::Persistentconnection pool that maintains one TCP+TLS connection per Puma thread, reused across all Candlepin calls within that thread. Stale connections are detected and re-established automatically.Key changes
persistent_connection.rb— NewPersistentConnectionmodule withPooledResponsewrapper preserving theRestClient::Responsecontract (inherits from String for MultiJson compatibility)candlepin.rb— IncludePersistentConnectioninCandlepinResource; disable forUpstreamCandlepinResource(different server)proxy.rb— Route Proxy calls throughCandlepinResource.get/post/put/deleteinstead ofrest_clientso they use the poolkatello.gemspec— Addnet-http-persistent >= 4.0dependencyDesign
CONTENT_TYPE_MAPtranslates RestClient symbol headers (:json→application/json) that Net::HTTP would pass literallyPooledResponseinherits fromStringsoMultiJson.load(exception.response)works inCandlepinError.from_exceptionuse_persistent_connectionclass attribute allows subclasses to opt outPackaging
Requires
rubygem-net-http-persistentin foreman-packaging. Its only runtime dependency (connection_pool) is already packaged. A Fedora spec exists atsrc.fedoraproject.org/rpms/rubygem-net-http-persistent.How to test
bundle exec rake test TEST=test/lib/resources/persistent_connection_test.rbss -tn state time-wait '( dport = 23443 )'Summary by CodeRabbit
New Features
Refactor
Tests