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

Add CLI flag to retry HTTP requests #256

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

s2b
Copy link

@s2b s2b commented Feb 4, 2025

Due to implementation details in certain hosting setups, there can be a delay between creating the temporary cachetool file and it being accessible via HTTP. To address this, both FileGetContents and SymfonyHTTP clients are extended to retry HTTP requests on failure. By default, that new configuration option is set to 3, which should cover most cases. However, its value can also be adjusted via CLI (--web-retry) or config file (webRetry).

The test setup is extended to cover both successful and failed retries. In the test setup, the availability delay of the php server is used to cover the mentioned test cases.

Resolves #209

@gordalina
Copy link
Owner

I think it would be a better fit to do the retry logic at the application level, not at the adapter. We could then use the --retry global argument, if the adapter returns an error then we can do the retry.

What do you think?

@s2b
Copy link
Author

s2b commented Feb 4, 2025

I see your point. The only disadvantage would be that the Symfony implementation couldn't be used then.

@gordalina
Copy link
Owner

I'm sorry, I wasn't very clear. When I said Application, i mean in src/CacheTool.php in the __call function.

@s2b
Copy link
Author

s2b commented Feb 5, 2025

Can you explain why that location is better? From my point of view, this feature is only related to the web-based workflow and isn't relevant for CLI. So if it should be implemented "higher up" in the code, my next choice would be src/Adapter/Web.php.

However, this would still mean that we couldn't make use of Symfony's more sophisticated implementation of retries in RetryableHttpClient. However, if you prefer that approach, that's fine by me and I'll adjust the PR.

@gordalina
Copy link
Owner

This is something that also happens with fastcgi, so doing it at the cachetool layer would address both issues. See #199

@s2b
Copy link
Author

s2b commented Feb 9, 2025

Maybe I'm missing something here, but I don't think that this is possible without some major refactoring of the adapters.

As far as I can see, the adapters behave quite differently when errors occur. Sometimes, RuntimeException is thrown, sometimes a serialized string is returned (which contains an error key to differentiate errors from other output) and sometimes only a string is returned (where it wouldn't be possible to differentiate between error and other output).

Maybe it makes sense to rename the config option to retry, but to leave the implementation in the adapter layer?

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.

404 when using --web
2 participants