[kamailio/kamailio] http_client: Add parameter connect_timeout_ms / timeout_ms (2) (PR #3611)
#### 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
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 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!
@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?
@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.
@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!
@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.
@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
@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 ```
@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.
@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.
Closed #3611.
-
Daniel-Constantin Mierla -
Nicolas C
-
Olle E. Johansson