[pacman-dev] [PATCH] Add config var to limit repo connection timeout
New ConnectionTimeout variable sets the amount of time trying to connect to a repository, before timing out and trying the next one. Signed-off-by: Nathan Aclander <nathan.aclander@gmail.com> --- doc/pacman.conf.5.asciidoc | 3 +++ etc/pacman.conf.in | 1 + lib/libalpm/alpm.h | 3 +++ lib/libalpm/dload.c | 2 +- lib/libalpm/handle.c | 7 +++++++ lib/libalpm/handle.h | 1 + src/pacman/conf.c | 7 +++++++ src/pacman/conf.h | 1 + 8 files changed, 24 insertions(+), 1 deletion(-) diff --git a/doc/pacman.conf.5.asciidoc b/doc/pacman.conf.5.asciidoc index b297e332..dee5139e 100644 --- a/doc/pacman.conf.5.asciidoc +++ b/doc/pacman.conf.5.asciidoc @@ -205,6 +205,9 @@ Options Disable defaults for low speed limit and timeout on downloads. Use this if you have issues downloading files with proxy and/or security gateway. +*ConnectionTimeout*:: + Set the amount of time trying to connect to a repository, before timnig + out and trying the next one. Repository Sections ------------------- diff --git a/etc/pacman.conf.in b/etc/pacman.conf.in index 7446944f..899ad18f 100644 --- a/etc/pacman.conf.in +++ b/etc/pacman.conf.in @@ -34,6 +34,7 @@ Architecture = auto #TotalDownload CheckSpace #VerbosePkgLists +#ConnectionTimeout = 10 # PGP signature checking #SigLevel = Optional diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index ffb2ad96..e0bf12ca 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -890,6 +890,9 @@ int alpm_option_set_remote_file_siglevel(alpm_handle_t *handle, int level); int alpm_option_set_disable_dl_timeout(alpm_handle_t *handle, unsigned short disable_dl_timeout); +/** Sets the timeout for the curl connect phase. */ +int alpm_option_set_connecttimeout(alpm_handle_t *handle, unsigned int timeout); + /** @} */ /** @addtogroup alpm_api_databases Database Functions diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 05813c40..4e0703b4 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -258,7 +258,7 @@ static void curl_set_handle_opts(struct dload_payload *payload, curl_easy_reset(curl); curl_easy_setopt(curl, CURLOPT_URL, payload->fileurl); curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, error_buffer); - curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, 10L); + curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, handle->connecttimeout); curl_easy_setopt(curl, CURLOPT_MAXREDIRS, 10L); curl_easy_setopt(curl, CURLOPT_FILETIME, 1L); curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0L); diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 476779c4..92bc9038 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -556,6 +556,13 @@ int SYMEXPORT alpm_option_set_usesyslog(alpm_handle_t *handle, int usesyslog) return 0; } +int SYMEXPORT alpm_option_set_connecttimeout(alpm_handle_t *handle, unsigned int timeout) +{ + CHECK_HANDLE(handle, return -1); + handle->connecttimeout = timeout; + return 0; +} + static int _alpm_option_strlist_add(alpm_handle_t *handle, alpm_list_t **list, const char *str) { char *dup; diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 14d20bbe..9defbc1b 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -103,6 +103,7 @@ struct __alpm_handle_t { upgrade operations */ int remotefilesiglevel; /* Signature verification level for remote file upgrade operations */ + unsigned int connecttimeout; /* timeout for the connect phase */ /* error code */ alpm_errno_t pm_errno; diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 3b79fbc7..2cdc157e 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -513,6 +513,8 @@ static int _parse_options(const char *key, char *value, setrepeatingoption(value, "IgnoreGroup", &(config->ignoregrp)); } else if(strcmp(key, "HoldPkg") == 0) { setrepeatingoption(value, "HoldPkg", &(config->holdpkg)); + } else if(strcmp(key, "ConnectionTimeout") == 0) { + config->connecttimeout = atoi(value); } else if(strcmp(key, "CacheDir") == 0) { setrepeatingoption(value, "CacheDir", &(config->cachedirs)); } else if(strcmp(key, "HookDir") == 0) { @@ -739,6 +741,10 @@ static int setup_libalpm(void) alpm_option_set_totaldlcb(handle, cb_dl_total); } + if(!config->connecttimeout) { + alpm_option_set_connecttimeout(handle, 10L); + } + alpm_option_set_arch(handle, config->arch); alpm_option_set_checkspace(handle, config->checkspace); alpm_option_set_usesyslog(handle, config->usesyslog); @@ -749,6 +755,7 @@ static int setup_libalpm(void) alpm_option_set_noextracts(handle, config->noextract); alpm_option_set_disable_dl_timeout(handle, config->disable_dl_timeout); + alpm_option_set_connecttimeout(handle, config->connecttimeout); for(i = config->assumeinstalled; i; i = i->next) { char *entry = i->data; diff --git a/src/pacman/conf.h b/src/pacman/conf.h index f45ed436..e1f9934e 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -118,6 +118,7 @@ typedef struct __config_t { /* select -Sc behavior */ unsigned short cleanmethod; alpm_list_t *holdpkg; + unsigned int connecttimeout; alpm_list_t *ignorepkg; alpm_list_t *ignoregrp; alpm_list_t *assumeinstalled; -- 2.21.0
On 7/4/19 7:47 am, Nathan Aclander wrote:
New ConnectionTimeout variable sets the amount of time trying to connect to a repository, before timing out and trying the next one.
Signed-off-by: Nathan Aclander <nathan.aclander@gmail.com>
We currently have the ability to wait 10 seconds or forever. I don't think more fine grained controls are needed. Allan
I'm fairly new to using git send-email, so I wasn't sure how to add an explanation to the patch while cleanly separating it from the git commit message. I can speak more to why I found it useful. While I was traveling, certain mirrors became unreachable, or extremely slow. I had to wait 10 seconds not only for each repo, but then again every time pacman would try and download an individual package. Since this was set at 10 seconds by default, the wait time was far too long. I wanted to change it to 1 second so that pacman would move on to the next mirror faster. Using this patch seemed more usable for me than to having to comment out mirrors manually depending my network conditions. Are you concerned that having this parameter be tunable would cause pacman.conf to be too complex? Nathan
On 04/06/19 at 04:43pm, Nathan Aclander wrote:
I'm fairly new to using git send-email, so I wasn't sure how to add an explanation to the patch while cleanly separating it from the git commit message. I can speak more to why I found it useful.
Use --annotate and put comments you don't want to end up in the log between the diff stats and the first 'diff --git' line: --- doc/pacman.conf.5.asciidoc | 3 +++ 8 files changed, 24 insertions(+), 1 deletion(-) <--- Comments go here. diff --git a/doc/pacman.conf.5.asciidoc b/doc/pacman.conf.5.asciidoc
While I was traveling, certain mirrors became unreachable, or extremely slow. I had to wait 10 seconds not only for each repo, but then again every time pacman would try and download an individual package. Since this was set at 10 seconds by default, the wait time was far too long. I wanted to change it to 1 second so that pacman would move on to the next mirror faster.
Using this patch seemed more usable for me than to having to comment out mirrors manually depending my network conditions. Are you concerned that having this parameter be tunable would cause pacman.conf to be too complex?
More or less, yes. We don't want to have to implement every single option that libcurl provides in pacman, so we try to require a fairly compelling justification before adding them. We are not against making network operations more configurable, though. In fact, we would like to get rid of the external downloader, but can't because some people need specific settings that we don't, and never will, expose directly. So, if you, or anybody else, can come up with a more general solution that doesn't require us to add dozens of options to pacman.conf, we'd love to hear it.
On Sun, 7 Apr 2019 at 21:02, Andrew Gregory <andrew.gregory.8@gmail.com> wrote:
On 04/06/19 at 04:43pm, Nathan Aclander wrote:
Using this patch seemed more usable for me than to having to comment out mirrors manually depending my network conditions. Are you concerned that having this parameter be tunable would cause pacman.conf to be too complex?
More or less, yes. We don't want to have to implement every single option that libcurl provides in pacman, so we try to require a fairly compelling justification before adding them. We are not against making network operations more configurable, though. In fact, we would like to get rid of the external downloader, but can't because some people need specific settings that we don't, and never will, expose directly. So, if you, or anybody else, can come up with a more general solution that doesn't require us to add dozens of options to pacman.conf, we'd love to hear it.
I ran into the same objections when I wanted to expose a curl option (in my case for client certificate authentication). But maintaining a patched pacman seemed like too much long-term work, so instead I made curl-inject-opt [1], a program using LD_PRELOAD to inject curl options into sessions of another program. I also added it to the AUR [2]. It's not nearly as user friendly as a native pacman configuration option would be though: $ sudo curl-inject-opt --connect-timeout 1000 pacman -Syu You could put that in a wrapper script of course. You can even call the wrapper `pacman` and put it somewhere in your PATH (if you specify the real pacman by absolute path). It also currently doesn't come close to supporting all CURL options, but the goal is to support all sensible CURL options. So if you need one that's missing, issues or pull requests are welcome. I just added --timeout and --connect-timeout :-) [1] https://github.com/de-vri-es/curl-inject-opt [2] https://aur.archlinux.org/packages/curl-inject-opt
Maarten de Vries <maarten@de-vri.es> writes:
I ran into the same objections when I wanted to expose a curl option (in my case for client certificate authentication). But maintaining a patched pacman seemed like too much long-term work, so instead I made curl-inject-opt [1], a program using LD_PRELOAD to inject curl options into sessions of another program. I also added it to the AUR [2].
That looks great, and would definitely solve my problem. Like you said, it's not the most user-friendly solution. I wish it be baked into pacman just so I don't have one more thing to set up the next time I reinstall Arch. Nathan
On 04/06/19 at 04:43pm, Nathan Aclander wrote:
I'm fairly new to using git send-email, so I wasn't sure how to add an explanation to the patch while cleanly separating it from the git commit message. I can speak more to why I found it useful.
Use --annotate and put comments you don't want to end up in the log between the diff stats and the first 'diff --git' line:
Thanks, I will make sure to do that in the future.
More or less, yes. We don't want to have to implement every single option that libcurl provides in pacman, so we try to require a fairly compelling justification before adding them. We are not against making network operations more configurable, though. In fact, we would like to get rid of the external downloader, but can't because some people need specific settings that we don't, and never will, expose directly. So, if you, or anybody else, can come up with a more general solution that doesn't require us to add dozens of options to pacman.conf, we'd love to hear it.
I'm not sure how to getting rid of an external downloader would solve the problem. Giving people the option to tune download options ( or any options ) would always result in a larger configuration file no? At best, we could stick all the curl options together in a block in pacman.conf to improve readability. I'm of the opinion that exposing more curl options is great, as long as pacman.conf remains organized. Supporting every single option is of course silly, but I thought Connection timeout would benefit enough people since it saved me from having to wait over 2 minutes for pacman to finish an update. I guess in the end it's a compromise between maintainability, customizability, and user-friendliness. Nathan
participants (4)
-
Allan McRae
-
Andrew Gregory
-
Maarten de Vries
-
Nathan Aclander