Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'retry_on_timeout': false blocks all ConnectException not only timeout #37

Open
ViktorCollin opened this issue Nov 21, 2023 · 0 comments
Assignees
Labels
Milestone

Comments

@ViktorCollin
Copy link

Detailed description

First of all I want to thank you for a great project!

When have set the retry_on_timeout setting to false (default value) it blocks all request resulting in a ConnectException from being retried. This causes confusion when using the library. I have some suggested solutions of varying degree ROI. I can help with the implementation if you want to, just let me know what solution you prefer.

Context

protected function shouldRetryConnectException(array $options, RequestInterface $request): bool
{
    return $options['retry_enabled']
        && ($options['retry_on_timeout'] ?? false)
        && $this->hasTimeAvailable($options) !== false
        && $this->countRemainingRetries($options) > 0
        && $this->ensureMethod($options, $request);
}

https://github.com/caseyamcl/guzzle_retry_middleware/blob/v2.9.0/src/GuzzleRetryMiddleware.php#L230-L243

Possible implementation

  1. Just rename retry_on_timeout to retry_on_connection_issue
    Pros: Small amount of work
    Cons: Require a major version bump which may be a bit aggressive for such a small change
  2. Introduce a new option called retry_on_connection_issue with a default value of false and deprecating the retry_on_timeout option. Check if ether one is true when deciding if a retry should be done
    Pros: Keep backwards compatibility
    Cons: Require the documentation to be clear in order to avoid confusion

There are more possible solutions, please let me know what you think

@caseyamcl caseyamcl self-assigned this Nov 30, 2023
@caseyamcl caseyamcl added the bug label Nov 30, 2023
@caseyamcl caseyamcl added this to the 2.9.1 milestone Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants