[kamailio/kamailio] http_client: Add parameter connect_timeout_ms / timeout_ms (PR #3609)
19 Oct
2023
19 Oct
'23
7:57 a.m.
#### 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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609(a)github.com>
19 Oct
19 Oct
10:32 a.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#issuecomment-1771114732
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/c1771114732(a)github.com>
20 Oct
20 Oct
8:53 a.m.
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) ?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#issuecomment-1772686893
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/c1772686893(a)github.com>
9:35 a.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#issuecomment-1772753418
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/c1772753418(a)github.com>
9:42 a.m.
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!
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#issuecomment-1772764514
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/c1772764514(a)github.com>
12:31 p.m.
Regarding the http_client API, there is no impact, because it uses the name of a preset
http connection.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#issuecomment-1773045819
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/c1773045819(a)github.com>
12:46 p.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#issuecomment-1773066161
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/c1773066161(a)github.com>
12:47 p.m.
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 :(
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#issuecomment-1773067457
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/c1773067457(a)github.com>
12:48 p.m.
@nchaigne: ok, let's see it.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#issuecomment-1773068842
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/c1773068842(a)github.com>
12:57 p.m.
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 ?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#issuecomment-1773081226
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/c1773081226(a)github.com>
1:30 p.m.
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!
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#issuecomment-1773126143
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/c1773126143(a)github.com>
24 Oct
24 Oct
8:55 a.m.
Closing this one, considering the related discussion in the other PR.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#issuecomment-1777151625
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/c1777151625(a)github.com>
8:55 a.m.
Closed #3609.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3609#event-10753550479
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3609/issue_event/10753550479(a)github.com>
444
days inactive
449
days old
12 comments
3 participants
participants (3)
-
Daniel-Constantin Mierla -
Nicolas C
-
Olle E. Johansson