Skip to content

DefaultHTTPClientConfig accepts negative timeouts that fail or misbehave in HC5 #14

Description

@marcio-absmartly

Split out of the #11 review so it is tracked rather than lost.

Finding

Originally raised by CodeRabbit on core-api/src/test/java/com/absmartly/sdk/DefaultHTTPClientConfigTest.java.

DefaultHTTPClientConfig accepts negative timeout values without validation, but DefaultHTTPClient passes them straight to Apache HC5:

  • Timeout.ofMilliseconds(config.getConnectTimeout()) and Timeout.ofMilliseconds(config.getConnectionRequestTimeout()) throw IllegalArgumentException on a negative value — Timeout enforces Args.notNegative(). The failure surfaces at client construction, not at config time, so the stack trace points away from the actual mistake.
  • TimeValue.ofMilliseconds(config.getConnectionKeepAlive()) accepts -1 and interprets it as an indefinite keep-alive, which is unlikely to be what a caller passing -1 intended.

Verified against the current code: the factual claims hold.

Why it was not fixed in #11

DefaultHTTPClientConfig and DefaultHTTPClient are untouched by that branch — git diff origin/main...HEAD over both files is empty. The permissive setters are pre-existing behaviour on main, and rejecting values they currently accept is a breaking API change for anyone relying on them today. That belongs in its own PR with a release note, not smuggled into a test-coverage branch.

DefaultHTTPClientConfigTest (added by #11) pins only the POJO round-trip: it asserts the config stores what it was given. The finding is right that those cases read as if they bless negative inputs, but they do not exercise DefaultHTTPClient.

Options

  1. Validate in the setters — reject negative values with IllegalArgumentException at the point of the mistake. Breaking for existing callers; needs a release note and a major/minor version decision.
  2. Validate at client constructionDefaultHTTPClient normalises or rejects before handing values to HC5, with a clear message naming the config field. Narrower blast radius, still a behaviour change for anyone currently getting the raw HC5 exception.
  3. Document only — javadoc the accepted ranges and the -1 keep-alive meaning, leave behaviour alone.

Whichever is chosen, DefaultHTTPClientConfigTest's negative-value cases should be updated to assert the decided behaviour rather than the bare round-trip.

cc @joalves — flagging as agreed in the #11 thread.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions