From 2c21c17294ec265ecf66f7f3a080071ecf6b4437 Mon Sep 17 00:00:00 2001 From: Nick Porter Date: Thu, 23 Feb 2023 18:25:33 +0000 Subject: [PATCH] Define max_retries for pool connections (#4908) Allows control over the number of times a connection operation can be retried before the module call fails. Previously this was always set to the number of connections in the pool - so on a system with a large number of open connections, and a remote server going slow, this would easily block threads. --- raddb/mods-available/ldap | 5 +++++ raddb/mods-available/sql | 5 +++++ src/include/connection.h | 2 ++ src/main/connection.c | 15 +++++++++++++++ src/modules/rlm_ldap/ldap.c | 6 +++--- src/modules/rlm_sql/sql.c | 4 ++-- 6 files changed, 32 insertions(+), 5 deletions(-) diff --git a/raddb/mods-available/ldap b/raddb/mods-available/ldap index 5d87f848ca..e3032a44d3 100644 --- a/raddb/mods-available/ldap +++ b/raddb/mods-available/ldap @@ -696,5 +696,10 @@ ldap { # # The solution is to either lower the 'min' connections, # or increase lifetime/idle_timeout. + + # Maximum number of times an operation can be retried + # if it returns an error which indicates the connection + # needs to be restarted. This includes timeouts. + max_retries = 5 } } diff --git a/raddb/mods-available/sql b/raddb/mods-available/sql index 7bcb664d32..0f435add70 100644 --- a/raddb/mods-available/sql +++ b/raddb/mods-available/sql @@ -325,6 +325,11 @@ sql { # # The solution is to either lower the "min" connections, # or increase lifetime/idle_timeout. + + # Maximum number of times an operation can be retried + # if it returns an error which indicates the connection + # needs to be restarted. This includes timeouts. + max_retries = 5 } # Set to 'yes' to read radius clients from the database ('nas' table) diff --git a/src/include/connection.h b/src/include/connection.h index e63820db84..1fd4be4c8e 100644 --- a/src/include/connection.h +++ b/src/include/connection.h @@ -84,6 +84,8 @@ fr_connection_pool_t *fr_connection_pool_module_init(CONF_SECTION *module, */ int fr_connection_pool_get_num(fr_connection_pool_t *pool); +int fr_connection_pool_get_retries(fr_connection_pool_t *pool); + /* * Pool management */ diff --git a/src/main/connection.c b/src/main/connection.c index b5a0eea32e..7ae4a2a19b 100644 --- a/src/main/connection.c +++ b/src/main/connection.c @@ -97,6 +97,8 @@ struct fr_connection_pool_t { uint32_t spare; //!< Number of spare connections to try. uint32_t pending; //!< Number of pending open connections. uint32_t retry_delay; //!< seconds to delay re-open after a failed open. + uint32_t max_retries; //!< Maximum number of retries to attempt for any given + //!< operation (e.g. query or bind) uint32_t cleanup_interval; //!< Initial timer for how often we sweep the pool //!< for free connections. (0 is infinite). int delay_interval; //!< When we next do a cleanup. Initialized to @@ -158,6 +160,7 @@ static const CONF_PARSER connection_config[] = { { "cleanup_interval", FR_CONF_OFFSET(PW_TYPE_INTEGER, fr_connection_pool_t, cleanup_interval), "30" }, { "idle_timeout", FR_CONF_OFFSET(PW_TYPE_INTEGER, fr_connection_pool_t, idle_timeout), "60" }, { "retry_delay", FR_CONF_OFFSET(PW_TYPE_INTEGER, fr_connection_pool_t, retry_delay), "1" }, + { "max_retries", FR_CONF_OFFSET(PW_TYPE_INTEGER, fr_connection_pool_t, max_retries), "5" }, { "spread", FR_CONF_OFFSET(PW_TYPE_BOOLEAN, fr_connection_pool_t, spread), "no" }, CONF_PARSER_TERMINATOR }; @@ -1257,6 +1260,18 @@ int fr_connection_pool_get_num(fr_connection_pool_t *pool) return pool->stats.num; } +/** Get the number of times an operation should be retried + * + * The lower of either the number of available connections or + * the configured max_retries. + * + * @param pool to get the retry count for. + * @return the number of times an operation can be retried. + */ +int fr_connection_pool_get_retries(fr_connection_pool_t *pool) +{ + return (pool->max_retries < pool->stats.num) ? pool->max_retries : pool->stats.num; +} /** Get the number of connections currently in the pool * diff --git a/src/modules/rlm_ldap/ldap.c b/src/modules/rlm_ldap/ldap.c index 97fdcbe2f3..542c086223 100644 --- a/src/modules/rlm_ldap/ldap.c +++ b/src/modules/rlm_ldap/ldap.c @@ -717,7 +717,7 @@ ldap_rcode_t rlm_ldap_bind(rlm_ldap_t const *inst, REQUEST *request, ldap_handle * For sanity, for when no connections are viable, * and we can't make a new one. */ - num = retry ? fr_connection_pool_get_num(inst->pool) : 0; + num = retry ? fr_connection_pool_get_retries(inst->pool) : 0; for (i = num; i >= 0; i--) { #ifdef WITH_SASL if (sasl && sasl->mech) { @@ -877,7 +877,7 @@ ldap_rcode_t rlm_ldap_search(LDAPMessage **result, rlm_ldap_t const *inst, REQUE * For sanity, for when no connections are viable, * and we can't make a new one. */ - for (i = fr_connection_pool_get_num(inst->pool); i >= 0; i--) { + for (i = fr_connection_pool_get_retries(inst->pool); i >= 0; i--) { (void) ldap_search_ext((*pconn)->handle, dn, scope, filter, search_attrs, 0, serverctrls, clientctrls, &tv, 0, &msgid); @@ -1004,7 +1004,7 @@ ldap_rcode_t rlm_ldap_modify(rlm_ldap_t const *inst, REQUEST *request, ldap_hand * For sanity, for when no connections are viable, * and we can't make a new one. */ - for (i = fr_connection_pool_get_num(inst->pool); i >= 0; i--) { + for (i = fr_connection_pool_get_retries(inst->pool); i >= 0; i--) { RDEBUG2("Modifying object with DN \"%s\"", dn); (void) ldap_modify_ext((*pconn)->handle, dn, mods, NULL, NULL, &msgid); diff --git a/src/modules/rlm_sql/sql.c b/src/modules/rlm_sql/sql.c index 5cc020eb45..a18e00b1fe 100644 --- a/src/modules/rlm_sql/sql.c +++ b/src/modules/rlm_sql/sql.c @@ -292,7 +292,7 @@ sql_rcode_t rlm_sql_query(rlm_sql_t *inst, REQUEST *request, rlm_sql_handle_t ** /* * inst->pool may be NULL is this function is called by mod_conn_create. */ - count = inst->pool ? fr_connection_pool_get_num(inst->pool) : 0; + count = inst->pool ? fr_connection_pool_get_retries(inst->pool) : 0; /* * Here we try with each of the existing connections, then try to create @@ -392,7 +392,7 @@ sql_rcode_t rlm_sql_select_query(rlm_sql_t *inst, REQUEST *request, rlm_sql_hand /* * inst->pool may be NULL is this function is called by mod_conn_create. */ - count = inst->pool ? fr_connection_pool_get_num(inst->pool) : 0; + count = inst->pool ? fr_connection_pool_get_retries(inst->pool) : 0; /* * For sanity, for when no connections are viable, and we can't make a new one -- 2.47.3