Skip to content

Add opt-in connect timeout support to SDK HTTP client (#56)#57

Open
chrismshea wants to merge 1 commit into
mainfrom
56-connect-timeout-and-exception-surface
Open

Add opt-in connect timeout support to SDK HTTP client (#56)#57
chrismshea wants to merge 1 commit into
mainfrom
56-connect-timeout-and-exception-surface

Conversation

@chrismshea

Copy link
Copy Markdown

Summary

Adds opt-in support for a Guzzle connect timeout to the SDK's HTTP client, and documents the timeout exception surface. Backward-compatible: the default request timeout (30s) is unchanged, and no connect timeout is applied unless one is configured.

Motivation: the SDK is called synchronously in user-facing contexts (e.g. the Magento extension's add-to-cart path). With only a total request timeout and no connect timeout, a slow or unreachable API can block a request and tie up web/PHP workers. This adds the fast-fail lever for the unreachable-host case, and clarifies why a timeout is not caught by HttpException.

Changes

  • Http.php — new $connectTimeout constructor argument (default 0). When > 0, it is applied as Guzzle RequestOptions::CONNECT_TIMEOUT; when 0, no connect timeout is set, preserving prior behavior for existing consumers.
  • Sdk.php — new connect_timeout config key (default 0), plumbed to Http. Config docblock now documents api_request_timeout, connect_timeout, and the timeout exception surface.
  • HttpTest.php — 2 new tests asserting the client receives CONNECT_TIMEOUT only when configured.

Exception surface (documented in Sdk.php)

A Guzzle request/connect timeout raises GuzzleHttp\Exception\ConnectException / RequestException, which is not response-based and is therefore not represented by SubscribePro\Exception\HttpException (which requires a ResponseInterface). Consumers that must handle timeouts should catch the Guzzle transfer exceptions (or \RuntimeException) in addition to HttpException.

Backward compatibility

  • Default api_request_timeout remains 30.
  • connect_timeout defaults to 0 (disabled) — existing consumers see no behavior change.
  • The new Http constructor arg is optional and last-positional.

Testing

  • vendor/bin/phpunit tests/HttpTest.php → 20/20 pass (18 existing + 2 new).
  • Verified end-to-end in a Magento 2.4.8 environment: an unreachable API with connect_timeout=1 fails at ~1.0s with a ConnectException (confirming the connect timeout is applied).

Related

Closes #56

Adds a new `connect_timeout` config key (default 0 = disabled) plumbed
from Sdk config through Http to Guzzle RequestOptions::CONNECT_TIMEOUT.
When unset/0, no CONNECT_TIMEOUT is passed, preserving prior behavior for
existing consumers. The default api_request_timeout (30s) is unchanged.

Also documents the timeout exception surface: a Guzzle connect/request
timeout raises ConnectException/RequestException (not response-based),
which is not represented by HttpException; consumers must catch the
Guzzle transfer exceptions to handle timeouts.

Refs #56
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.

Reduce default API request timeout and add connect-timeout support

1 participant