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

Made driver HTTP request timeouts configurable. #171

Merged
merged 8 commits into from
Jun 12, 2024

Conversation

nzkozar
Copy link
Contributor

@nzkozar nzkozar commented Jun 11, 2024

Hi.

In a recent attempt to make use of this library for a project, I hit a case where the hard coded HTTP request timeout of 3s was holding up project performance due to the driver service (GeoPlugin) blocking my IP and not returning a response.

In this PR, I've made both timeout and connection timeout used in the HttpDriver::http() method configurable in location.php config file. Their default and fallback values remain at 3 seconds as they are in the current master branch.

I believe this change improves this library, by allowing developer more granular control, by setting the maximum time a driver HTTP request is allowed to hold up the execution of the program.

Let me know if any code changes are required to merge this.

@nzkozar
Copy link
Contributor Author

nzkozar commented Jun 11, 2024

Update:
After more digging through Laravel and Guzzle code, I've now upgraded this PR to allow for float timeout values.

The Http facade methods timeout(int) and connectTimeout(int) do not enable setting timeouts shorter than 1 second.
But most of the IP info APIs will respond in far less then 1 second when all is ok, so for this library I believe it makes sense to support this.

Sources for reference:

@stevebauman
Copy link
Owner

stevebauman commented Jun 11, 2024

Thanks for the PR @nzkozar!

Can we wrap these new config options inside of a parent key named http? That way we can just plug the config values directly into the Http::withOptions() method.

@nzkozar
Copy link
Contributor Author

nzkozar commented Jun 12, 2024

@stevebauman done. This does however force any future http config additions to be compatible with the Http::withOptions() parameters instead of them being able to be used with any individual Http facade methods.

@stevebauman stevebauman merged commit a937130 into stevebauman:master Jun 12, 2024
0 of 8 checks passed
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.

None yet

2 participants