[kamailio/kamailio] http_client: Add parameter connect_timeout_ms / timeout_ms (2) (PR #3611)
20 Oct
2023
20 Oct
'23
1:25 p.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:
- [ ] 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.
If either timeout or timeout_ms is defined at connection level, then they take precedence
over the global parameters.
These principles also apply to the file configuration.
The timeout in ms is used internally to set CURLOPT_TIMEOUT_MS.
The value is shown in ms when using RPC "httpclient.listcon".
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3611
-- Commit Summary --
* http_client: Add parameter connect_timeout_ms / timeout_ms (2)
-- File Changes --
M src/modules/http_client/curlcon.c (103)
M src/modules/http_client/doc/http_client_admin.xml (32)
M src/modules/http_client/functions.c (24)
M src/modules/http_client/http_client.c (12)
M src/modules/http_client/http_client.h (13)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3611.patch
https://github.com/kamailio/kamailio/pull/3611.diff
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611(a)github.com>
20 Oct
20 Oct
2:09 p.m.
I don't see a major difference from the point of view of the internal variables/fields
compared with the other PR #3609.
Maybe a simpler approach is to introduce a new parameter to specify what is the type
(unit) of the timeout, like `timeout_mode`, which if it is 0 (default) is going to
consider the values in seconds and if set to 1, then it is going to consider the values in
milliseconds and based on its value, the appropriate curl option is set.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611#issuecomment-1773180161
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611/c1773180161(a)github.com>
3:37 p.m.
I don't see a major difference from the point of
view of the internal variables/fields compared with the other PR #3609.
Maybe a simpler approach is to introduce a new parameter to specify what is the type
(unit) of the timeout, like `timeout_mode`, which if it is 0 (default) is going to
consider the values in seconds and if set to 1, then it is going to consider the values in
milliseconds and based on its value, the appropriate curl option is set.
I was trying to implement Olle's proposal, i.e. having only one timeout internally
(visible in the curl_con_t struct, and shown through RPC "httpclient.listcon").
Your proposal is interesting, it removes all the complexity and still allows existing
configuration to work as before. I'll look into it.
Thanks!
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611#issuecomment-1773298282
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611/c1773298282(a)github.com>
23 Oct
23 Oct
2:45 a.m.
@oej commented on this pull request.
@@ -187,15 +187,35 @@
modparam("http_client", "maxdatasize", 2000)
</para>
<para>
<emphasis>
- Default value is zero, i.e.,
- the timeout function is disabled.
+ Default value is 4.
Can you explain this change?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611#pullrequestreview-1691808459
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611/review/1691808459(a)github.com>
3:46 a.m.
@nchaigne commented on this pull request.
@@ -187,15 +187,35 @@
modparam("http_client", "maxdatasize", 2000)
</para>
<para>
<emphasis>
- Default value is zero, i.e.,
- the timeout function is disabled.
+ Default value is 4.
I probably should have not changed that in my pull request because it was unrelated to my
changes.
The default value for connection_timeout is 4.
But actually it's more complicated than that, and a bit confusing...
If the parameter connection_timeout is explicitly set to 0, then it is changed back to 4
in function mod_init.
However, if "httpcon" are defined, they are initialized when they are parsed,
i.e. before "mod_init" is called. Hence, for these connections the timeout is 0,
which means "no timeout".
But for HTTP requests performed without a named connection, the timeout will be 4...
In all cases, the default is 4, not 0.
The documentation should probably better explain all this.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611#discussion_r1368249834
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611/review/1691898036(a)github.com>
3:59 a.m.
@oej commented on this pull request.
@@ -187,15 +187,35 @@
modparam("http_client", "maxdatasize", 2000)
</para>
<para>
<emphasis>
- Default value is zero, i.e.,
- the timeout function is disabled.
+ Default value is 4.
Ouch, that seems like something that needs to be addressed, but in another PR. I think
this PR should be focused and not have that change, even if it's small. Open an issue
with the text you just wrote and attack it in a separate PR closing that issue. Thanks for
pointing this out!
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611#discussion_r1368268457
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611/review/1691923412(a)github.com>
6:19 a.m.
@nchaigne commented on this pull request.
@@ -187,15 +187,35 @@
modparam("http_client", "maxdatasize", 2000)
</para>
<para>
<emphasis>
- Default value is zero, i.e.,
- the timeout function is disabled.
+ Default value is 4.
Current behaviour is messy and confusing. Depending on the order of declarations in
configuration, the actual behaviour changes.
I propose to add parameter "timeout_mode" as suggested by Daniel, and rework the
timeout behaviour as described hereafter.
A curl timeout will always be set. It makes no sense to allow to block forever, and
it's dangerous.
If need be, user can configure a very large timeout.
```
default global timeout = 0 (unconfigured).
Parse connections as usual. If they have a timeout configured, use it.
In mod_init:
if global timeout == 0 (unconfigured):
if timeout_mode == 0 -> global timeout = 4 (seconds)
else -> global timeout = 4000 (milliseconds)
for each connection "conn":
if conn.timeout is configured, keep its value.
else: conn.timeout = global timeout (in seconds or milliseconds, depending on
timeout_mode).
When doing Curl requests (curL_request_url):
if timeout_mode == 0: set CURLOPT_TIMEOUT
else: set CURLOPT_TIMEOUT_MS
```
Let me know what you think.
I you agree, I will do a pull request with this proposal.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611#discussion_r1368443380
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611/review/1692191634(a)github.com>
24 Oct
24 Oct
8:54 a.m.
@miconda commented on this pull request.
@@ -187,15 +187,35 @@
modparam("http_client", "maxdatasize", 2000)
</para>
<para>
<emphasis>
- Default value is zero, i.e.,
- the timeout function is disabled.
+ Default value is 4.
I am for adding `timeout_mode`, seems cleaner to understand, imo. The only thing on your
proposal that breaks the existing functionality is the ability to disable the timeout. I
agree that there should be a timeout by default and I cannot think of a use case for
disabling it (maybe when it is sure the http service is available(?!?) and not setting it
saves some cpu/timer cycles), so I would propose the following:
- if timeout_mode is 0, then no timeout is set (disabled)
- if timeout_mode is 1, then timeout value is in seconds and if connection_timeout is not
set, then it is set to 4 (default), setting CURLOPT_TIMEOUT
- if timeout_mode is 2, then timeout value is in milliseconds and if connection_timeout is
not set, then it is set to 4000, setting CURLOPT_TIMEOUT_MS
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611#discussion_r1370110775
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611/review/1694764625(a)github.com>
10:35 a.m.
@nchaigne commented on this pull request.
@@ -187,15 +187,35 @@
modparam("http_client", "maxdatasize", 2000)
</para>
<para>
<emphasis>
- Default value is zero, i.e.,
- the timeout function is disabled.
+ Default value is 4.
I am for adding `timeout_mode`, seems cleaner to
understand, imo. The only thing on your proposal that breaks the existing functionality is
the ability to disable the timeout. I agree that there should be a timeout by default and
I cannot think of a use case for disabling it (maybe when it is sure the http service is
available(?!?) and not setting it saves some cpu/timer cycles), so I would propose the
following:
* if timeout_mode is 0, then no timeout is set (disabled)
* if timeout_mode is 1, then timeout value is in seconds and if connection_timeout is not
set, then it is set to 4 (default), setting CURLOPT_TIMEOUT
* if timeout_mode is 2, then timeout value is in milliseconds and if connection_timeout
is not set, then it is set to 4000, setting CURLOPT_TIMEOUT_MS
This seems a good idea. I will then implement this new timeout_mode behaviour.
In addition, if timeout_mode is 0, I propose to fixup all connections to force their
"timeout" to 0 (to reflect the fact that no timeout will be handled - this would
be useful when using RPC command "httpclient.listcon").
To recap:
```
default global timeout = 0 (unconfigured).
Parse connections as usual. If they have a timeout configured, use it.
In mod_init:
if global timeout == 0 (unconfigured), and timeout_mode is 1 or 2:
if timeout_mode == 1 -> global timeout = 4 (seconds)
if timeout_mode == 2 -> global timeout = 4000 (milliseconds)
for each connection "conn" (fixup):
if timeout_mode is not 1 or 2 -> conn.timeout = 0 (to reflect the fact that no
timeout will be handled)
else if conn.timeout is not configured -> conn.timeout = global timeout (in seconds
or milliseconds, depending on timeout_mode).
When doing Curl requests (curL_request_url):
if timeout_mode == 1: set CURLOPT_TIMEOUT
if timeout_mode == 2: set CURLOPT_TIMEOUT_MS
```
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611#discussion_r1370301748
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611/review/1695065191(a)github.com>
11:15 a.m.
@miconda commented on this pull request.
@@ -187,15 +187,35 @@
modparam("http_client", "maxdatasize", 2000)
</para>
<para>
<emphasis>
- Default value is zero, i.e.,
- the timeout function is disabled.
+ Default value is 4.
Ok.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611#discussion_r1370380835
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611/review/1695189348(a)github.com>
11:16 a.m.
@nchaigne commented on this pull request.
@@ -187,15 +187,35 @@
modparam("http_client", "maxdatasize", 2000)
</para>
<para>
<emphasis>
- Default value is zero, i.e.,
- the timeout function is disabled.
+ Default value is 4.
I have opened a new pull request for the new implementation.
This one can be closed.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611#discussion_r1370381900
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611/review/1695190999(a)github.com>
11:24 a.m.
Closed #3611.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3611#event-10755683869
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3611/issue_event/10755683869(a)github.com>
443
days inactive
447
days old
11 comments
3 participants
participants (3)
-
Daniel-Constantin Mierla -
Nicolas C
-
Olle E. Johansson