Fix HTTP::Client reconnection errors and CI coverage build#1
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces per-call UNIX-socket HTTP requests with a shared HTTPClient connection pool in Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant DocrClient as Docr::Client
participant Pool as HTTPClient::Client
participant Docker as Docker daemon (unix:///var/run/docker.sock)
App->>DocrClient: call(request)
DocrClient->>Pool: pool.checkout { client -> execute request }
Pool->>Docker: HTTP over UNIX socket (via checked-out client)
Docker-->>Pool: HTTP response
Pool-->>DocrClient: yield response
DocrClient->>DocrClient: drain body, parse status / handle errors
DocrClient-->>App: return result or raise DockerAPIError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/testcontainers/patches.cr (1)
50-64: Consider explicitly closing socket and client for proper resource cleanup.The socket and HTTP::Client created on lines 51-52 are never explicitly closed. While Crystal's GC will eventually clean them up, explicit cleanup is better practice to prevent potential file descriptor exhaustion under heavy test load.
♻️ Suggested improvement with explicit cleanup
def call(method : String, url : String | URI, headers : HTTP::Headers | Nil = nil, body : IO | Slice(UInt8) | String | Nil = nil, &) socket = UNIXSocket.new("/var/run/docker.sock") client = HTTP::Client.new(socket) + begin client.exec(method, url, headers, body) do |response| unless response.success? body_text = response.body_io?.try(&.gets_to_end) || "{\"message\": \"No response body\"}" error = Docr::Types::ErrorResponse.from_json(body_text) raise Docr::Errors::DockerAPIError.new(error.message, response.status_code) end yield response ensure response.try(&.body_io?.try(&.gets_to_end)) end + ensure + client.close + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/testcontainers/patches.cr` around lines 50 - 64, The call method opens a UNIXSocket and an HTTP::Client but never explicitly closes them, risking FD leaks; update call (where UNIXSocket.new and HTTP::Client.new are created and client.exec is used) to ensure both the HTTP client and the underlying socket are closed in the ensure block (after reading response.body_io and raising or yielding), e.g. call client.close (or client.finish) and socket.close in the ensure so resources are always released even on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/testcontainers/patches.cr`:
- Around line 50-64: The call method opens a UNIXSocket and an HTTP::Client but
never explicitly closes them, risking FD leaks; update call (where
UNIXSocket.new and HTTP::Client.new are created and client.exec is used) to
ensure both the HTTP client and the underlying socket are closed in the ensure
block (after reading response.body_io and raising or yielding), e.g. call
client.close (or client.finish) and socket.close in the ensure so resources are
always released even on error.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/testcontainers/patches.cr`:
- Around line 55-66: Wrap the call to self.class.pool.checkout { |client|
client.exec(...) { |response| ... } } so that any exception raised by
client.exec is rescued, the client is closed (call client.close) before
re-raising to avoid returning a broken client to the pool, and change any uses
of response.body_io?.gets_to_end to response.body_io?.skip_to_end to discard the
remaining body without allocating memory; update the ensure block to call
response.try(&.body_io?.try(&.skip_to_end)) and add a rescue around client.exec
that closes the client and re-raises the original error.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
shard.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
shard.ymlsrc/testcontainers/patches.cr
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/testcontainers/patches.cr (1)
59-61: Harden non-success error parsing against malformed responses.Line 60 calls
from_jsonwithout error handling. If the Docker API returns a non-JSON response body (or a response that doesn't match theErrorResponseschema),JSON::ParseExceptionwill be raised and bypass your custom error mapping. Wrap the parsing in a begin-rescue to fall back to the raw body text:Suggested change
unless response.success? body_text = response.body_io?.try(&.gets_to_end) || "{\"message\": \"No response body\"}" - error = Docr::Types::ErrorResponse.from_json(body_text) - raise Docr::Errors::DockerAPIError.new(error.message, response.status_code) + message = begin + Docr::Types::ErrorResponse.from_json(body_text).message + rescue JSON::ParseException + body_text + end + raise Docr::Errors::DockerAPIError.new(message, response.status_code) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/testcontainers/patches.cr` around lines 59 - 61, The error parsing in the non-success block needs to be hardened: wrap the call to Docr::Types::ErrorResponse.from_json(body_text) in a begin-rescue that catches JSON::ParseException (and/or StandardError) and falls back to using the raw body_text for the error message; then raise Docr::Errors::DockerAPIError.new with either the parsed error.message or the raw body_text along with response.status_code so malformed or non-JSON responses still produce a meaningful DockerAPIError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/testcontainers/patches.cr`:
- Around line 55-70: The rescue currently catches all exceptions and closes the
pooled client (evicting healthy connections) — restrict that to
transport/connection failures only by only rescuing transport-related exceptions
(e.g., Errno::*, EOFError, IOError, SocketError, etc.) around the client.exec
block; keep raising any other exceptions (including Docr::Errors::DockerAPIError
and exceptions from yield response) so they don't close the client. Concretely,
replace the generic "rescue ex" with either an explicit "rescue e : Errno::*,
EOFError, IOError, SocketError" (or equivalent transport exception list) to call
client.close and re-raise, or add a conditional inside the rescue that only
calls client.close when ex is a transport error, and otherwise re-raise
immediately; refer to the self.class.pool.checkout block, client.exec, the raise
of Docr::Errors::DockerAPIError and the yield response to locate the change.
---
Nitpick comments:
In `@src/testcontainers/patches.cr`:
- Around line 59-61: The error parsing in the non-success block needs to be
hardened: wrap the call to Docr::Types::ErrorResponse.from_json(body_text) in a
begin-rescue that catches JSON::ParseException (and/or StandardError) and falls
back to using the raw body_text for the error message; then raise
Docr::Errors::DockerAPIError.new with either the parsed error.message or the raw
body_text along with response.status_code so malformed or non-JSON responses
still produce a meaningful DockerAPIError.
src/testcontainers/patches.cr
Outdated
| self.class.pool.checkout do |client| | ||
| begin | ||
| client.exec(method, url, headers, body) do |response| | ||
| unless response.success? | ||
| body_text = response.body_io?.try(&.gets_to_end) || "{\"message\": \"No response body\"}" | ||
| error = Docr::Types::ErrorResponse.from_json(body_text) | ||
| raise Docr::Errors::DockerAPIError.new(error.message, response.status_code) | ||
| end | ||
|
|
||
| client.exec(method, url, headers, body) do |response| | ||
| unless response.success? | ||
| body_text = response.body_io?.try(&.gets_to_end) || "{\"message\": \"No response body\"}" | ||
| error = Docr::Types::ErrorResponse.from_json(body_text) | ||
| raise Docr::Errors::DockerAPIError.new(error.message, response.status_code) | ||
| yield response | ||
| ensure | ||
| response.try(&.body_io?.try(&.skip_to_end)) | ||
| end | ||
| rescue ex | ||
| client.close | ||
| raise ex |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the rescue scope in the pooled client call
rg -n -C4 'def call\(|pool\.checkout|rescue ex|client\.close|raise' src/testcontainers/patches.crRepository: dragosv/testcontainers-crystal
Length of output: 1018
Scope the rescue to transport failures only.
Line 68 currently rescues all exceptions and closes the pooled client, including non-transport exceptions like DockerAPIError (raised on line 61 after successful connection) and exceptions from yield response (line 64). This evicts healthy connections from the pool and degrades reuse under normal error paths.
Suggested change
- rescue ex
+ rescue ex : IO::Error
client.close
- raise ex
+ raise
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/testcontainers/patches.cr` around lines 55 - 70, The rescue currently
catches all exceptions and closes the pooled client (evicting healthy
connections) — restrict that to transport/connection failures only by only
rescuing transport-related exceptions (e.g., Errno::*, EOFError, IOError,
SocketError, etc.) around the client.exec block; keep raising any other
exceptions (including Docr::Errors::DockerAPIError and exceptions from yield
response) so they don't close the client. Concretely, replace the generic
"rescue ex" with either an explicit "rescue e : Errno::*, EOFError, IOError,
SocketError" (or equivalent transport exception list) to call client.close and
re-raise, or add a conditional inside the rescue that only calls client.close
when ex is a transport error, and otherwise re-raise immediately; refer to the
self.class.pool.checkout block, client.exec, the raise of
Docr::Errors::DockerAPIError and the yield response to locate the change.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/testcontainers/patches.cr (1)
78-81:⚠️ Potential issue | 🟠 MajorRestrict pooled-client eviction to transport errors.
At Line 78, the generic
rescue excloses the client at Line 79 for all exceptions, includingDocr::Errors::DockerAPIErrorand exceptions fromyield response. That evicts healthy sockets and reduces pool effectiveness.🔧 Suggested fix
- rescue ex + rescue ex : IO::Error + client.close + raise + rescue ex : Socket::Error client.close - raise ex + raise + rescue ex + # Keep non-transport failures from evicting healthy pooled clients. + raise end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/testcontainers/patches.cr` around lines 78 - 81, The current generic rescue (rescue ex) closes the pooled `client` for all exceptions which evicts healthy sockets; change it to only close the client for transport-level errors. Replace the generic rescue block around the `client` usage with a targeted rescue that matches transport/IO errors (e.g. Docr::Errors::DockerTransportError, SocketError, EOFError, Errno::EPIPE, IOError => ex) and call `client.close` and `raise ex` only for those; let other exceptions (including Docr::Errors::DockerAPIError or errors raised by `yield response`) propagate without calling `client.close` so the socket remains in the pool. Ensure you update the rescue clause that currently reads `rescue ex` and reference the `client` variable in that block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/testcontainers/patches.cr`:
- Line 85: The rescue currently checks ex.message for "This HTTP::Client cannot
be reconnected" which is fragile; change the rescue to explicitly catch
RuntimeError (e.g., rescue ex : RuntimeError) and handle it alongside IO::Error
and Socket::Error, and update the conditional that uses ex to rely on the
exception type rather than string matching; also prefer creating HTTP::Client
via host/URI (HTTP::Client.new("host") or HTTP::Client.new(URI.parse(...))) in
the code paths that instantiate clients so reconnects are handled automatically
instead of relying on reconnect logic in the retry block that references ex and
HTTP::Client.
---
Duplicate comments:
In `@src/testcontainers/patches.cr`:
- Around line 78-81: The current generic rescue (rescue ex) closes the pooled
`client` for all exceptions which evicts healthy sockets; change it to only
close the client for transport-level errors. Replace the generic rescue block
around the `client` usage with a targeted rescue that matches transport/IO
errors (e.g. Docr::Errors::DockerTransportError, SocketError, EOFError,
Errno::EPIPE, IOError => ex) and call `client.close` and `raise ex` only for
those; let other exceptions (including Docr::Errors::DockerAPIError or errors
raised by `yield response`) propagate without calling `client.close` so the
socket remains in the pool. Ensure you update the rescue clause that currently
reads `rescue ex` and reference the `client` variable in that block.
src/testcontainers/patches.cr
Outdated
| end | ||
| rescue ex | ||
| # If the pool returns a broken connection or the connection drops during checkout, retry up to 3 times | ||
| if (ex.is_a?(IO::Error) || ex.is_a?(Socket::Error) || ex.message == "This HTTP::Client cannot be reconnected") && retry_count < 3 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file to understand the context
cat -n src/testcontainers/patches.cr | head -100Repository: dragosv/testcontainers-crystal
Length of output: 1807
🏁 Script executed:
# Check for shard.yml to see Crystal version and dependencies
cat shard.ymlRepository: dragosv/testcontainers-crystal
Length of output: 1807
🏁 Script executed:
# Search for references to the error message to understand the context better
rg "This HTTP::Client cannot be reconnected" --type crystal -B 2 -A 2Repository: dragosv/testcontainers-crystal
Length of output: 1807
🌐 Web query:
Crystal HTTP::Client "This HTTP::Client cannot be reconnected" exception
💡 Result:
That exception is raised when an HTTP::Client instance was created bound to an existing IO (e.g. HTTP::Client.new(io, ...)). Those clients have @reconnect = false, so once the underlying connection is gone (@io becomes nil after close or a dropped keep-alive), the next request tries to “reconnect” and Crystal raises:
- “Instances created with this constructor cannot be reconnected…” [1]
raise "This HTTP::Client cannot be reconnected"when@iois nil and@reconnectis false [2]
Fixes
-
Don’t construct the client from an
IOunless you truly need to. Prefer:client = HTTP::Client.new("example.com", tls: true) # or HTTP::Client.new(URI.parse(...))
These clients can open a new TCP/TLS connection when needed.
-
If you must use
HTTP::Client.new(io, ...)(custom socket/proxy/tunnel), then you must recreate the client with a freshIOwhenever the connection is closed/dropped (you can’t reuse the sameHTTP::Client). -
If the drop happens “randomly” between requests, it’s often the server closing an idle keep-alive connection; in that case either create a new client per request, or add retry logic that rebuilds the IO + client on failure.
Sources: [1] [2]
🌐 Web query:
Crystal HTTP::Client exception classes reconnect error
💡 Result:
In Crystal’s stdlib, there isn’t a dedicated HTTP::Client::ReconnectError exception.
What gets raised for “cannot be reconnected”
If you built the client from an existing IO (HTTP::Client.new(io, ...)), it disables reconnects (@reconnect = false). On the next request (after close, or when keep-alive isn’t possible), Crystal raises a RuntimeError with message:
"This HTTP::Client cannot be reconnected"(raised viaraise "...", i.e.RuntimeError). [1]
What to rescue for real reconnect/network failures
HTTP::Client’s internal request execution rescues IO::Error and will retry once by closing the socket and issuing the request again (intended for cases where the server closed a keep-alive connection). [2]
So, in practice you’ll commonly want to rescue:
IO::Error(includes timeouts likeIO::TimeoutError) [2]Socket::Error/Socket::ConnectError(these are in the socket error hierarchy underIO::Error) [3]
Typical rescue pattern
begin
client.get("/")
rescue ex : IO::Error
# network / read-write / timeout / connect-related failures
rescue ex : RuntimeError
# includes: "This HTTP::Client cannot be reconnected"
endSources:
[1] Crystal stdlib HTTP::Client source: raises “This HTTP::Client cannot be reconnected” when @reconnect is false. (github.com)
[2] Crystal stdlib HTTP::Client source: rescues IO::Error, closes, and retries once. (github.com)
[3] Crystal 0.34.0 release notes: socket exceptions (Socket::Error, Socket::ConnectError) are under IO::Error. (crystal-lang.org)
Citations:
- 1: https://github.com/crystal-lang/crystal/blob/e9eff00/src/http/client.cr
- 2: https://github.com/crystal-lang/crystal/blob/e9eff00/src/http/client.cr
- 3: https://crystal-lang.org/2020/04/06/crystal-0.34.0-released/?utm_source=openai
🌐 Web query:
Crystal language HTTP::Client library exception hierarchy
💡 Result:
Crystal’s stdlib HTTP::Client does not define its own dedicated exception hierarchy (there is no HTTP::Client::Error base class in the stdlib API); instead it raises/propagates a few existing exception families:
ExceptionArgumentError— invalid host input / invalid TLS+scheme combinations (e.g. missing/unsupported scheme, TLS context given for anhttpURI, host string isn’t “host-only”). [1][2]URI::Error— may bubble up fromURI.parsein URL-based helpers (not always converted toArgumentError). [2]IO::Error(network I/O problems;HTTP::Clientexplicitly rescues/reenacts these in request execution) [3]IO::TimeoutError— connect/read/write timeouts. [4]IO::EOFError— e.g. “Unexpected end of http response”. [5]Socket::Error(and subclasses likeSocket::ConnectError) — connection-level failures fromTCPSocket(Crystal’s socket errors are underIO::Error). [6][7]
- OpenSSL/TLS exceptions (e.g.
OpenSSL::Error) — TLS handshake/socket creation failures are re-raised fromOpenSSL::SSL::Socket::Client.new. [7]
Sources:
[1] https://github.com/crystal-lang/crystal/blob/e9eff00/src/http/client.cr#L2362-L2384
[2] https://github.com/crystal-lang/crystal/blob/e9eff00/src/http/client.cr#L3531-L3584
[3] https://github.com/crystal-lang/crystal/blob/e9eff00/src/http/client.cr#L3129-L3137
[4] https://crystal-lang.org/api/master/HTTP/Client.html#connect_timeout%3D%28connect_timeout%3A%20Number%29
[5] https://github.com/crystal-lang/crystal/blob/e9eff00/src/http/client.cr#L3229-L3254
[6] https://crystal-lang.org/2020/04/06/crystal-0.34.0-released/
[7] https://github.com/crystal-lang/crystal/blob/e9eff00/src/http/client.cr#L3469-L3507
Rescue a typed exception rather than matching error message text.
Line 85 checks ex.message == "This HTTP::Client cannot be reconnected", which is fragile. In Crystal's stdlib, this error is raised as a RuntimeError—there is no dedicated exception class. Instead of matching the message string, rescue RuntimeError explicitly:
rescue ex : RuntimeError
# Handle "This HTTP::Client cannot be reconnected" and other runtime errorsBetter still, avoid the underlying issue entirely: this error only occurs when HTTP::Client is created with an existing IO object (HTTP::Client.new(io, ...)). Prefer constructing clients by host/URI (HTTP::Client.new("host.com") or HTTP::Client.new(URI.parse(...))), which can reconnect automatically on keep-alive failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/testcontainers/patches.cr` at line 85, The rescue currently checks
ex.message for "This HTTP::Client cannot be reconnected" which is fragile;
change the rescue to explicitly catch RuntimeError (e.g., rescue ex :
RuntimeError) and handle it alongside IO::Error and Socket::Error, and update
the conditional that uses ex to rely on the exception type rather than string
matching; also prefer creating HTTP::Client via host/URI
(HTTP::Client.new("host") or HTTP::Client.new(URI.parse(...))) in the code paths
that instantiate clients so reconnects are handled automatically instead of
relying on reconnect logic in the retry block that references ex and
HTTP::Client.
…ion type over RuntimeError
dragosv
changed the title
1 participant
Description
This PR addresses two main issues causing failures in the CI pipeline:
Flaky Docker integration tests failing with This HTTP::Client cannot be reconnected (Exception).
The coverage job throwing an ld linker error due to a missing bin directory.
Type of Change
Summary by CodeRabbit
Bug Fixes
Chores