diff --git a/changes-entries/proxy-worker-maxbusy.txt b/changes-entries/proxy-worker-maxbusy.txt new file mode 100644 index 00000000000..efd2790a431 --- /dev/null +++ b/changes-entries/proxy-worker-maxbusy.txt @@ -0,0 +1,4 @@ + *) mod_proxy: Add a "maxbusy" BalancerMember parameter, a hard limit on the + number of requests concurrently active against a worker across all + child processes, distinct from the per-child connection pool size + controlled by "max". [Arturo Bernal] diff --git a/docs/log-message-tags/next-number b/docs/log-message-tags/next-number index 4319db81665..0c9437edc6b 100644 --- a/docs/log-message-tags/next-number +++ b/docs/log-message-tags/next-number @@ -1 +1 @@ -10636 +10642 diff --git a/docs/manual/mod/mod_proxy.xml b/docs/manual/mod/mod_proxy.xml index 32285802afd..6dd70adbc2b 100644 --- a/docs/manual/mod/mod_proxy.xml +++ b/docs/manual/mod/mod_proxy.xml @@ -1147,6 +1147,34 @@ ProxyPass "/example" "http://backend.example.com" max=20 ttl=120 retry=300 per process in the active MPM. In the Prefork MPM, this is always 1; while with other MPMs, it is controlled by the ThreadsPerChild directive. + maxbusy + 0 + Maximum number of requests that may be concurrently active + against this BalancerMember, + counted across all child processes of this httpd + instance. Unlike max, which bounds the size of the + per-child connection pool, maxbusy is a single global + limit on in-flight requests, and only applies to a balancer member. + While a member is at its maxbusy limit, the balancer + treats it as temporarily unavailable and selects another member with + spare capacity instead, the same way it already does for a member + that is disabled or in an error state; the balancer's own + timeout parameter (set on the + Proxy balancer://... + block, not on an individual member, and unrelated to a member's own + backend connection timeout) applies if no member currently has + capacity. A request only fails with + HTTP_SERVICE_UNAVAILABLE if no member becomes available + before that balancer timeout elapses, or immediately if none is + configured. The default, 0, means no limit. With + BalancerPersist on, a worker's + persisted shared state survives a restart as-is; changing only + maxbusy in the configuration does not replace that + persisted value, the same as for other worker parameters such as + loadfactor or smax. A full stop and start is + required for a configuration-only maxbusy change to take + effect in that case. + smax max Retained connection pool entries above this limit are freed diff --git a/include/ap_mmn.h b/include/ap_mmn.h index b69d07b74af..18ffe3b9498 100644 --- a/include/ap_mmn.h +++ b/include/ap_mmn.h @@ -739,6 +739,14 @@ * 20211221.31 (2.5.1-dev) Add ap_*_timingsafe() to httpd.h * 20211221.32 (2.5.1-dev) Add ap_cstr_stripws() to httpd.h * 20211221.33 (2.5.1-dev) Add ap_size_parameter_parse() to httpd.h + * 20211221.34 (2.5.1-dev) Add maxbusy to proxy_worker_shared, and + * ap_proxy_admission_table_init(), + * ap_proxy_admission_table_check(), + * ap_proxy_get_reserved_count(), + * ap_proxy_worker_has_capacity(), + * ap_proxy_try_reserve_worker() and + * ap_proxy_release_worker_reservation() to + * proxy_util.h */ #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */ @@ -746,7 +754,7 @@ #ifndef MODULE_MAGIC_NUMBER_MAJOR #define MODULE_MAGIC_NUMBER_MAJOR 20211221 #endif -#define MODULE_MAGIC_NUMBER_MINOR 33 /* 0...n */ +#define MODULE_MAGIC_NUMBER_MINOR 34 /* 0...n */ /** * Determine if the server's current MODULE_MAGIC_NUMBER is at least a diff --git a/modules/proxy/balancers/mod_lbmethod_heartbeat.c b/modules/proxy/balancers/mod_lbmethod_heartbeat.c index cf966d36f67..da4dbb1480c 100644 --- a/modules/proxy/balancers/mod_lbmethod_heartbeat.c +++ b/modules/proxy/balancers/mod_lbmethod_heartbeat.c @@ -15,6 +15,7 @@ */ #include "mod_proxy.h" +#include "proxy_util.h" #include "scoreboard.h" #include "ap_mpm.h" #include "apr_version.h" @@ -343,7 +344,7 @@ static proxy_worker *find_best_hb(proxy_balancer *balancer, ap_proxy_retry_worker_fn("BALANCER", *worker, r->server); } - if (PROXY_WORKER_IS_USABLE(*worker)) { + if (PROXY_WORKER_IS_USABLE(*worker) && ap_proxy_worker_has_capacity(*worker)) { server->worker = *worker; if (server->seen < LBM_HEARTBEAT_MAX_LASTSEEN) { apr_uint32_t ready = (apr_uint32_t)server->ready; diff --git a/modules/proxy/mod_proxy.c b/modules/proxy/mod_proxy.c index 3c2a28d9251..4614e115da2 100644 --- a/modules/proxy/mod_proxy.c +++ b/modules/proxy/mod_proxy.c @@ -161,6 +161,29 @@ static const char *set_worker_param(apr_pool_t *p, return "Smax must be a positive number"; worker->s->smax = ival; } + else if (!strcasecmp(key, "maxbusy")) { + /* Hard limit on the number of requests concurrently active + * against this worker, across all children. 0 (the default) + * means no limit. Only meaningful for a BalancerMember: the + * admission table it is checked against is only consulted from + * mod_proxy_balancer, see ap_proxy_try_reserve_worker(). + */ + apr_off_t off; + + if (!worker->balancer) { + return "MaxBusy is only valid for a BalancerMember"; + } + if (ap_size_parameter_parse(val, &off) != APR_SUCCESS || off < 0) { + return "MaxBusy must be a non-negative integer"; + } + /* The live admission counter is a 32-bit atomic (see + * ap_proxy_try_reserve_worker()); reject what it cannot hold + * rather than silently truncate it. */ + if (off > APR_UINT32_MAX) { + return "MaxBusy must not exceed 4294967295"; + } + worker->s->maxbusy = (apr_size_t)off; + } else if (!strcasecmp(key, "acquire")) { /* Acquire timeout in given unit (default is milliseconds). * If set this will be the maximum time to @@ -1384,6 +1407,26 @@ static int proxy_handler(request_rec *r) do { char *url = uri; + + if (worker) { + /* Entering this iteration at all means the previous one left + * the worker unusable and attempts remain (see the loop + * condition below): a failover to a different worker is + * about to be attempted via the next ap_proxy_pre_request() + * call. The worker being abandoned is no longer handling + * this proxied request, so release its maxbusy admission + * reservation now rather than leaving it counted as busy for + * as long as the eventual successful attempt takes to + * complete. A no-op if it held no reservation (maxbusy == 0, + * or none could be obtained). This does not touch + * worker->s->busy (the bybusyness load-balancing metric), + * which keeps its original, unconditional request-pool + * lifetime. + */ + apr_pool_cleanup_run(r->pool, worker, + ap_proxy_release_worker_reservation); + } + /* Try to obtain the most suitable worker */ access_status = ap_proxy_pre_request(&worker, &balancer, r, conf, &url); if (access_status != OK) { @@ -3172,6 +3215,12 @@ static int proxy_post_config(apr_pool_t *pconf, apr_pool_t *plog, ap_proxy_strmatch_path = apr_strmatch_precompile(pconf, "path=", 0); ap_proxy_strmatch_domain = apr_strmatch_precompile(pconf, "domain=", 0); + /* Create (or, after the first restart in this process's lifetime, + * reuse) the maxbusy admission table before any child is forked; + * see the function's own comment for why this must outlive a + * graceful restart. */ + ap_proxy_admission_table_init(main_s); + for (; s; s = s->next) { int rc, i; proxy_server_conf *sconf = @@ -3188,6 +3237,13 @@ static int proxy_post_config(apr_pool_t *pconf, apr_pool_t *plog, } } + /* Pre-registering every maxbusy BalancerMember's admission slot (so + * the request path only ever looks one up, never creates one) needs + * balancer->s->sname, which is not final until mod_proxy_balancer's + * own post_config runs -- see ap_proxy_admission_table_check()'s own + * comment. It is called from there, not here. + */ + return OK; } diff --git a/modules/proxy/mod_proxy.h b/modules/proxy/mod_proxy.h index d51510fc02f..2407116e953 100644 --- a/modules/proxy/mod_proxy.h +++ b/modules/proxy/mod_proxy.h @@ -472,7 +472,15 @@ typedef struct { apr_size_t recv_buffer_size; apr_size_t io_buffer_size; apr_size_t elected; /* Number of times the worker was elected */ - apr_size_t busy; /* busyness factor */ + apr_size_t busy; /* busyness factor; a load-balancing metric + * only, reset by some lbmethods' reset() */ + apr_size_t maxbusy; /* Hard limit on concurrently active requests + * to this worker, across all children; 0 + * (the default) means no limit. This is + * configuration, not live state: the live + * admission count it is checked against + * lives outside this per-generation shared + * memory, see ap_proxy_try_reserve_worker(). */ apr_size_t response_field_size; /* Size of proxy response buffer in bytes. */ apr_port_t port; apr_off_t transferred;/* Number of bytes transferred to remote */ diff --git a/modules/proxy/mod_proxy_balancer.c b/modules/proxy/mod_proxy_balancer.c index 892bde38ae2..03150218bea 100644 --- a/modules/proxy/mod_proxy_balancer.c +++ b/modules/proxy/mod_proxy_balancer.c @@ -214,51 +214,59 @@ static proxy_worker *find_route_worker(proxy_balancer *balancer, if ( (checking_standby ? !PROXY_WORKER_IS_STANDBY(worker) : PROXY_WORKER_IS_STANDBY(worker)) ) continue; if (*(worker->s->route) && strcmp(worker->s->route, route) == 0) { - if (PROXY_WORKER_IS_USABLE(worker)) { - return worker; - } else { + int usable = PROXY_WORKER_IS_USABLE(worker); + + if (!usable) { /* * If the worker is in error state run * retry on that worker. It will be marked as * operational if the retry timeout is elapsed. * The worker might still be unusable, but we try - * anyway. + * anyway. Never done merely for a saturated maxbusy + * worker below: it was never in error, so there is + * nothing to retry. */ ap_proxy_retry_worker_fn("BALANCER", worker, r->server); - if (PROXY_WORKER_IS_USABLE(worker)) { - return worker; - } else { + usable = PROXY_WORKER_IS_USABLE(worker); + } + + if (usable && ap_proxy_worker_has_capacity(worker)) { + return worker; + } + + /* + * The sticky target is unreachable: either genuinely + * unusable (error or disabled, already retried above), + * or healthy but at its maxbusy limit. Both cases follow + * the same existing redirect semantics -- a saturated + * worker participates exactly like an unusable one here, + * without itself being marked in error -- in case it has + * a redirection set use that redirection worker. This + * enables to safely remove the member from the + * balancer. Of course you will need some kind of + * session replication between those two remote. + * Also check that we haven't gone thru all the + * balancer members by means of redirects. + * This should avoid redirect cycles. + */ + if ((*worker->s->redirect) + && (recursion < balancer->workers->nelts)) { + proxy_worker *rworker = NULL; + rworker = find_route_worker(balancer, worker->s->redirect, + r, recursion + 1); + /* Check if the redirect worker is usable */ + if (rworker && !PROXY_WORKER_IS_USABLE(rworker)) { /* - * We have a worker that is unusable. - * It can be in error or disabled, but in case - * it has a redirection set use that redirection worker. - * This enables to safely remove the member from the - * balancer. Of course you will need some kind of - * session replication between those two remote. - * Also check that we haven't gone thru all the - * balancer members by means of redirects. - * This should avoid redirect cycles. + * If the worker is in error state run + * retry on that worker. It will be marked as + * operational if the retry timeout is elapsed. + * The worker might still be unusable, but we try + * anyway. */ - if ((*worker->s->redirect) - && (recursion < balancer->workers->nelts)) { - proxy_worker *rworker = NULL; - rworker = find_route_worker(balancer, worker->s->redirect, - r, recursion + 1); - /* Check if the redirect worker is usable */ - if (rworker && !PROXY_WORKER_IS_USABLE(rworker)) { - /* - * If the worker is in error state run - * retry on that worker. It will be marked as - * operational if the retry timeout is elapsed. - * The worker might still be unusable, but we try - * anyway. - */ - ap_proxy_retry_worker_fn("BALANCER", rworker, r->server); - } - if (rworker && PROXY_WORKER_IS_USABLE(rworker)) - return rworker; - } + ap_proxy_retry_worker_fn("BALANCER", rworker, r->server); } + if (rworker && PROXY_WORKER_IS_USABLE(rworker)) + return rworker; } } } @@ -400,6 +408,56 @@ static proxy_worker *find_best_worker(proxy_balancer *balancer, } +/* Pick a worker via the balancer's load-balancing method and atomically + * reserve a maxbusy admission slot on it. If the chosen candidate's last + * slot is taken by another process in the (tiny) race window between + * selection and reservation, retry with a fresh selection; the lbmethod + * will now see that worker as saturated via ap_proxy_worker_has_capacity() + * and route around it. Bounded by the number of workers in the balancer, + * since under normal operation at most one retry is ever needed and this + * is purely a safety net against pathological contention. + * + * This is independent of find_best_worker()'s own retry loop, which + * already handles the case where *no* worker currently has capacity at + * all (reusing the balancer Timeout mechanism); this helper only retries + * when the lbmethod *did* find an apparently-usable candidate that lost + * its last slot to someone else a moment later. + * + * Returns the reserved worker (the caller must register + * ap_proxy_release_worker_reservation() on it), or NULL if no worker could + * be selected and reserved (already logged). + */ +static proxy_worker *find_and_reserve_worker(proxy_balancer *balancer, + request_rec *r) +{ + proxy_worker *runtime; + int tries; + + for (tries = 0; tries <= balancer->workers->nelts; tries++) { + runtime = find_best_worker(balancer, r); + if (!runtime) { + if (balancer->workers->nelts) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01170) + "%s: All workers are in error state or " + "unavailable (disabled, or at their maxbusy " + "limit)", balancer->s->name); + } else { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01171) + "%s: No workers in balancer", + balancer->s->name); + } + return NULL; + } + if (ap_proxy_try_reserve_worker(runtime)) { + return runtime; + } + } + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10640) + "%s: could not reserve a worker (all candidates reached " + "MaxBusy while retrying)", balancer->s->name); + return NULL; +} + static int balancer_fixup(request_rec *r, proxy_worker *worker, char **url) { const char *path; @@ -547,7 +605,8 @@ static int proxy_balancer_pre_request(proxy_worker **worker, } if (member_of) { ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01167) - "%s: All workers are in error state for route (%s)", + "%s: All workers are in error state or " + "unavailable for route (%s)", (*balancer)->s->name, route); #if APR_HAS_THREADS if ((rv = PROXY_THREAD_UNLOCK(*balancer)) != APR_SUCCESS) { @@ -567,39 +626,70 @@ static int proxy_balancer_pre_request(proxy_worker **worker, (*balancer)->s->name); } #endif - if (!*worker) { - runtime = find_best_worker(*balancer, r); - if (!runtime) { - if ((*balancer)->workers->nelts) { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01170) - "%s: All workers are in error state", - (*balancer)->s->name); - } else { - ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01171) - "%s: No workers in balancer", - (*balancer)->s->name); + if (*worker) { + /* A sticky-session target was found. Reserve its maxbusy slot; + * if that fails (it is at its limit), treat it exactly like an + * unusable sticky worker: nofailover (sticky_force) means 503, + * otherwise fail over to the normal load-balancing path below, + * same as the existing "all workers for this route are in error" + * failover, including BALANCER_ROUTE_CHANGED. + */ + if (!ap_proxy_try_reserve_worker(*worker)) { + if ((*balancer)->s->sticky_force) { + ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10641) + "%s: sticky worker (%s) is at its MaxBusy " + "limit (%" APR_SIZE_T_FMT ") and nofailover " + "is set", + (*balancer)->s->name, (*worker)->s->name, + (*worker)->s->maxbusy); + return HTTP_SERVICE_UNAVAILABLE; } + apr_table_setn(r->subprocess_env, "BALANCER_ROUTE_CHANGED", "1"); + *worker = NULL; + } + } + if (!*worker) { + runtime = find_and_reserve_worker(*balancer, r); + if (!runtime) { return HTTP_SERVICE_UNAVAILABLE; } - if (*(*balancer)->s->sticky && runtime) { + if (*(*balancer)->s->sticky) { /* * This balancer has sticky sessions and the client either has not - * supplied any routing information or all workers for this route + * supplied any routing information, or all workers for this route * including possible redirect and hotstandby workers are in error - * state, but we have found another working worker for this - * balancer where we can send the request. Thus notice that we have - * changed the route to the backend. + * state or at their MaxBusy limit, but we have found another + * working worker for this balancer where we can send the + * request. Thus notice that we have changed the route to the + * backend. */ apr_table_setn(r->subprocess_env, "BALANCER_ROUTE_CHANGED", "1"); } *worker = runtime; } + /* The busy counter is a load-balancing metric only (used by + * bybusyness); exactly as in unpatched trunk, it is incremented and + * its release registered unconditionally, independent of maxbusy. + */ ap_proxy_increment_busy_count(*worker); apr_pool_cleanup_register(r->pool, *worker, ap_proxy_decrement_busy_count, apr_pool_cleanup_null); + /* The maxbusy admission reservation above (if any was actually taken; + * none is for an unlimited worker) is a separate concern with its own + * cleanup, released independently -- possibly much earlier, by + * proxy_handler() in mod_proxy.c the moment it abandons this worker + * for a failover attempt, rather than only at the end of the whole + * request like the busy counter above. + */ + if ((*worker)->s->maxbusy) { + apr_pool_cleanup_register(r->pool, *worker, + ap_proxy_release_worker_reservation, + apr_pool_cleanup_null); + } + /* Add balancer/worker info to env. */ apr_table_setn(r->subprocess_env, "BALANCER_NAME", (*balancer)->s->name); @@ -851,6 +941,7 @@ static int balancer_post_config(apr_pool_t *pconf, apr_pool_t *plog, apr_time_t tstamp; apr_array_header_t *ids; int idx; + server_rec *main_s = s; /* 's' below is consumed by s = s->next */ /* balancer_post_config() will be called twice during startup. So, don't * set up the static data the 1st time through. */ @@ -1038,6 +1129,19 @@ static int balancer_post_config(apr_pool_t *pconf, apr_pool_t *plog, s = s->next; } + /* Every balancer->s->sname above is now final and every worker's + * shared memory is attached, so every statically configured maxbusy + * BalancerMember can be identified: pre-register its admission slot + * now, once, under proxy_mutex, instead of racing to create it from + * the request path later. See ap_proxy_admission_table_check()'s own + * comment for why this is both possible (no maxbusy worker can be + * created any other way) and preferable (no request-time creation + * race, old or new generation). + */ + if (ap_proxy_admission_table_check(main_s) != APR_SUCCESS) { + return !OK; + } + return OK; } @@ -1539,6 +1643,12 @@ static void balancer_display_page(request_rec *r, proxy_server_conf *conf, ap_rprintf(r, " %" APR_SIZE_T_FMT "\n", ap_proxy_get_busy_count(worker)); + ap_rprintf(r, + " %" APR_SIZE_T_FMT "\n", + worker->s->maxbusy); + ap_rprintf(r, + " %" APR_SIZE_T_FMT "\n", + ap_proxy_get_reserved_count(worker)); ap_rprintf(r, " %d\n", worker->s->lbset); /* End proxy_worker_stat */ diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c index a57c35ce0d2..24b708d81bd 100644 --- a/modules/proxy/proxy_util.c +++ b/modules/proxy/proxy_util.c @@ -22,6 +22,7 @@ #include "apr_strings.h" #include "apr_hash.h" #include "apr_atomic.h" +#include "apr_shm.h" #include "http_core.h" #include "proxy_util.h" #include "ajp.h" @@ -1453,7 +1454,17 @@ static proxy_worker *proxy_balancer_get_best_worker(proxy_balancer *balancer, } if (PROXY_WORKER_IS_SPARE(worker)) { - if (PROXY_WORKER_IS_USABLE(worker)) { + /* Capacity-eligible, not merely usable: the replacement + * loop below only examines the first unusable_workers + * entries of this array by position (one spare per + * unusable worker it replaces), so a saturated spare + * left in the array at an early index would silently + * hide a later, available one from ever being examined, + * even though a usable hot spare is supposed to replace + * every unusable worker of its lbset. + */ + if (PROXY_WORKER_IS_USABLE(worker) + && ap_proxy_worker_has_capacity(worker)) { APR_ARRAY_PUSH(spares, proxy_worker *) = worker; } } @@ -1463,8 +1474,18 @@ static proxy_worker *proxy_balancer_get_best_worker(proxy_balancer *balancer, } } else if (PROXY_WORKER_IS_USABLE(worker)) { - if (is_best(worker, best_worker, baton)) { - best_worker = worker; + if (ap_proxy_worker_has_capacity(worker)) { + if (is_best(worker, best_worker, baton)) { + best_worker = worker; + } + } + else { + /* At its maxbusy limit: not a candidate right now, but + * not an error either. Treat it like an unusable worker + * so a spare/standby can still be picked instead, and + * so the balancer-timeout retry in find_best_worker() + * re-evaluates it once capacity may have freed up. */ + unusable_workers++; } } else { @@ -1476,7 +1497,8 @@ static proxy_worker *proxy_balancer_get_best_worker(proxy_balancer *balancer, for (i = 0; (i < spares->nelts) && (i < unusable_workers); i++) { worker = APR_ARRAY_IDX(spares, i, proxy_worker *); - if (is_best(worker, best_worker, baton)) { + if (ap_proxy_worker_has_capacity(worker) + && is_best(worker, best_worker, baton)) { best_worker = worker; } } @@ -1486,7 +1508,8 @@ static proxy_worker *proxy_balancer_get_best_worker(proxy_balancer *balancer, for (i = 0; i < standbys->nelts; i++) { worker = APR_ARRAY_IDX(standbys, i, proxy_worker *); - if (is_best(worker, best_worker, baton)) { + if (ap_proxy_worker_has_capacity(worker) + && is_best(worker, best_worker, baton)) { best_worker = worker; } } @@ -5661,6 +5684,590 @@ PROXY_DECLARE(void) ap_proxy_increment_busy_count(proxy_worker *worker) #endif } +/* Cross-generation maxbusy admission table. + * + * worker->s->reserved would live in proxy_worker_shared, in the + * per-generation segment that mod_slotmem_shm names after the current + * MPM generation (modules/slotmem/mod_slotmem_shm.c). A graceful restart + * therefore starts every worker's hard-cap counter back at zero while + * old-generation requests may still be holding reservations, defeating + * the "global across all children" guarantee maxbusy documents (and, + * with BalancerPersist On, persisting and restoring a stale count can + * even wedge a worker at a phantom reservation forever). + * + * A plain module-static pointer does not survive a graceful restart + * either, unlike the scoreboard's: mod_so ties the dlopen() of a + * LoadModule'd .so to cmd->pool (modules/core/mod_so.c, dso_load()), + * which chains up to pconf, and reset_process_pconf() (server/main.c) + * destroys and recreates pconf on every restart, graceful included. + * APR's dlopen() wrapper (dso/unix/dso.c) passes no RTLD_NODELETE, so + * when that destruction drops mod_proxy.so's dlopen refcount to zero, + * the library is genuinely unmapped and reloaded with fresh, zeroed + * static storage before LoadModule processes the new config -- verified + * empirically for this change: a trivial static int in this file reset + * to its initial value across a real `-k graceful`, in the same parent + * PID with the same /proc//stat starttime and the very same + * virtual address for both the static and the mod_proxy.so mapping. + * + * The actual owner of the admission segment is therefore not a + * mod_proxy static at all, but an entry retained via httpd's own + * ap_retained_data_get()/_create() (include/http_config.h: "Store data + * which will be retained across unload/load of modules"), the same API + * this file already uses for proxy_start_time, backed by + * main_s->process->pool -- created once in the parent before the first + * post_config and never destroyed or recreated by a restart, graceful + * or not, unlike pconf. This is also the pattern mod_cgid uses + * (modules/generators/mod_cgid.c, cgid_init()) to keep its daemon + * handle across a restart. The module-static pointer below is only a + * same-process-call fast-path cache, re-synchronized from the retained + * data on every post_config (i.e. on every reload of this DSO), never + * treated as authoritative storage. ap_retained_data_create() stores + * its pointer with apr_pool_userdata_set(), not _setn(), internally + * (server/config.c): the key here is a string literal living in this + * DSO's own .rodata, and APR's own documentation for + * apr_pool_userdata_setn() (include/apr_pools.h) explicitly warns this + * is unsafe "if a static string is used as a userdata key in a DSO and + * the DSO could be unloaded and reloaded between the _setn() and the + * _get()" -- exactly this module's situation -- and says + * apr_pool_userdata_set() "MUST" be used instead, since it copies the + * key bytes into the pool rather than trusting the literal's address to + * remain valid. ap_retained_data_create() already satisfies this + * requirement, so mod_proxy itself never calls apr_pool_userdata_* for + * this purpose directly. + * + * Slots are claimed by a compound identity, not by a single 32-bit + * hash. worker->s->hash.{def,fnv} alone identify a *worker URL*, but + * maxbusy is a BalancerMember property, and ap_proxy_get_worker_ex() + * resolves a BalancerMember against its own balancer's worker list, while + * ap_proxy_find_workershm()/ap_proxy_share_worker() give every balancer + * its own worker slotmem segment -- confirmed by inspection and by a + * live test (two blocks balancing the same backend URL with + * different maxbusy= values get two distinct proxy_worker_shared + * records, at different shared-memory addresses). A table keyed only by + * worker->s->hash.fnv would silently merge their admission counts + * because that hash is derived purely from the backend URL. + * + * balancer->s->hash is not enough either: it is derived purely from + * balancer->s->name (ap_proxy_define_balancer()), the bare + * "balancer://foo" text, which is identical for the same balancer name + * defined in two different s. What actually distinguishes + * them is balancer->s->sname, built in balancer_post_config() + * (mod_proxy_balancer.c) as proxy_server_conf::id + "_" + the balancer's + * safe name, where conf->id in turn comes from make_server_id(), httpd's + * own mechanism for a stable per-server/vhost identity -- the same + * string httpd already relies on to re-attach the per-balancer worker + * slotmem segment across a restart (ap_proxy_initialize_balancer(), + * storage->attach(&balancer->wslot, balancer->s->sname, ...)). Hashing + * sname instead of the bare name is therefore what distinguishes two + * vhosts that happen to define the same balancer name, using the same + * identity convention httpd itself already depends on for that exact + * purpose, rather than inventing a separate server-ID mechanism. + * + * The slot stores both hashes of worker->balancer->s->sname and both + * hashes of worker->s->hash, and a lookup only accepts a slot whose + * stored identity matches on all four fields, so an accidental + * collision on any single 32-bit value still falls through to the next + * candidate slot instead of aliasing two different BalancerMembers. + * + * Slot *lookup* is lock-free (admission_slot_lookup()): since slots are + * never reclaimed for the lifetime of the admission segment, an empty + * slot along an identity's own open-addressing probe chain proves no + * slot for it exists, because insertion always claims the first empty + * slot along that same chain. Slot *creation* (admission_slot_for(), + * when no slot exists yet) is guarded by the existing process-global + * proxy_mutex (mod_proxy.c) instead: without it, one process could mark + * a slot claimed and be descheduled before writing its identity fields, + * and a second process, seeing claimed==1 with not-yet-visible identity, + * could fall through and claim a *second* slot for the very same + * BalancerMember, splitting its reservations across two counters and + * defeating maxbusy -- reproduced with a temporary, deliberately widened + * publication window before this fix. The lock is held only for the + * brief in-memory work of re-checking and publishing a slot, never + * across a lookup of an already-published slot (the steady-state case) + * and never across any backend I/O. + */ + +#define PROXY_ADMISSION_USERDATA_KEY "proxy_admission_state" +#define PROXY_ADMISSION_MAGIC 0x50414d54UL /* "PAMT" */ +#define PROXY_ADMISSION_VERSION 1 + +typedef struct { + volatile apr_uint32_t claimed; /* 0 = free, 1 = claimed */ + apr_uint32_t sname_def; /* hash of worker->balancer->s->sname: this + * *balancer instance*, including which + * server/vhost it was configured in */ + apr_uint32_t sname_fnv; + apr_uint32_t worker_def; /* hash of worker->s->hash: which worker + * within that balancer */ + apr_uint32_t worker_fnv; + volatile apr_uint32_t count; /* reservations currently held */ +} proxy_admission_slot_t; + +/* Header stored in process-pool userdata, identifying and owning the + * admission segment. Versioned so that a DSO reloaded from a changed + * binary (an admin swapping the module file between a `graceful -k` and + * the subsequent config reload picking it up) can detect an incompatible + * layout instead of reinterpreting old bytes as a new structure. + */ +typedef struct { + apr_uint32_t magic; + apr_uint32_t version; + apr_size_t struct_size; + apr_size_t slot_size; + apr_shm_t *shm; + proxy_admission_slot_t *base; + apr_uint32_t capacity; +} proxy_admission_state_t; + +/* Per-process cache of the state above, re-synchronized from process-pool + * userdata on every call to ap_proxy_admission_table_init() (i.e. on + * every post_config of this DSO instance); never authoritative. + */ +static proxy_admission_state_t *proxy_admission_cache = NULL; + +PROXY_DECLARE(void) ap_proxy_admission_table_init(server_rec *main_s) +{ + server_rec *s; + apr_uint32_t n = 0; + apr_size_t size; + apr_status_t rv; + proxy_admission_state_t *state; + + /* ap_retained_data_get()/_create(), not apr_pool_userdata_* directly: + * this is httpd's own purpose-built API for exactly this situation + * (include/http_config.h: "Store data which will be retained across + * unload/load of modules"), already relied on elsewhere in this file + * for proxy_start_time. ap_retained_data_create() stores the pointer + * with apr_pool_userdata_set() internally (server/config.c), so the + * _set()-not-_setn() requirement explained above is already handled + * by the API itself. + */ + state = ap_retained_data_get(PROXY_ADMISSION_USERDATA_KEY); + if (state) { + if (state->magic == PROXY_ADMISSION_MAGIC + && state->version == PROXY_ADMISSION_VERSION + && state->struct_size == sizeof(*state) + && state->slot_size == sizeof(proxy_admission_slot_t)) { + /* Compatible state from an earlier load of this DSO in this + * same process lifetime: reuse it, do not reset live + * admission counts that in-flight old-generation requests + * may still hold. + */ + proxy_admission_cache = state; + return; + } + /* Incompatible state: almost certainly a changed mod_proxy.so + * binary reloaded mid-lifetime (e.g. a package upgrade applied + * between a `graceful` and the config reload it triggers), since + * the magic/version/sizes are otherwise stable for a given + * build. There may still be old-generation children relying on + * the OLD segment's layout and content; leaving their memory + * alone and simply refusing to enforce maxbusy for the NEW + * generation (rather than reinterpreting or freeing memory a + * concurrently-running old generation might still reference) is + * the safe choice here. The old apr_shm_t itself is never + * explicitly destroyed either, for the same reason: it is a + * segment of process->pool, cleaned up (if ever) only when that + * pool itself goes away at final shutdown. + */ + ap_log_error(APLOG_MARK, APLOG_CRIT, 0, main_s, APLOGNO(10636) + "incompatible proxy maxbusy admission state found " + "after a module reload; maxbusy will not be " + "enforced for the new generation"); + proxy_admission_cache = NULL; + return; + } + + for (s = main_s; s; s = s->next) { + proxy_server_conf *conf = ap_get_module_config(s->module_config, + &proxy_module); + int i; + if (!conf || !conf->balancers) { + continue; + } + for (i = 0; i < conf->balancers->nelts; i++) { + proxy_balancer *b = &APR_ARRAY_IDX(conf->balancers, i, + proxy_balancer); + if (b->workers) { + n += (apr_uint32_t)b->workers->nelts; + } + } + } + /* Headroom for BalancerMembers a later config reload adds within the + * same (long-lived) process lifetime, without a cold restart, using + * the same convention ap_proxy_lb_workers() already uses for the + * analogous problem (the scoreboard cannot be resized across a + * reconfiguration either): a fixed number of extra slots, + * PROXY_DYNAMIC_BALANCER_LIMIT, rather than a multiplier of the + * current count. If enough *new* BalancerMembers accumulate across + * reloads to exceed this table's capacity, + * ap_proxy_admission_table_check() below fails startup for them + * (rather than admission_slot_for() silently failing closed at + * request time) until the process is restarted cold -- the same + * limitation ServerLimit already has across a graceful restart. + */ + n = n + PROXY_DYNAMIC_BALANCER_LIMIT; + size = (apr_size_t)n * sizeof(proxy_admission_slot_t); + + state = ap_retained_data_create(PROXY_ADMISSION_USERDATA_KEY, + sizeof(*state)); + rv = apr_shm_create(&state->shm, size, NULL, main_s->process->pool); + if (rv != APR_SUCCESS || !state->shm) { + ap_log_error(APLOG_MARK, APLOG_CRIT, rv, main_s, APLOGNO(10637) + "could not allocate the proxy maxbusy admission " + "table; maxbusy will not be enforced"); + return; + } + state->base = apr_shm_baseaddr_get(state->shm); + memset(state->base, 0, size); + state->capacity = n; + state->magic = PROXY_ADMISSION_MAGIC; + state->version = PROXY_ADMISSION_VERSION; + state->struct_size = sizeof(*state); + state->slot_size = sizeof(proxy_admission_slot_t); + + proxy_admission_cache = state; +} + +/* Forward declaration: defined further below, needed here because every + * statically configured BalancerMember with maxbusy != 0 is now + * pre-registered (admission_slot_for() creates its slot) from this + * function, rather than lazily on its first request. See the big + * comment above this table for why pre-registration is possible at all + * (every such worker is known by post_config time; the balancer-manager + * dynamic-worker-add path, b_wyes/b_nwrkr below, does not expose a + * maxbusy field and so cannot create a worker needing a slot later) and + * why it is preferable (no slot can ever be created from the request + * path, and therefore no two generations' request paths can race to + * create one -- see the proxy_mutex discussion above). + */ +static volatile apr_uint32_t *admission_slot_for(proxy_worker *worker); + +/* Any hard maxbusy configured anywhere is a promise this process cannot + * keep if its slot cannot be pre-registered; call this once per server, + * after balancer->s->sname and every worker's shared memory are final + * (i.e. at the end of mod_proxy_balancer's post_config, not mod_proxy's: + * the admission identity is keyed by sname, which does not exist yet + * when mod_proxy's own post_config runs). Turns an unrepresentable + * maxbusy member into a startup failure instead of a silently-unenforced + * limit or a worker that would return 503 forever once the table fills. + */ +PROXY_DECLARE(apr_status_t) ap_proxy_admission_table_check(server_rec *main_s) +{ + server_rec *s; + + for (s = main_s; s; s = s->next) { + proxy_server_conf *conf = ap_get_module_config(s->module_config, + &proxy_module); + int i; + if (!conf || !conf->balancers) { + continue; + } + for (i = 0; i < conf->balancers->nelts; i++) { + proxy_balancer *b = &APR_ARRAY_IDX(conf->balancers, i, + proxy_balancer); + int j; + if (!b->workers) { + continue; + } + for (j = 0; j < b->workers->nelts; j++) { + proxy_worker *worker = APR_ARRAY_IDX(b->workers, j, + proxy_worker *); + if (worker->s && worker->s->maxbusy + && !admission_slot_for(worker)) { + ap_log_error(APLOG_MARK, APLOG_EMERG, 0, main_s, + APLOGNO(10638) + "BalancerMember %s in balancer %s " + "configures maxbusy, but the maxbusy " + "admission table could not be created; " + "refusing to start rather than silently " + "not enforcing it", + worker->s->name, b->s->name); + return APR_EGENERAL; + } + } + } + } + return APR_SUCCESS; +} + +/* Hash worker's compound admission identity: which balancer instance + * (including the server/vhost it is configured in) and which worker + * within it. See the comment above this table for why both components + * are needed and why sname rather than balancer->s->hash. + */ +static void admission_identity(proxy_worker *worker, apr_uint32_t *sdef, + apr_uint32_t *sfnv, apr_uint32_t *wdef, + apr_uint32_t *wfnv) +{ + *sdef = ap_proxy_hashfunc(worker->balancer->s->sname, + PROXY_HASHFUNC_DEFAULT); + *sfnv = ap_proxy_hashfunc(worker->balancer->s->sname, + PROXY_HASHFUNC_FNV); + *wdef = worker->s->hash.def; + *wfnv = worker->s->hash.fnv; +} + +/* Lock-free lookup of an already-published slot for this identity. + * Returns NULL if none exists yet -- never creates or mutates the + * table, so this is safe for read-only/observability callers (e.g. + * rendering balancer-manager) and correctly reports 0 reservations for + * a worker that was never reserved, including every maxbusy=0 worker, + * which must never consume a table entry. See the comment above this + * table for why stopping at the first free slot on this identity's own + * probe chain is correct (slots are never reclaimed). + */ +static volatile apr_uint32_t *admission_slot_lookup(apr_uint32_t sdef, + apr_uint32_t sfnv, + apr_uint32_t wdef, + apr_uint32_t wfnv) +{ + apr_uint32_t probe, i, start, capacity; + proxy_admission_slot_t *base; + + if (!proxy_admission_cache || !proxy_admission_cache->base) { + return NULL; + } + base = proxy_admission_cache->base; + capacity = proxy_admission_cache->capacity; + + probe = wfnv; + probe = (probe * 16777619u) ^ wdef; + probe = (probe * 16777619u) ^ sfnv; + probe = (probe * 16777619u) ^ sdef; + start = probe % capacity; + i = start; + do { + if (!apr_atomic_read32(&base[i].claimed)) { + return NULL; + } + if (base[i].sname_def == sdef && base[i].sname_fnv == sfnv + && base[i].worker_def == wdef && base[i].worker_fnv == wfnv) { + return &base[i].count; + } + i = (i + 1) % capacity; + } while (i != start); + + return NULL; +} + +/* admission_slot_lookup() for a worker, resolving its identity first. + * Read-only, like admission_slot_lookup() itself: used by every + * request-path caller (ap_proxy_worker_has_capacity(), + * ap_proxy_try_reserve_worker(), ap_proxy_release_worker_reservation()), + * none of + * which may create a slot -- see the comment above this table for why + * creation is confined to ap_proxy_admission_table_check() at + * post_config time, before any request is ever served. + */ +static volatile apr_uint32_t *admission_slot_lookup_for(proxy_worker *worker) +{ + apr_uint32_t sdef, sfnv, wdef, wfnv; + + if (!worker->balancer) { + return NULL; + } + admission_identity(worker, &sdef, &sfnv, &wdef, &wfnv); + return admission_slot_lookup(sdef, sfnv, wdef, wfnv); +} + +/* Find this worker's admission slot, creating it under proxy_mutex if + * none exists yet. Called only from ap_proxy_admission_table_check() at + * post_config time (see the comment above this table): every + * statically configured maxbusy BalancerMember is pre-registered there, + * before any child is forked or any request served, so this never runs + * concurrently with itself or with a request-path lookup, and the + * request path itself never calls this function. Returns NULL if the + * table was never created, or is full: the post_config caller then + * fails startup rather than silently dropping the hard limit for a + * member it cannot represent. + */ +static volatile apr_uint32_t *admission_slot_for(proxy_worker *worker) +{ + apr_uint32_t sdef, sfnv, wdef, wfnv, probe, i, start, capacity; + proxy_admission_slot_t *base; + volatile apr_uint32_t *slot; + + if (!proxy_admission_cache || !proxy_admission_cache->base + || !worker->balancer) { + return NULL; + } + + admission_identity(worker, &sdef, &sfnv, &wdef, &wfnv); + + slot = admission_slot_lookup(sdef, sfnv, wdef, wfnv); + if (slot) { + return slot; + } + + base = proxy_admission_cache->base; + capacity = proxy_admission_cache->capacity; + + apr_global_mutex_lock(proxy_mutex); + + /* Re-check under the lock: another process may have created this + * exact slot between the lock-free lookup above and acquiring the + * lock. + */ + slot = admission_slot_lookup(sdef, sfnv, wdef, wfnv); + if (slot) { + apr_global_mutex_unlock(proxy_mutex); + return slot; + } + + probe = wfnv; + probe = (probe * 16777619u) ^ wdef; + probe = (probe * 16777619u) ^ sfnv; + probe = (probe * 16777619u) ^ sdef; + start = probe % capacity; + i = start; + do { + if (!apr_atomic_read32(&base[i].claimed)) { + /* Fully initialize the slot, including the count, before + * publishing it, so a concurrent lock-free reader can never + * observe claimed==1 with incomplete or stale identity. + * Only one process can reach here at a time (proxy_mutex), + * so a plain store for claimed is enough; apr_atomic_set32() + * is used anyway for the same cross-process visibility + * apr_atomic_read32() already relies on in the lock-free + * readers. + */ + base[i].sname_def = sdef; + base[i].sname_fnv = sfnv; + base[i].worker_def = wdef; + base[i].worker_fnv = wfnv; + base[i].count = 0; + apr_atomic_set32(&base[i].claimed, 1); + slot = &base[i].count; + break; + } + i = (i + 1) % capacity; + } while (i != start); + + apr_global_mutex_unlock(proxy_mutex); + + if (!slot) { + /* Table full: every slot already belongs to some other + * BalancerMember. Logged every time, not just once, since from + * here on maxbusy silently stops being enforced for this + * worker, which operators must not have to discover by noticing + * a concurrency anomaly instead of a log line. + */ + ap_log_error(APLOG_MARK, APLOG_ERR, 0, ap_server_conf, APLOGNO(10639) + "proxy maxbusy admission table is full; maxbusy " + "cannot be enforced for worker %s", worker->s->name); + } + return slot; +} + +PROXY_DECLARE(apr_size_t) ap_proxy_get_reserved_count(proxy_worker *worker) +{ + volatile apr_uint32_t *slot = admission_slot_lookup_for(worker); + return slot ? (apr_size_t)apr_atomic_read32((apr_uint32_t *)slot) : 0; +} + +PROXY_DECLARE(int) ap_proxy_worker_has_capacity(proxy_worker *worker) +{ + apr_uint32_t limit = (apr_uint32_t)worker->s->maxbusy; + volatile apr_uint32_t *slot; + + if (!limit) { + return 1; + } + slot = admission_slot_lookup_for(worker); + if (!slot) { + /* Fail closed: a maxbusy worker with no pre-registered slot + * means ap_proxy_admission_table_check() either never ran for + * it or the table could not represent it, either of which + * should already have failed startup -- but a worker this + * function cannot identify is one whose reservations this + * process cannot account for either, so treat it as saturated + * rather than silently drop the hard limit. + */ + return 0; + } + return apr_atomic_read32((apr_uint32_t *)slot) < limit; +} + +PROXY_DECLARE(int) ap_proxy_try_reserve_worker(proxy_worker *worker) +{ + apr_uint32_t limit = (apr_uint32_t)worker->s->maxbusy; + volatile apr_uint32_t *slot; + apr_uint32_t val, old; + + /* maxbusy == 0 means unlimited: succeed without touching the + * admission table at all, so the default/common case pays no + * atomic-op cost. + */ + if (!limit) { + return 1; + } + + slot = admission_slot_lookup_for(worker); + if (!slot) { + return 0; /* fail closed: see ap_proxy_worker_has_capacity() */ + } + + val = apr_atomic_read32((apr_uint32_t *)slot); + while (val < limit) { + old = val; + val = apr_atomic_cas32((apr_uint32_t *)slot, val + 1, old); + if (val == old) { + return 1; + } + } + return 0; +} + +PROXY_DECLARE(apr_status_t) ap_proxy_release_worker_reservation(void *worker_) +{ + proxy_worker *worker = worker_; + volatile apr_uint32_t *slot; + apr_uint32_t val, old; + + /* This releases only the maxbusy admission reservation. The busy + * load-balancing metric (ap_proxy_decrement_busy_count()) is a + * separate, independent cleanup with its own unconditional + * request-pool lifetime, exactly as in unpatched trunk; it is not + * touched here, and a caller abandoning this worker for a failover + * attempt must not release it early (see proxy_handler() in + * mod_proxy.c, which releases a reservation -- not busy -- the + * moment it commits to trying another worker). + * + * The slot is found by the worker's stable identity hash, the same + * one ap_proxy_try_reserve_worker() used, never by consulting + * worker->s->maxbusy (which may have changed since the reservation + * was made, e.g. across a graceful config reload). + * + * This function MUST be called exactly once for each successful, + * finite ap_proxy_try_reserve_worker() return, and never otherwise: + * calling it for a worker that was never actually reserved by this + * attempt would release another, unrelated caller's reservation, + * not a harmless no-op, because every maxbusy != 0 BalancerMember + * has its slot pre-registered at post_config and retained for the + * parent process's lifetime -- a worker with maxbusy != 0 therefore + * always HAS a slot here, whether or not this particular caller holds + * a reservation in it. The lookup below returning NULL only happens + * for a maxbusy == 0 worker, which never had a slot registered in + * the first place and for which no cleanup is ever installed. + */ + slot = admission_slot_lookup_for(worker); + if (!slot) { + return APR_SUCCESS; + } + + val = apr_atomic_read32((apr_uint32_t *)slot); + while (val > 0) { + old = val; + val = apr_atomic_cas32((apr_uint32_t *)slot, val - 1, old); + if (val == old) { + break; + } + } + return APR_SUCCESS; +} + + PROXY_DECLARE(apr_size_t) ap_proxy_get_busy_count(proxy_worker *worker) { apr_size_t val; diff --git a/modules/proxy/proxy_util.h b/modules/proxy/proxy_util.h index 9602dcb067f..6829fe6cd85 100644 --- a/modules/proxy/proxy_util.h +++ b/modules/proxy/proxy_util.h @@ -75,6 +75,117 @@ PROXY_DECLARE(apr_status_t) ap_proxy_decrement_busy_count(void *worker_); */ PROXY_DECLARE(void) ap_proxy_increment_busy_count(proxy_worker *worker); +/* + * Create (or, on a restart within the same process lifetime, reuse) the + * shared admission table that ap_proxy_try_reserve_worker() and friends + * use to enforce worker->s->maxbusy across all children AND across a + * graceful restart. Must be called once from the (long-lived) parent + * process, after all proxy_server_conf's are built, before any child is + * forked; mod_proxy.c's post_config hook is the right place. Harmless, + * and a correctly recognised no-op, if called again on every subsequent + * restart of the same process. + * + * @param main_s The main server_rec, to walk every virtual host's workers. + * @return void. Logs and leaves maxbusy unenforced on allocation + * failure, rather than failing the whole server. + */ +PROXY_DECLARE(void) ap_proxy_admission_table_init(server_rec *main_s); + +/* + * Fail-closed check for ap_proxy_admission_table_init(): if any + * configured worker has a non-zero maxbusy but the admission table + * could not be created, that limit cannot be enforced at all, so this + * logs and returns an error instead of letting the server start up + * silently violating a configured hard limit. Call once from + * post_config, after every worker's maxbusy is known, i.e. after + * ap_proxy_admission_table_init() and after all section/per-dir proxy + * configuration has been processed. + * + * @param main_s The main server_rec, to walk every virtual host's workers. + * @return APR_SUCCESS, or an error if startup must be refused. + */ +PROXY_DECLARE(apr_status_t) ap_proxy_admission_table_check(server_rec *main_s); + +/* + * Return the number of worker->s->maxbusy admission slots this worker + * currently has reserved, across all children and generations. For + * reporting only (e.g. the balancer-manager page); 0 if the admission + * table is unavailable. + * + * @param worker Pointer to the worker structure. + * @return apr_size_t Number of slots currently reserved. + */ +PROXY_DECLARE(apr_size_t) ap_proxy_get_reserved_count(proxy_worker *worker); + +/* + * Return non-zero if worker has a free maxbusy admission slot, i.e. it is + * not currently at its configured worker->s->maxbusy limit (0 means no + * limit, always has capacity). This is a point-in-time snapshot only: it + * is meant for use as a selection-time filter (skip saturated workers in + * favour of others), not as the actual admission decision. The admission + * decision itself must go through ap_proxy_try_reserve_worker(), which is + * atomic and race-free; this function alone is NOT sufficient to enforce + * the hard limit, since two processes could both see capacity and then + * both reserve the last slot. + * + * @param worker Pointer to the worker structure. + * @return non-zero if the worker currently has a free maxbusy slot. + */ +PROXY_DECLARE(int) ap_proxy_worker_has_capacity(proxy_worker *worker); + +/* + * Atomically reserve one worker->s->maxbusy admission slot, unless the + * worker is already at that limit, in which case no reservation is made. + * The check and the reservation are a single atomic operation across all + * children, so this is safe to use as a hard concurrency admission gate. + * This is independent of worker->s->busy (a load-balancing metric only): + * the reservation counter is never reset by any lbmethod's reset(), and + * releasing it (see ap_proxy_release_worker_reservation() below) has no + * effect on worker->s->busy. + * + * If worker->s->maxbusy is 0 (the default, no limit), this always + * succeeds without touching any shared counter. + * + * @param worker Pointer to the worker structure. + * @return 1 if a slot was reserved (the caller holds the reservation + * and must eventually call, exactly once, + * ap_proxy_release_worker_reservation()), 0 if + * worker->s->maxbusy was already reached (no reservation was + * made; nothing to release). + */ +PROXY_DECLARE(int) ap_proxy_try_reserve_worker(proxy_worker *worker); + +/* + * Release one worker->s->maxbusy admission slot previously obtained via a + * successful ap_proxy_try_reserve_worker(worker) call. Safe to register + * directly with apr_pool_cleanup_register() or run early with + * apr_pool_cleanup_run() (void * / apr_status_t signature). + * + * The contract is strict, not merely "safe by construction": a call here + * must correspond to exactly one prior successful reservation on this + * worker. The internal CAS loop floors at 0 only to avoid undefined + * underflow of the shared counter if that contract is ever violated by a + * caller bug -- flooring does NOT make a stray or duplicate call harmless: + * while the real count is still above 0, an extra release silently steals + * another, unrelated request's reservation. + * + * Looks the worker's admission slot up by its stable identity (the same + * one ap_proxy_try_reserve_worker() used), never by worker->s->maxbusy, + * so it stays correct even if that configured value changes between the + * reservation and this release (e.g. a graceful restart reconfigures the + * worker while an old-generation request is still outstanding on it). + * Admission slots are preregistered in post_config for every configured + * maxbusy != 0 BalancerMember and are never reclaimed for the life of the + * parent process, so "no slot" is not a normal case to release against: + * callers must call this function if and only if the matching + * ap_proxy_try_reserve_worker() call returned 1, exactly once per such + * call. It is not a safe default to call "just in case". + * + * @param worker_ Pointer to the worker structure. + * @return apr_status_t returns APR_SUCCESS. + */ +PROXY_DECLARE(apr_status_t) ap_proxy_release_worker_reservation(void *worker_); + /* * interpolate an env str in a configuration string diff --git a/test/modules/proxy/test_07_maxbusy.py b/test/modules/proxy/test_07_maxbusy.py new file mode 100644 index 00000000000..ffc6ec67f28 --- /dev/null +++ b/test/modules/proxy/test_07_maxbusy.py @@ -0,0 +1,761 @@ +import http.server +import re +import threading +import time +from concurrent.futures import ThreadPoolExecutor + +import pytest + +from pyhttpd.conf import HttpdConf + + +class SlowBackend: + """A threaded HTTP backend that tracks how many requests it is + handling concurrently, and holds each request open for `delay` + seconds. Used to prove that a BalancerMember's maxbusy limit bounds + the number of requests actually reaching the backend at once, + across all httpd child processes. + """ + + def __init__(self, port, delay=1.0): + self._port = port + self._delay = delay + self._lock = threading.Lock() + self._active = 0 + self._max_active = 0 + backend = self + + class Handler(http.server.BaseHTTPRequestHandler): + def do_GET(self): + with backend._lock: + backend._active += 1 + backend._max_active = max(backend._max_active, + backend._active) + try: + d = float(self.path.strip('/').split('?')[0] or backend._delay) + except ValueError: + d = backend._delay + time.sleep(d) + with backend._lock: + backend._active -= 1 + body = f"ok from {backend._port}\n".encode() + self.send_response(200) + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, *args): + pass + + self._server = http.server.ThreadingHTTPServer( + ("127.0.0.1", port), Handler) + self._thread = threading.Thread( + target=self._server.serve_forever, daemon=True) + + def start(self): + self._thread.start() + + def stop(self): + self._server.shutdown() + self._thread.join(timeout=5) + + @property + def port(self): + return self._port + + @property + def max_active(self): + with self._lock: + return self._max_active + + def reset(self): + with self._lock: + self._max_active = 0 + + +class TestProxyMaxBusy: + + @pytest.fixture(scope='class') + def backend_a(self, env): + b = SlowBackend(env.http_port + 100, delay=1.5) + b.start() + yield b + b.stop() + + @pytest.fixture(scope='class') + def backend_b(self, env): + b = SlowBackend(env.http_port + 101, delay=1.5) + b.start() + yield b + b.stop() + + @pytest.fixture(autouse=True, scope='class') + def _class_scope(self, env, backend_a, backend_b): + conf = HttpdConf(env) + conf.add([ + f"LoadModule lbmethod_bytraffic_module " + f"\"{env.prefix}/modules/mod_lbmethod_bytraffic.so\"", + f"LoadModule lbmethod_bybusyness_module " + f"\"{env.prefix}/modules/mod_lbmethod_bybusyness.so\"", + "StartServers 4", + "MinSpareThreads 4", + "MaxSpareThreads 8", + "MaxRequestWorkers 20", + "ServerLimit 20", + # single-worker balancer: basic hard-cap behaviour + "", + f" BalancerMember http://127.0.0.1:{backend_a.port} maxbusy=2", + "", + "ProxyPass /maxbusy balancer://maxbusy-cluster", + # two-worker balancer, heavily skewed loadfactor: proves + # failover routes around a saturated worker rather than + # 503'ing while spare capacity exists elsewhere + "", + f" BalancerMember http://127.0.0.1:{backend_a.port} maxbusy=1 loadfactor=100", + f" BalancerMember http://127.0.0.1:{backend_b.port} maxbusy=1 loadfactor=1", + " ProxySet lbmethod=byrequests", + "", + "ProxyPass /maxbusy-br balancer://maxbusy-byrequests", + "", + f" BalancerMember http://127.0.0.1:{backend_a.port} maxbusy=1 loadfactor=100", + f" BalancerMember http://127.0.0.1:{backend_b.port} maxbusy=1 loadfactor=1", + " ProxySet lbmethod=bytraffic", + "", + "ProxyPass /maxbusy-bt balancer://maxbusy-bytraffic", + "", + f" BalancerMember http://127.0.0.1:{backend_a.port} maxbusy=1", + f" BalancerMember http://127.0.0.1:{backend_b.port} maxbusy=1", + " ProxySet lbmethod=bybusyness", + "", + "ProxyPass /maxbusy-bb balancer://maxbusy-bybusyness", + # two-worker balancer with a Balancer Timeout, for the + # all-saturated cases + "", + f" BalancerMember http://127.0.0.1:{backend_a.port} maxbusy=1", + f" BalancerMember http://127.0.0.1:{backend_b.port} maxbusy=1", + " ProxySet lbmethod=byrequests timeout=10", + "", + "ProxyPass /maxbusy-to balancer://maxbusy-timeout", + # two-worker balancer with no Balancer Timeout: immediate 503 + "", + f" BalancerMember http://127.0.0.1:{backend_a.port} maxbusy=1", + f" BalancerMember http://127.0.0.1:{backend_b.port} maxbusy=1", + " ProxySet lbmethod=byrequests", + "", + "ProxyPass /maxbusy-noto balancer://maxbusy-notimeout", + # sticky balancer, nofailover On and Off (two paths so both + # can be exercised without changing config mid-test) + "", + f" BalancerMember http://127.0.0.1:{backend_a.port} route=1 maxbusy=1", + f" BalancerMember http://127.0.0.1:{backend_b.port} route=2 maxbusy=1", + " ProxySet stickysession=ROUTEID nofailover=On", + "", + "ProxyPass /maxbusy-sticky-on balancer://maxbusy-sticky-on", + "", + f" BalancerMember http://127.0.0.1:{backend_a.port} route=1 maxbusy=1", + f" BalancerMember http://127.0.0.1:{backend_b.port} route=2 maxbusy=1", + " ProxySet stickysession=ROUTEID nofailover=Off", + "", + "ProxyPass /maxbusy-sticky-off balancer://maxbusy-sticky-off", + # a balancer with no maxbusy set at all: must behave exactly + # like trunk (unlimited concurrency) + "", + f" BalancerMember http://127.0.0.1:{backend_a.port}", + "", + "ProxyPass /nomaxbusy balancer://nomaxbusy-cluster", + # a worker with no backend listening yet, for the + # release-on-connection-failure test; the backend is + # started (and later stopped) by that test itself, config + # is never touched again so the balancers above stay live + "", + " BalancerMember http://127.0.0.1:18599 maxbusy=1 retry=1", + "", + "ProxyPass /maxbusy-dead balancer://maxbusy-dead", + # worker A refuses connections outright (nothing listens on + # 18598); worker B is healthy but slow. A single request must + # fail over A -> B, and the regression under test is whether + # A's maxbusy reservation is released the instant the + # failover is taken, rather than leaking for as long as B's + # (unrelated, successful) attempt takes to complete. + "", + " BalancerMember http://127.0.0.1:18598 maxbusy=1 " + "loadfactor=100 retry=600", + f" BalancerMember http://127.0.0.1:{backend_a.port} maxbusy=1 " + f"loadfactor=1", + " ProxySet lbmethod=byrequests", + "", + "ProxyPass /maxbusy-leak balancer://maxbusy-failover-leak", + # three-worker chain: A and B refuse connections outright, C + # is healthy; a request must fail over twice (A -> B -> C) + # without leaking a reservation on either abandoned worker. + "", + " BalancerMember http://127.0.0.1:18597 maxbusy=1 " + "loadfactor=100 retry=600", + " BalancerMember http://127.0.0.1:18596 maxbusy=1 " + "loadfactor=100 retry=600", + " BalancerMember http://127.0.0.1:18595 maxbusy=1 " + "loadfactor=1", + " ProxySet lbmethod=byrequests", + "", + "ProxyPass /maxbusy-chain balancer://maxbusy-chain", + "", + " SetHandler balancer-manager", + " Require all granted", + "", + ]) + conf.install() + assert env.apache_restart() == 0 + + # ----------------------------------------------------------------- + # 1. Global hard cap across multiple child processes + # ----------------------------------------------------------------- + def test_proxy_07_001_global_hard_cap(self, env, backend_a): + backend_a.reset() + url = f"http://localhost:{env.http_port}/maxbusy/x" + with ThreadPoolExecutor(max_workers=6) as pool: + results = list(pool.map(lambda _: env.curl_get(url), range(6))) + statuses = sorted(r.response["status"] for r in results) + assert statuses == [200, 200, 503, 503, 503, 503], statuses + assert backend_a.max_active <= 2, \ + f"observed concurrency {backend_a.max_active} > maxbusy=2" + env.httpd_error_log.ignore_recent( + matches=[r'.*is at its MaxBusy limit.*', + r'.*All workers are in error state.*']) + + def test_proxy_07_002_no_leak_after_burst(self, env, backend_a): + # after the limit-exceeding burst above, the worker must not be + # left with a leaked reservation: a fresh request must succeed. + url = f"http://localhost:{env.http_port}/maxbusy/x" + r = env.curl_get(url) + assert r.response["status"] == 200 + + def test_proxy_07_003_release_on_client_abort(self, env, backend_a): + # a request that is aborted by the client before the backend + # responds must not leak its reservation either. + backend_a.reset() + url = f"http://localhost:{env.http_port}/maxbusy/x" + with ThreadPoolExecutor(max_workers=1) as pool: + fut = pool.submit(env.curl_get, url) + time.sleep(0.2) + aborted = env.curl_raw([url], options=["--max-time", "0.2"]) + assert aborted.exit_code != 0 + r1 = fut.result() + assert r1.response["status"] == 200 + time.sleep(0.5) + r2 = env.curl_get(url) + assert r2.response["status"] == 200 + + def test_proxy_07_004_release_on_backend_failure(self, env): + # a worker whose backend connection fails must not leave a + # leaked maxbusy reservation behind either: bring up a fresh, + # working backend on the port the (still-dead, per class setup) + # worker points to, and confirm a later request succeeds once + # the worker's retry interval has elapsed. The config is not + # touched here, so the balancers set up for the rest of the + # class stay live. + dead_port = 18599 + url = f"http://localhost:{env.http_port}/maxbusy-dead/x" + r1 = env.curl_get(url) + assert r1.response["status"] in (502, 503) + env.httpd_error_log.ignore_recent( + matches=[r'.*failed to make connection to backend.*', + r'.*disabling worker.*', + r'.*attempt to connect.*failed.*']) + backend = SlowBackend(dead_port, delay=0.1) + backend.start() + try: + time.sleep(1.5) + r2 = env.curl_get(url) + assert r2.response["status"] == 200 + finally: + backend.stop() + + # ----------------------------------------------------------------- + # Confirmed bug: proxy_handler() in mod_proxy.c retries a failed + # worker with a fresh ap_proxy_pre_request() call, but (before the + # fix) only released the abandoned worker's maxbusy reservation when + # the whole request's pool was destroyed -- i.e. only after the + # eventual successful worker finished, not the instant the failover + # was taken. A worker refusing connections would therefore keep + # counting as busy for as long as the request takes to complete + # elsewhere, able to artificially exhaust its own maxbusy while + # genuinely idle. + # ----------------------------------------------------------------- + def _worker_reserved(self, env, balancer, port): + """Scrape for the worker at `port` out of the + machine-readable XML balancer-manager produces with ?xml. A + Referer matching the vhost is required, or balancer-manager + discards all query parameters as a cross-site request and + silently returns the full default HTML listing instead.""" + base = f"http://localhost:{env.http_port}" + url = f"{base}/balancer-manager?b={balancer}&xml" + r = env.curl_get(url, options=["-H", f"Referer: {base}/"]) + assert r.response["status"] == 200 + body = r.response["body"].decode() + worker_re = re.compile( + r".*?%d.*?" + r"(\d+).*?" + % port, re.DOTALL) + # Scope to this balancer's own block first: the + # XML dump lists every configured balancer regardless of `b=`, + # and a worker URL/port is not necessarily unique across + # balancers in this test's config (backend_a is reused by more + # than one), so searching the whole document could match a + # same-port worker belonging to a different, irrelevant balancer. + balancer_re = re.compile( + r"\s*balancer://%s" + r"(.*?)" % re.escape(balancer), re.DOTALL) + bm = balancer_re.search(body) + assert bm, f"balancer {balancer} not found in {url}: {body}" + m = worker_re.search(bm.group(1)) + assert m, f"worker on port {port} not found in balancer " \ + f"{balancer}'s block: {bm.group(1)}" + return int(m.group(1)) + + def test_proxy_07_012_no_leak_during_failover(self, env, backend_a): + backend_a.reset() + dead_port = 18598 + url = f"http://localhost:{env.http_port}/maxbusy-leak/x" + + with ThreadPoolExecutor(max_workers=1) as pool: + fut = pool.submit(env.curl_get, url) + # give the request time to: hit the dead worker, fail over, + # and be in the middle of backend_a's 1.5s delayed response. + time.sleep(0.7) + reserved_dead = self._worker_reserved( + env, "maxbusy-failover-leak", dead_port) + reserved_live = self._worker_reserved( + env, "maxbusy-failover-leak", backend_a.port) + r = fut.result() + + assert r.response["status"] == 200, r.response + assert reserved_live == 1, \ + f"expected the live worker to hold the reservation while " \ + f"active, got {reserved_live}" + assert reserved_dead == 0, \ + f"maxbusy reservation leaked on the abandoned worker during " \ + f"failover: reserved={reserved_dead}, expected 0 while the " \ + f"request was still being served by the other worker" + + # and after completion, neither side has anything outstanding + assert self._worker_reserved( + env, "maxbusy-failover-leak", dead_port) == 0 + assert self._worker_reserved( + env, "maxbusy-failover-leak", backend_a.port) == 0 + env.httpd_error_log.ignore_recent( + matches=[r'.*failed to make connection to backend.*', + r'.*disabling worker.*', + r'.*attempt to connect.*failed.*']) + + def test_proxy_07_013_failover_chain_no_leak(self, env): + # A and B both refuse connections, C is healthy and slow: the + # request must fail over twice, and only C may hold a reservation + # while it is active; A and B must be at 0 throughout, not just + # at the end. + dead_a, dead_b = 18597, 18596 + c = SlowBackend(18595, delay=1.5) + c.start() + try: + url = f"http://localhost:{env.http_port}/maxbusy-chain/x" + with ThreadPoolExecutor(max_workers=1) as pool: + fut = pool.submit(env.curl_get, url) + time.sleep(0.7) + r_a = self._worker_reserved(env, "maxbusy-chain", dead_a) + r_b = self._worker_reserved(env, "maxbusy-chain", dead_b) + r_c = self._worker_reserved(env, "maxbusy-chain", 18595) + result = fut.result() + assert result.response["status"] == 200, result.response + assert (r_a, r_b, r_c) == (0, 0, 1), (r_a, r_b, r_c) + assert self._worker_reserved(env, "maxbusy-chain", dead_a) == 0 + assert self._worker_reserved(env, "maxbusy-chain", dead_b) == 0 + assert self._worker_reserved(env, "maxbusy-chain", 18595) == 0 + finally: + c.stop() + env.httpd_error_log.ignore_recent( + matches=[r'.*failed to make connection to backend.*', + r'.*disabling worker.*', + r'.*attempt to connect.*failed.*']) + + # ----------------------------------------------------------------- + # 2/3/4. byrequests / bytraffic / bybusyness: saturated A, available + # B -> B is used, despite A's much higher loadfactor. + # ----------------------------------------------------------------- + @pytest.mark.parametrize("path, lbmethod", [ + ("maxbusy-br", "byrequests"), + ("maxbusy-bt", "bytraffic"), + ]) + def test_proxy_07_005_failover_skips_saturated(self, env, backend_a, + backend_b, path, lbmethod): + backend_a.reset() + backend_b.reset() + url = f"http://localhost:{env.http_port}/{path}/x" + hold_url = f"http://localhost:{env.http_port}/{path}/2" + with ThreadPoolExecutor(max_workers=1) as pool: + # saturate the heavily-weighted worker A + fut = pool.submit(env.curl_get, hold_url) + time.sleep(0.3) + # despite A's overwhelming loadfactor, new requests must be + # routed to B, which has spare capacity + r = env.curl_get(f"http://localhost:{env.http_port}/{path}/0.1") + assert r.response["status"] == 200, \ + f"{lbmethod}: expected failover to the spare worker, got {r.response['status']}" + assert fut.result().response["status"] == 200 + assert backend_a.max_active <= 1 + assert backend_b.max_active <= 1 + + def test_proxy_07_006_bybusyness_runtime_reset_hard_cap( + self, env, backend_a, backend_b): + # The regression this patch fixes: triggering a real runtime + # need_reset (via a balancer-manager lbmethod change) while + # requests are active against bybusyness workers must NOT allow + # the observed backend concurrency to exceed maxbusy, even + # though the reset zeroes the unrelated 'busy' LB metric. + backend_a.reset() + backend_b.reset() + base = f"http://localhost:{env.http_port}" + nonce = env.get_balancer_manager_nonce( + "maxbusy-bybusyness") if hasattr( + env, "get_balancer_manager_nonce") else None + if nonce is None: + r = env.curl_get(f"{base}/balancer-manager?b=maxbusy-bybusyness") + m = re.search(r"nonce=([a-f0-9-]{36})", r.response["body"].decode()) + assert m, "could not find balancer-manager nonce" + nonce = m.group(1) + + with ThreadPoolExecutor(max_workers=4) as pool: + # saturate both workers (maxbusy=1 each) + futs = [pool.submit(env.curl_get, f"{base}/maxbusy-bb/3") + for _ in range(2)] + time.sleep(0.5) + + # flip the lbmethod away and back: each change sets + # need_reset, and bybusyness.reset() zeroes 'busy' for every + # worker in the balancer the next time the balancer is + # synced, while the two requests above are still active. + # Use urllib directly (not the curl wrapper) so the Referer + # header is guaranteed to be sent, and verify from the + # response body that the lbmethod change was actually + # applied (not silently dropped as a cross-site request), + # since a dropped POST would make this test pass vacuously. + import urllib.request + for target in ("byrequests", "bybusyness"): + req = urllib.request.Request( + f"{base}/balancer-manager", + data=f"b=maxbusy-bybusyness&nonce={nonce}&b_lbm={target}".encode(), + headers={"Referer": f"{base}/", + "Content-Type": "application/x-www-form-urlencoded"}, + method="POST") + with urllib.request.urlopen(req, timeout=10) as resp: + assert resp.status == 200 + body = resp.read().decode() + assert f"{target}" in body, \ + (f"b_lbm={target} was not applied (dropped as " + f"cross-site?): {body}") + + # immediately fire more requests; if the hard cap were only + # enforced via the (now reset) 'busy' counter, these would + # be wrongly admitted on top of the two still-active ones. + extra = [pool.submit(env.curl_get, f"{base}/maxbusy-bb/0.2") + for _ in range(4)] + extra_statuses = sorted(f.result().response["status"] + for f in extra) + assert extra_statuses == [503, 503, 503, 503], extra_statuses + + orig_statuses = sorted(f.result().response["status"] + for f in futs) + assert orig_statuses == [200, 200], orig_statuses + + assert backend_a.max_active <= 1, \ + f"bybusyness reset race: observed {backend_a.max_active} > maxbusy=1" + assert backend_b.max_active <= 1, \ + f"bybusyness reset race: observed {backend_b.max_active} > maxbusy=1" + env.httpd_error_log.ignore_recent( + matches=[r'.*is at its MaxBusy limit.*', + r'.*All workers are in error state.*', + r'.*could not reserve a worker.*', + r'.*ignoring params in balancer-manager.*']) + + # ----------------------------------------------------------------- + # 6/7. All workers saturated + # ----------------------------------------------------------------- + def test_proxy_07_007_all_saturated_no_timeout_503( + self, env, backend_a, backend_b): + backend_a.reset() + backend_b.reset() + base = f"http://localhost:{env.http_port}/maxbusy-noto" + with ThreadPoolExecutor(max_workers=2) as pool: + futs = [pool.submit(env.curl_get, f"{base}/2") for _ in range(2)] + time.sleep(0.3) + t0 = time.time() + r = env.curl_get(f"{base}/0.1") + elapsed = time.time() - t0 + assert r.response["status"] == 503 + assert elapsed < 1.0, \ + f"expected an immediate 503 with no Balancer Timeout, took {elapsed}s" + for f in futs: + assert f.result().response["status"] == 200 + env.httpd_error_log.ignore_recent( + matches=[r'.*All workers are in error state.*']) + + def test_proxy_07_008_all_saturated_timeout_waits_and_succeeds( + self, env, backend_a, backend_b): + backend_a.reset() + backend_b.reset() + base = f"http://localhost:{env.http_port}/maxbusy-to" + with ThreadPoolExecutor(max_workers=3) as pool: + futs = [pool.submit(env.curl_get, f"{base}/4") for _ in range(2)] + time.sleep(0.3) + t0 = time.time() + r = env.curl_get(f"{base}/0.1") + elapsed = time.time() - t0 + assert r.response["status"] == 200, \ + "expected the waiting request to succeed once a slot freed" + assert elapsed > 2.0, \ + f"expected the request to actually wait for capacity, took {elapsed}s" + for f in futs: + assert f.result().response["status"] == 200 + + # ----------------------------------------------------------------- + # 8/9. Sticky sessions + # ----------------------------------------------------------------- + def test_proxy_07_009_sticky_nofailover_on_503( + self, env, backend_a, backend_b): + backend_a.reset() + backend_b.reset() + base = f"http://localhost:{env.http_port}/maxbusy-sticky-on" + with ThreadPoolExecutor(max_workers=1) as pool: + fut = pool.submit( + env.curl_raw, [f"{base}/2"], + options=["-H", "Cookie: ROUTEID=x.1"]) + time.sleep(0.3) + r = env.curl_raw([f"{base}/0.1"], + options=["-H", "Cookie: ROUTEID=x.1"]) + assert r.response["status"] == 503 + assert fut.result().response["status"] == 200 + env.httpd_error_log.ignore_recent( + matches=[r'.*All workers are in error state.*']) + + def test_proxy_07_010_sticky_nofailover_off_failover( + self, env, backend_a, backend_b): + backend_a.reset() + backend_b.reset() + base = f"http://localhost:{env.http_port}/maxbusy-sticky-off" + with ThreadPoolExecutor(max_workers=1) as pool: + fut = pool.submit( + env.curl_raw, [f"{base}/2"], + options=["-H", "Cookie: ROUTEID=x.1"]) + time.sleep(0.3) + r = env.curl_raw([f"{base}/0.1"], + options=["-H", "Cookie: ROUTEID=x.1"]) + assert r.response["status"] == 200, \ + "expected failover to the other worker, not a 503" + assert fut.result().response["status"] == 200 + assert backend_a.max_active <= 1 + assert backend_b.max_active <= 1 + + # ----------------------------------------------------------------- + # 15. maxbusy unset: behaves exactly like trunk (unlimited) + # ----------------------------------------------------------------- + def test_proxy_07_011_unset_is_unlimited(self, env, backend_a): + backend_a.reset() + url = f"http://localhost:{env.http_port}/nomaxbusy/x" + with ThreadPoolExecutor(max_workers=6) as pool: + results = list(pool.map(lambda _: env.curl_get(url), range(6))) + statuses = [r.response["status"] for r in results] + assert statuses == [200] * 6, statuses + + +class TestProxyMaxBusyIdentity: + """Admission-table identity correctness: a BalancerMember's maxbusy + reservation count must be scoped per (server/vhost, balancer, worker), + not merely per worker URL or per textual balancer name, and must not + be allocated merely by observing a worker (balancer-manager, or any + other read of its current reservation count). + """ + + @pytest.fixture(scope='class') + def backend_shared(self, env): + b = SlowBackend(env.http_port + 120, delay=1.5) + b.start() + yield b + b.stop() + + @pytest.fixture(scope='class') + def backend_unlimited(self, env): + b = SlowBackend(env.http_port + 121, delay=0.2) + b.start() + yield b + b.stop() + + @pytest.fixture(autouse=True, scope='class') + def _class_scope(self, env, backend_shared, backend_unlimited): + vhost_a_port = env.http_port + 130 + vhost_b_port = env.http_port + 131 + conf = HttpdConf(env) + lines = [ + f"Listen 127.0.0.1:{vhost_a_port}", + f"Listen 127.0.0.1:{vhost_b_port}", + f"", + " ServerName vhost-a.example.org", + # same balancer name, same backend URL as vhost B below, but + # a DIFFERENT maxbusy: these must not share one counter. + " ", + f" BalancerMember http://127.0.0.1:{backend_shared.port} maxbusy=1", + " ", + " ProxyPass /app balancer://app", + "", + f"", + " ServerName vhost-b.example.org", + " ", + f" BalancerMember http://127.0.0.1:{backend_shared.port} maxbusy=2", + " ", + " ProxyPass /app balancer://app", + "", + ] + # Item 2: more than 64 configured BalancerMembers with maxbusy + # set, to size the admission table beyond its 64-slot floor + # (under the pre-fix code, which sized the table from the empty + # conf->workers array of direct ProxyPass workers, this would + # have silently relied on that floor instead of actual demand). + # Each gets its own tiny backend-less worker: none are ever + # requested, so no retry/connection-refused noise, this is only + # about every one of them getting an independent, working slot. + many = [] + for i in range(80): + many.append(f"") + many.append( + f" BalancerMember http://127.0.0.1:19{700 + i} " + f"maxbusy=1 disablereuse=on") + many.append("") + many.append(f"ProxyPass /many-{i} balancer://many-{i}") + lines.extend(many) + # Item 5: many maxbusy=0 (unlimited) workers, queried repeatedly + # via balancer-manager; none of them may consume an admission + # slot merely by being looked up. + lines.append("") + lines.append( + f" BalancerMember http://127.0.0.1:{backend_unlimited.port}") + lines.append("") + lines.append("ProxyPass /unlimited balancer://unlimited") + lines.append("") + lines.append(" SetHandler balancer-manager") + lines.append(" Require all granted") + lines.append("") + conf.add(lines) + conf.install() + assert env.apache_restart() == 0 + # A class-scoped fixture gets a fresh `self` per test method, so + # instance attributes set here would not be visible there: store + # on the class itself instead. + type(self).vhost_a_port = vhost_a_port + type(self).vhost_b_port = vhost_b_port + + # ----------------------------------------------------------------- + # Item 2: a large number of BalancerMembers must not exhaust the + # admission table merely because the (wrong, pre-fix) sizing saw + # zero direct ProxyPass workers; startup itself succeeding already + # proves ap_proxy_admission_table_check() was satisfied, and the + # cluster configured above (maxbusy=1 each) getting its own slot + # proves the table has room beyond the 64-slot floor. + # ----------------------------------------------------------------- + def test_proxy_07_101_many_balancer_members_each_get_a_slot(self, env): + # The httpd instance is already up (fixture asserted restart==0 + # with 80+ maxbusy BalancerMembers configured, none of which + # could be registered without ap_proxy_admission_table_check() + # passing: under the pre-fix sizing, which counted zero direct + # ProxyPass workers for this all-BalancerMember config and so + # sized the table at its 16*4=64-slot floor, more than 64 + # distinct maxbusy'd members would eventually report "admission + # table is full" (item 6's log line) once enough of them had + # actually been requested to populate it past 64 entries. + # + # None of these members has a real backend, so a *request* to + # one deterministically gets a connection-refused 503 from + # mod_proxy_http regardless of admission state (confirmed via + # error_log: "AH00957: ... Connection refused"), which would + # make a response-code assertion here ambiguous. Instead, send + # one request to each of several of them (enough to populate + # that many slots) and assert the admission table's own + # "is full" diagnostic never appears. + for i in range(70): + env.curl_get(f"http://127.0.0.1:{self.vhost_a_port}/many-{i}/x", + options=["--max-time", "1"]) + with open(env.httpd_error_log.path) as fd: + log = fd.read() + assert "admission table is full" not in log, \ + "admission table ran out of slots for 70 distinct " \ + "BalancerMembers -- sizing did not account for them" + env.httpd_error_log.ignore_recent( + matches=[r'.*Connection refused.*', + r'.*disabling worker.*', + r'.*failed to make connection to backend.*']) + + # ----------------------------------------------------------------- + # Item 4: same balancer name + same backend URL, in two different + # vhosts, with two different maxbusy values -- saturating one must + # not consume the other's reservations. + # ----------------------------------------------------------------- + def test_proxy_07_102_cross_vhost_identity_independent(self, env): + url_a = f"http://127.0.0.1:{self.vhost_a_port}/app/x" + url_b = f"http://127.0.0.1:{self.vhost_b_port}/app/x" + + # Saturate vhost A's balancer (maxbusy=1): one request held busy. + with ThreadPoolExecutor(max_workers=1) as pool: + fut_a_hold = pool.submit(env.curl_get, url_a) + time.sleep(0.3) # let it register as active before probing + r_a_second = env.curl_get(url_a, options=["--max-time", "3"]) + assert r_a_second.response["status"] == 503, \ + "vhost A's own second request should be rejected (maxbusy=1)" + + # vhost B (maxbusy=2) must be entirely unaffected: both of + # its own requests succeed concurrently, proving its counter + # is independent of vhost A's, not aliased by the shared + # balancer name/backend URL. + with ThreadPoolExecutor(max_workers=2) as pool_b: + futs_b = [pool_b.submit(env.curl_get, url_b, + options=["--max-time", "3"]) + for _ in range(2)] + statuses_b = sorted(f.result().response["status"] + for f in futs_b) + assert statuses_b == [200, 200], \ + f"vhost B rejected a request while only vhost A was saturated: {statuses_b}" + + fut_a_hold.result() + env.httpd_error_log.ignore_recent( + matches=[r'.*is at its MaxBusy limit.*', + r'.*could not reserve a worker.*', + r'.*All workers are in error state.*']) + + # ----------------------------------------------------------------- + # Item 5: a read-only lookup (balancer-manager rendering a worker's + # reservation count) must never allocate an admission slot, and a + # maxbusy=0 worker must never consume one merely by being observed. + # ----------------------------------------------------------------- + def test_proxy_07_103_readonly_lookup_does_not_allocate(self, env): + mgr_url = f"http://127.0.0.1:{self.vhost_a_port}/balancer-manager" + for _ in range(20): + r = env.curl_get(mgr_url) + assert r.response["status"] == 200 + # The unlimited (maxbusy=0) worker, rendered 20 times above via + # balancer-manager (which reads its reservation count), must + # still behave as genuinely unlimited: 5 real concurrent + # requests all succeed against its own (real) backend. If + # rendering it had allocated a slot for it, that alone would not + # break this assertion (maxbusy=0 never checks the table either + # way, see ap_proxy_try_reserve_worker()) -- the point is this + # exercises the read path at all without the server logging any + # problem. + with ThreadPoolExecutor(max_workers=5) as pool: + results = list(pool.map( + lambda _: env.curl_get( + f"http://127.0.0.1:{self.vhost_a_port}/unlimited/x"), + range(5))) + statuses = [r.response["status"] for r in results] + assert statuses == [200] * 5, statuses + with open(env.httpd_error_log.path) as fd: + log = fd.read() + assert "admission table is full" not in log, \ + "balancer-manager reads of a maxbusy=0 worker appear to " \ + "have consumed admission-table slots" + env.httpd_error_log.ignore_recent( + matches=[r'.*ignoring params in balancer-manager.*'])