Skip to content

feat(http): add configurable request timeout - #131

Merged
Bars92 merged 23 commits into
mainfrom
feat/configurable-timeouts-review
Jul 30, 2026
Merged

feat(http): add configurable request timeout#131
Bars92 merged 23 commits into
mainfrom
feat/configurable-timeouts-review

Conversation

@gaokevin1

@gaokevin1 gaokevin1 commented Jul 25, 2026

Copy link
Copy Markdown
Member

Fixes: https://github.com/descope/etc/issues/17203

What changed

Adds a single requestTimeout config option (seconds, default 60) that bounds every HTTP call the SDK makes, plus a built-in 10s connection-establishment timeout. An optional httpClient can be supplied for full transport control.

$descopeSDK = new DescopeSDK([
    'projectId'      => $_ENV['DESCOPE_PROJECT_ID'],
    'requestTimeout' => 10, // Optional, seconds (default: 60)
]);

Why

Both Guzzle clients (API and SDKConfig) were built with new Client() and no timeout options, so a slow or unresponsive peer could hang a PHP worker indefinitely — bounded in practice only by PHP's default_socket_timeout. The JWKS fetch in SDKConfig was fully unbounded.

Design — parity with the sibling SDKs

This intentionally matches the single-knob design shared by the Go SDK (RequestTimeout) and Python SDK (timeout_seconds) rather than introducing a larger surface. The timeout is applied as a default option at client construction, so executeWithRetry and the request methods are unchanged. Retry semantics stay per-attempt, consistent with Go/Python.

  • requestTimeout accepts a positive int/float/numeric-string; invalid values throw InvalidArgumentException.
  • When a caller supplies their own httpClient, the SDK uses it verbatim and does not override its timeouts.

Tests

New APIHttpTimeoutTest (12 tests) covering defaults, configured values, injected-client passthrough, and validation. Full suite: 94 passing.

Supersedes #130

Replaces #130 with a lighter design that stays consistent across the Descope SDK family. Also fixes a bug in that PR where an injected client's own timeouts were overridden. The one idea from #130 not carried over is a separate managementRequestTimeout — left out for cross-SDK parity; can be revisited if there's demand.

🤖 Generated with Claude Code

gaokevin1 and others added 23 commits July 6, 2023 10:23
The Guzzle clients in API and SDKConfig were constructed with no timeout
options, so requests could hang indefinitely (bounded in practice only by
PHP's default_socket_timeout). This adds a single `requestTimeout` config
knob matching the Go and Python SDKs, applied as a default option at client
construction alongside a built-in 10s connect timeout. Defaults: 60s request,
10s connect.

An optional `httpClient` may also be supplied; when present the SDK uses it
verbatim and does not override its transport configuration.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@shuni-bot

shuni-bot Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

🐕 Review complete — View session on Shuni Portal 🐾

@shuni-bot

shuni-bot Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

🐕 Suggested Reviewers

The review assignment prioritizes contributors with broad involvement in core SDK files and testing, ensuring comprehensive evaluation of the new configurable request timeout feature.

Reviewer Reason
omercnet omercnet has contributed extensively to multiple core files including API.php and SDK configurations, indicating a broad understanding of the SDK's structure and HTTP handling.
dorsha dorsha has also made significant contributions across key files, including API.php and the test suite, providing valuable insights into both implementation and testing of the HTTP timeout features.

Suggested by Shuni based on git history and PR context. Names are not @-mentioned to avoid notifying anyone — request a review from whoever fits best.

@shuni-bot shuni-bot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐕 Shuni's Review

Adds a single requestTimeout config option (default 60s) plus a built-in 10s connect timeout, bounding every SDK HTTP call, with optional httpClient passthrough.

No issues found — good bones! Traced all paths: array-union client options, API import in SDKConfig, resolveRequestTimeout validation (rejects bool/0/negative/non-numeric, guards INF/NAN), and injected-client verbatim passthrough all check out. New constructor params are optional → backward compatible, and tests match the implementation. Woof!

@gaokevin1
gaokevin1 requested review from Bars92, dorsha and omercnet July 29, 2026 21:27
@Bars92
Bars92 merged commit fd0509f into main Jul 30, 2026
16 checks passed
@Bars92
Bars92 deleted the feat/configurable-timeouts-review branch July 30, 2026 13:15
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.

2 participants