Repository navigation
[jdbc-v2] Keep the configured socket_timeout after a pool restores the network timeout - #3266
Open
polyglotAI-bot wants to merge 5 commits into
Open
polyglotAI-bot wants to merge 5 commits into
polyglotAI-bot wants to merge 5 commits into
Conversation
Add a delayed-response test for the JDBC timeout save/set/restore sequence used by pools. The test intentionally fails on upstream because restoring the reported zero timeout disables the configured socket timeout. Keep the reproduction separate from any proposed fix. Refs #3260 Co-authored-by: codex (0.153.2)
Connection#getNetworkTimeout() returned 0 when socket_timeout was set in the client configuration and setNetworkTimeout was not called. A pool that saved this value and restored it after validation stored a socket_timeout=0 override in the connection settings, and that override disabled the configured timeout for every later request. getNetworkTimeout() now returns the connection override if one is set, else the socket_timeout value of the client configuration. setNetworkTimeout(executor, 0) still disables the socket timeout. Fixes: #3260 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Stathis Souris seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…imeout SonarCloud reported a possible NullPointerException (java:S2259) on the fallback path of ConnectionImpl#getNetworkTimeout(): parseValue() can return null, and the result was unboxed without a check. Read the value the same way as StatementImpl reads async: getOrDefault() with the property default, then Integer.parseInt(). The returned value does not change: socket_timeout is an Integer property, and parseValue() uses Integer.parseInt() for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
3 tasks done
|
Hi @chernser @mzitnik. Apologies in advance. Initially I opened this issue and this test-only PR. I signed Contributor License Agreement, but I'm not 100% sure this is resolved yet or not. Thanks in advance. |
4 tasks done
chernser
reviewed
Nov 4, 2026
chernser
reviewed
Nov 4, 2026
chernser
requested changes
Nov 4, 2026
chernser
left a comment
chernser
left a comment
Contributor
There was a problem hiding this comment.
tests should be moved and other properties should be checked
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #3260. Based on #3262 by @ssouris-te: this PR contains their commit
4d970c2(the reproduction test) without changes, plus the fix and more tests.Closes #3260
Root cause
ConnectionImpl#getNetworkTimeout()read only the connection'sQuerySettings. Whensocket_timeoutwas set in the client configuration andsetNetworkTimeoutwas not called, the method returned0(no limit), but requests used the configured value. A connection pool (for example HikariCP) savesgetNetworkTimeout(), sets a validation timeout, and then restores the saved value. The restore calledsetNetworkTimeout(executor, 0), which putsocket_timeout=0into the connection settings.Clientmerges operation settings over the client configuration, so this override disabled the configured timeout for all later requests.Fix
getNetworkTimeout()returns the connection value ifsetNetworkTimeoutset one. Otherwise it returns thesocket_timeoutvalue of the client configuration, parsed withClientConfigProperties.SOCKET_OPERATION_TIMEOUT.parseValue(the same parse the client applies). The save/set/restore cycle now restores the configured value.setNetworkTimeoutis not changed: an explicit0still disables the socket timeout, as the JDBC specification requires.Changes
jdbc-v2ConnectionImpl#getNetworkTimeout(): fall back to the clientsocket_timeoutwhen no connection value is set.ConnectionNetworkTimeoutTest: kept the contributor testtestRestoringNetworkTimeoutPreservesConfiguredSocketTimeoutwithout changes. Added:testGetNetworkTimeoutReportsEffectiveSocketTimeout(@DataProvider:socket_timeout=500/0/ not set): checks the initial value, the explicit override, and the value after restore.testZeroNetworkTimeoutDisablesConfiguredSocketTimeout: withsocket_timeout=500,setNetworkTimeout(executor, 0)returns0, and a response delayed by 1800 ms is read without a timeout. This keeps the explicit-zero behavior.docs/features.md: "Connection lifecycle" now describes whatgetNetworkTimeout()returns.history/latest/3260.md: changelog entry.Compatibility
getNetworkTimeout()now returns the configuredsocket_timeout(not0) untilsetNetworkTimeoutis called. Whensocket_timeoutis not set, it still returns0.socket_timeoutnow applies after validation. A request that takes longer fails with a socket timeout. Because the pool passed an executor tosetNetworkTimeout, the connection is then closed (this is the existingonNetworkTimeoutbehavior). Before this fix, such requests had no limit.setNetworkTimeoutdoes not change.Test
testRestoringNetworkTimeoutPreservesConfiguredSocketTimeoutfails ("Expected SQLException to be thrown, but nothing was thrown") andtestGetNetworkTimeoutReportsEffectiveSocketTimeout[500]fails (expected [500] but found [0]). The contrast rows pass.ConnectionNetworkTimeoutTest5/5 pass, and thejdbc-v2unit suite passes (1968 tests).ConnectionTestintegration tests against ClickHouse 26.9: the timeout tests (testNetworkTimeoutCausesConnectionClosure,testHandlingTimeoutWithoutTimeoutIsSet) pass. The 3 SSL tests fail in our local environment only, because the test HTTPS port points to a plain HTTP port. That failure also occurs without this change.Checklist
history/latest/3260.md)Pre-PR validation (docs/changes_checklist.md, docs/ai-review.md)
mainevery time)docs/features.mdand the history entryFollow-up (not in this PR)
The review found that writer inserts (
WriterStatementImpl), the table-schema lookup inprepareStatement, and statements created beforesetNetworkTimeoutdo not use the connection network timeout. This is not changed by this PR. We will track it separately.🤖 Generated with Claude Code
Note
Medium Risk
Changes timeout semantics for pooled JDBC connections after validation (configured limits now apply again) and adds a new public Client API that mutates shared client configuration at runtime.
Overview
Fixes connection-pool validation breaking configured
socket_timeout:Connection#getNetworkTimeout()used to report0when only the client had a timeout set, so save/restore calledsetNetworkTimeout(..., 0)and disabled enforcement via merged query settings.client-v2addsClient#setNetworkTimeout,getNetworkTimeout, and aChronoUnitoverload that update/read livesocket_timeouton the client config (with validation).jdbc-v2ConnectionImpldelegates JDBC network timeout get/set to those methods instead ofdefaultQuerySettings, so restore reapplies the configured limit and existing statements pick up changes.Docs and a changelog entry describe runtime client timeout and JDBC behavior. WireMock tests cover client runtime timeouts, JDBC get/restore, and validation.
Reviewed by Cursor Bugbot for commit 4074927. Bugbot is set up for automated code reviews on this repo. Configure here.