Skip to content

feat: add per-phase timeout value type to sdk-core#150

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
feat/per-phase-timeout-model
Open

feat: add per-phase timeout value type to sdk-core#150
OmarAlJarrah wants to merge 1 commit into
mainfrom
feat/per-phase-timeout-model

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

Adds a per-phase Timeout value type to sdk-core that splits a single call's I/O budget into four phases a transport can bound independently:

  • connectTimeout — connection establishment (and TLS handshake)
  • readTimeout — waiting for response bytes
  • writeTimeout — flushing request bytes
  • requestTimeout — the end-to-end ceiling for the whole call

The read and write phases inherit requestTimeout when left unset, so a caller who only knows "this call must finish within N seconds" can set the request budget alone and have read/write follow. connectTimeout is intentionally separate and does not inherit. Every phase uses Duration.ZERO to mean "no limit", matching the convention of the clients this maps onto (OkHttp, java.net.http). The builder rejects negative or unrepresentable durations.

The type is a hand-written, immutable value type following the existing SDK conventions (private constructor + Builder implementing Builder<T> + prefilled newBuilder(), @JvmStatic/@JvmOverloads for Java callers). effectiveReadTimeout / effectiveWriteTimeout resolve the inheritance so transport adapters can read the value a phase should actually enforce without re-implementing the fallback.

Independent of the retry total-timeout

A Timeout bounds a single attempt's I/O phases; the retry total-timeout (RetrySettings.totalTimeout) bounds the sum of all attempts plus backoff. Neither derives from the other, and the KDoc documents the separation. Transports translate the resolved phases to native settings — that wiring is out of scope here.

Configuration surface

Exposed through Configuration with four well-known keys (CONNECT_TIMEOUT, READ_TIMEOUT, WRITE_TIMEOUT, REQUEST_TIMEOUT) and a getTimeout() accessor that resolves a Timeout from those keys using the existing duration grammar (ISO-8601 + shorthand). Consistent with the other typed accessors, a missing or malformed value never throws — it behaves as if the key were absent and falls back to the supplied default / inheritance.

Tests

  • TimeoutTest — defaults/NONE, read/write inheritance, explicit pinning (including pinning a phase to zero so it does not inherit), connect-does-not-inherit, all-phases-set, non-negative + representability validation, newBuilder round-trip preserving explicit flags, and equals/hashCode/toString.
  • ConfigurationTimeoutTest — key-to-phase mapping, request-driven inheritance, malformed-value tolerance, default fill, and env/sysprop layering.

Gated build (scoped, --no-daemon)

./gradlew :sdk-core:test :sdk-core:ktlintCheck :sdk-core:detekt :sdk-core:apiCheck --no-daemon

Result: BUILD SUCCESSFUL. :sdk-core:apiDump was run and the regenerated sdk-core/api/sdk-core.api is committed.

Closes #41

Introduce an immutable Timeout value type splitting a call's I/O budget into
connect, read, write, and request phases. The read and write phases inherit the
request timeout when left unset, so a caller can express "finish within N
seconds" with a single value while still being able to pin individual phases.
Connect establishment is kept separate and does not inherit. Every phase uses
Duration.ZERO to mean "no limit", matching the underlying HTTP clients this maps
onto. The builder validates durations are non-negative and nanosecond-
representable.

This is a core value type only; transports translate the resolved phases onto
their native settings. It is deliberately independent of the retry total-timeout
budget — a Timeout bounds a single attempt's phases, while the retry budget
bounds the sum of all attempts plus backoff, and the two compose.

Wire it into the configuration surface with well-known keys (CONNECT_TIMEOUT,
READ_TIMEOUT, WRITE_TIMEOUT, REQUEST_TIMEOUT) and a Configuration.getTimeout()
accessor that resolves a Timeout from those keys, preserving the value type's
inheritance and never throwing on a malformed value.

Closes #41
@OmarAlJarrah

Copy link
Copy Markdown
Member Author

This adds a standalone, immutable per-phase Timeout value type to the config package (connect/read/write/request, with read/write inheriting the request timeout unless pinned), the four well-known timeout keys, and a Configuration.getTimeout(default) resolver. The shape is clean and the api snapshot is regenerated correctly. One thing should be fixed before merging; the rest are minor.

Issues

getTimeout() can throw on a parseable-but-oversized config value, breaking its "never throws" contractsdk-core/.../config/Configuration.kt:107-117. The KDoc promises that a bad config value behaves as if the key were absent and that resolution never throws. But parseDuration accepts large-yet-valid durations (e.g. READ_TIMEOUT=200000dDuration.ofDays(200000), ~547 years, which does not overflow), and that value flows straight into the builder's validated(), which rejects anything above the ~292-year nanosecond-representable bound with IllegalArgumentException. So any duration between ~292 years and the Duration overflow point parses successfully and then throws inside getTimeout, with no try/catch around the builder calls. This can crash configuration resolution on operator- or attacker-controlled env/sysprop input. Either clamp/validate inside getTimeout so the bad value is treated as absent, or have parseDuration reject anything above the representable bound. Worth a test for the oversized case.

getTimeout(default) drops the default's explicit read/write pinningsdk-core/.../config/Configuration.kt:108-117. The resolver only pins read/write when their own key is present and parseable; it never consults default.effectiveReadTimeout / effectiveWriteTimeout. So a default that explicitly pinned read (e.g. readTimeout(2s)) loses that pin when no READ_TIMEOUT key is set — read silently falls back to the resolved request timeout. That matches the KDoc, but it's surprising for a parameter named default, and there's no test covering a default with explicit read/write pins. Worth confirming this is the intended precedence and adding a test either way.

equals/hashCode use effective phases while the raw getters don'tsdk-core/.../config/Timeout.kt:239-254, 212-213. Equality compares the effective (inheritance-applied) read/write timeouts, but getReadTimeout()/getWriteTimeout() return the raw stored Duration (ZERO when unset). So two instances that are equals and share a hashCode can return different values from the raw getters (e.g. ofRequest(10s) vs a builder with requestTimeout(10s).readTimeout(10s)). This is documented as intentional, but it's an easy trap for callers reading the raw getters; a test pinning the divergence would make the contract explicit.

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.

Add a per-phase Timeout value type (connect/read/write/request)

1 participant