[kamailio/kamailio] http_client: Add parameter connect_timeout_ms / timeout_ms (PR #3609)
#### Pre-Submission Checklist - [x] Commit message has the format required by CONTRIBUTING guide - [x] Commits are split per component (core, individual modules, libs, utils, ...) - [x] Each component has a single commit (if not, squash them into one commit) - [x] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [ ] Small bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: - [x] PR should be backported to stable branches - [x] Tested changes locally - [ ] Related to issue #XXXX (replace XXXX with an open issue number)
#### Description
The new parameter connect_timeout_ms (global) / timeout_ms (in httpcon) allows to specify a timeout in milliseconds on curl requests. If this parameter is defined (non zero), then the timeout in seconds is ignored.
See https://curl.se/libcurl/c/CURLOPT_TIMEOUT_MS.html
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3609
-- Commit Summary --
* http_client: Add parameter connect_timeout_ms / timeout_ms
-- File Changes --
M src/modules/http_client/curlcon.c (55) M src/modules/http_client/doc/http_client_admin.xml (29) M src/modules/http_client/functions.c (22) M src/modules/http_client/http_client.c (12) M src/modules/http_client/http_client.h (12)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3609.patch https://github.com/kamailio/kamailio/pull/3609.diff
Thank you for contributing. I would prefer if there's only one variable and that we use the milliseconds always internally and just keep the old setting for backwards compatibility.
Thank you for contributing. I would prefer if there's only one variable and that we use the milliseconds always internally and just keep the old setting for backwards compatibility.
What do you mean exactly ? A single parameter "timeout" which could store seconds and milliseconds, e.g. "1.5" ? Or two parameters "timeout" and "timeout_ms" but only store a value in milliseconds internally (with priority to "timeout_ms" if defined) ?
I think we need two config options - but they set the same variable internally and only call ONE curl interface - the one you add here. We need also to verify the http_client API if we need to add something there. Having two variables internally is just overhead.
I think we need two config options - but they set the same variable internally and only call ONE curl interface - the one you add here. We need also to verify the http_client API if we need to add something there. Having two variables internally is just overhead.
Alright, I'll try something like you propose.
I had thought of this initially, but it seemed too convoluted because the configuration can come from 3 different sources : global parameter, httpcon parameter, or file configuration... so each time we need to "fixup" the actual timeout, depending on whether we have the ms timeout or not.
I'll try a new pull request, and then you can check if you think it is better. Thanks for the feedback!
Regarding the http_client API, there is no impact, because it uses the name of a preset http connection.
It might add more complexity to try to work on the same variable, as I quickly looked over the commit of the PR, one field is inside a cfg-param reload structure, which is trickier to figure out if it was set because of a modparam or because of a rpc command and then know which parameter name was used to set it. I think it will bring more overhead in code maintenance than two variables (or two fields in the cfg-param reload structure).
If two variables/fields is considered overhead, I would rather suggest to remove the old second-based timeout completely and add the new one with a different name, so on upgrading to the next release, the config has to be updated as well.
It might add more complexity to try to work on the same variable, as I quickly looked over the commit of the PR, one field is inside a cfg-param reload structure, which is trickier to figure out if it was set because of a modparam or because of a rpc command and then know which parameter name was used to set it. I think it will bring more overhead in code maintenance than two variables (or two fields in the cfg-param reload structure).
If two variables/fields is considered overhead, I would rather suggest to remove the old second-based timeout completely and add the new one with a different name, so on upgrading to the next release, the config has to be updated as well.
I'm almost done with the second pull request :(
@nchaigne: ok, let's see it.
It might add more complexity to try to work on the same variable, as I quickly looked over the commit of the PR, one field is inside a cfg-param reload structure, which is trickier to figure out if it was set because of a modparam or because of a rpc command and then know which parameter name was used to set it. I think it will bring more overhead in code maintenance than two variables (or two fields in the cfg-param reload structure).
If two variables/fields is considered overhead, I would rather suggest to remove the old second-based timeout completely and add the new one with a different name, so on upgrading to the next release, the config has to be updated as well.
Regarding the RPC commands, I see only "httpclient.listcon" for this module. I don't think the module can be reconfigured through RPC, or am I missing something ?
Alright, the new pull request is done: https://github.com/kamailio/kamailio/pull/3611
I have tested locally with all possible combinations (I hope). Yes, it's a bit complicated... having just a timeout in ms would have been much simpler. But I wanted to avoid breaking existing configurations.
Anyway... let me know what you prefer to do!
Closing this one, considering the related discussion in the other PR.
Closed #3609.
-
Daniel-Constantin Mierla -
Nicolas C
-
Olle E. Johansson