Conversation
Expose withReadTimeout() and withWriteTimeout() on DatabaseClientBuilder so callers can configure per-client OkHttp read and write timeouts without relying on global defaults.
There was a problem hiding this comment.
Pull request overview
Adds configurable OkHttp read/write timeouts to the MarkLogic Java client, exposing them via DatabaseClientBuilder, Spring-style property sources, and DatabaseClientFactory, and wiring the values into OkHttpServices while preserving the existing default “no timeout” behavior.
Changes:
- Added fluent builder methods and bean properties for
readTimeoutMillis/writeTimeoutMillis(default 0). - Plumbed timeouts through
DatabaseClientFactoryandDatabaseClientPropertySourceintoOkHttpServices, applying them on the OkHttp client builder when > 0. - Added/updated unit tests verifying defaults, configuration round-tripping, property parsing, and an end-to-end read-timeout firing scenario via MockWebServer.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| marklogic-client-api/src/main/java/com/marklogic/client/DatabaseClientBuilder.java | Adds withReadTimeout/withWriteTimeout builder APIs that store values in millis. |
| marklogic-client-api/src/main/java/com/marklogic/client/DatabaseClientFactory.java | Adds new timeout-aware newClient overload and timeout fields/accessors on Bean. |
| marklogic-client-api/src/main/java/com/marklogic/client/impl/DatabaseClientPropertySource.java | Adds property handlers for timeout millis and passes them through to the timeout-aware newClient overload. |
| marklogic-client-api/src/main/java/com/marklogic/client/impl/OkHttpServices.java | Extends ConnectionConfig with read/write timeouts and applies them to the OkHttp client builder. |
| marklogic-client-api/src/test/java/com/marklogic/client/test/DatabaseClientBuilderTest.java | Adds assertions for default (0) timeouts and configured millis values. |
| marklogic-client-api/src/test/java/com/marklogic/client/impl/DatabaseClientPropertySourceTest.java | Adds tests for timeout properties from Long/String/Integer sources. |
| marklogic-client-api/src/test/java/com/marklogic/client/impl/okhttp/OkHttpTimeoutTest.java | New tests validating wiring to OkHttp and a behavioral read-timeout firing case. |
| public record ConnectionConfig(String host, int port, String basePath, String database, | ||
| SecurityContext securityContext, List<OkHttpClientConfigurator> clientConfigurators) { | ||
| SecurityContext securityContext, List<OkHttpClientConfigurator> clientConfigurators, | ||
| long readTimeoutMillis, long writeTimeoutMillis) { | ||
| } |
| if (config.readTimeoutMillis() > 0) { | ||
| clientBuilder.readTimeout(config.readTimeoutMillis(), TimeUnit.MILLISECONDS); | ||
| } | ||
| if (config.writeTimeoutMillis() > 0) { | ||
| clientBuilder.writeTimeout(config.writeTimeoutMillis(), TimeUnit.MILLISECONDS); | ||
| } |
| /** | ||
| * Verifies that a configured read timeout actually fires when the server stalls before sending | ||
| * any response. This is an end-to-end behavioral test: if the timeout were not wired to the | ||
| * OkHttpClient, the call would block indefinitely rather than throwing a SocketTimeoutException. | ||
| */ |
rjrudin
left a comment
rjrudin
left a comment
There was a problem hiding this comment.
I recommend against doing this. A user can already configure these timeouts via DatabaseClientFactory.addConfigurator(new OkHttpClientConfigurator(...)).
So the problem identified by MLE-30245 - that the default timeouts of zero can possibly be exploited - can already be solved.
The ticket would thus need to justify the addition to the public API for the above solution. I don't think that justification exists. The main reason is that the OkHttp Builder has dozens of methods for configuring an OkHttp connection; the read and write timeouts are just two of them. There are two other timeouts as well - the connect timeout and the call timeout.
Adding these probably-very-rarely-used timeouts to public APIs creates a pattern that doesn't scale well - i.e. just how many OkHttp-specific parameters are we going to add to the public API, when a user already has a way of configuring these parameters?
|
@rjrudin It looks like configuring it with a OkHttpClientConfigurator for the DatabaseClientFactory sets the values globally. How does the scenario work when you have separate clients that you want to have different timeout values? |
Problem
Previously, all HTTP connections made by the Java client used OkHttp's default behavior of no read or write timeout. Long-running requests could therefore block indefinitely, exhausting thread pools in multi-tenant or production applications with no mechanism to recover without a JVM restart.
Solution
Two new fluent methods on
DatabaseClientBuilderallow callers to set read and write timeouts with anyTimeUnit:withReadTimeout(long value, TimeUnit unit)withWriteTimeout(long value, TimeUnit unit)Both methods validate that the value is non-negative and that the unit is non-null, then store the timeout in milliseconds. A value of zero preserves the existing no-timeout behavior. The timeouts flow through to OkHttp only when positive, leaving the existing default unchanged.
For Spring-based applications, the same timeouts can be configured via property sources using:
marklogic.client.readTimeoutMillismarklogic.client.writeTimeoutMillisChanges
DatabaseClientBuilderwithReadTimeoutandwithWriteTimeoutmethods with input validation and@since 8.2.0JavadocDatabaseClientFactoryreadTimeoutMillis/writeTimeoutMillisfields and accessors toBean; added 8-argnewClientoverload; 6-arg overload now delegates to 8-arg with zerosDatabaseClientPropertySourcereadTimeoutMillisandwriteTimeoutMillisproperties;newClient()passes timeout values through the 8-arg overload to preserve the global handle registryOkHttpServices.ConnectionConfigreadTimeoutMillisandwriteTimeoutMillisto the recordOkHttpServices.connect()OkHttpTimeoutTestSocketTimeoutExceptionagainst a stalling MockWebServerDatabaseClientBuilderTestDatabaseClientPropertySourceTestTesting
OkHttpTimeoutTest.readTimeoutFiresWhenServerStallsuses a 100 ms client timeout against a 5-second server delay to verify that aSocketTimeoutExceptionis actually thrown, not just that a timeout value is storedBackward Compatibility
No breaking changes. All existing callers that do not configure timeouts continue to use OkHttp's no-timeout default (value of zero). The new 8-arg
newClientoverload is additive; the existing 6-arg overload delegates to it with zeros.Jira Link: https://progresssoftware.atlassian.net/browse/MLE-30245