Skip to content

fix: respect custom query timeout in ping() method#246

Open
abodnar wants to merge 3 commits intosmi2:masterfrom
abodnar:fix/ping-timeout
Open

fix: respect custom query timeout in ping() method#246
abodnar wants to merge 3 commits intosmi2:masterfrom
abodnar:fix/ping-timeout

Conversation

@abodnar
Copy link

@abodnar abodnar commented Mar 5, 2026

Problem

ping() creates a CurlerRequest and only calls .connectTimeOut() on it, never .timeOut(). This means CURLOPT_TIMEOUTalways falls back to the hard-coded default of10seconds inCurlerRequest, silently ignoring any value set via setTimeout()`.

Fix

Append .timeOut($this->settings()->getTimeOut()) to the request chain in ping() (src/Transport/Http.php), matching the pattern already used in newRequest():

// newRequest() — lines 274–275
$new->timeOut($this->settings()->getTimeOut());                
$new->connectTimeOut($this->_connectTimeOut);
                 
// ping() — before                                                                                       
$request->url(...)->verbose(false)->GET()->connectTimeOut(...);

// ping() — after
$request->url(...)->verbose(false)->GET()->timeOut($this->settings()->getTimeOut())->connectTimeOut(...);

Test

tests/PingTimeoutTest::testQueryTimeoutIsRespectedByPing spins up a local TCP server that completes the handshake but never sends an HTTP response. With setTimeout(1) and a high setConnectTimeOut(5), the test asserts the operation times out in ~1 second — isolating CURLOPT_TIMEOUT from the connect timeout and confirming the fix works without requiring a live ClickHouse server.

Co-Authored with Claude Code

Adam Bodnar added 3 commits March 5, 2026 11:56
ping() was only setting CURLOPT_CONNECTTIMEOUT via connectTimeOut() but
never called timeOut(), so CURLOPT_TIMEOUT always fell back to the
hard-coded CurlerRequest default of 10 seconds, ignoring any value set
via setTimeout(). Add .timeOut($this->settings()->getTimeOut()) to the
request chain, matching the pattern already used in newRequest().

Co-Authored-By: Claude
Uses a local TCP server that completes the handshake but never sends an
HTTP response, isolating CURLOPT_TIMEOUT from CURLOPT_CONNECTTIMEOUT so
the assertion targets exactly the bug fixed in ping().

Co-Authored-By: Claude
ClientTest.setUp() requires a live ClickHouse connection, so the timeout
test (which only needs a local socket server) is extracted into its own
class to run without any external dependencies.

Co-Authored-By: Claude
@abodnar
Copy link
Author

abodnar commented Mar 5, 2026

The Scrutinizer issue seems outside the scope of what I've changed in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant