In plugins/experimental/rate_limit/limiter.h, RateLimiter::parseYaml():
// ToDo: One or both of these should be required
const YAML::Node &queue = node["queue"];
// If enabled, we default to UINT32_MAX, but the object default is still 0 (no queue)
if (queue) {
_max_queue = queue["size"] ? queue["size"].as<uint32_t>() : UINT32_MAX;
A queue: block without a size: sets the cap to UINT32_MAX. full() is
_size >= max_queue(), so it can never be true and the admission path never
rejects; every over-limit connection is queued. The docs state the same at
doc/admin-guide/plugins/rate_limit.en.rst:215: "The size is default to
UINT_MAX, which is essentially unlimited."
Two problems follow.
Resource holding. For the SNI limiter a queued entry is a suspended TLS
handshake, so it holds a socket, a NetVConnection, an SSL, and the handshake
buffers. A parked connection has its read VIO disabled, so the core cannot
notice the peer went away; nothing reaps it but
proxy.config.ssl.handshake_timeout_in, 30s by default. The effective ceiling
is proxy.config.net.connections_throttle, 30000 by default, each held for up
to 30s. For a plugin whose purpose is to bound resource consumption under load,
that is inverted.
Cost of removal. RateLimiter::remove() is linear in the queue depth, and
because _queue is a std::deque an erase from the middle is O(n) in element
moves however the element is located.
Suggested fix: require the user to specify a queue size when a queue: block is specified. Consider a different data structure for the queue.
In
plugins/experimental/rate_limit/limiter.h,RateLimiter::parseYaml():A
queue:block without asize:sets the cap toUINT32_MAX.full()is_size >= max_queue(), so it can never be true and the admission path neverrejects; every over-limit connection is queued. The docs state the same at
doc/admin-guide/plugins/rate_limit.en.rst:215: "The size is default toUINT_MAX, which is essentially unlimited."Two problems follow.
Resource holding. For the SNI limiter a queued entry is a suspended TLS
handshake, so it holds a socket, a
NetVConnection, anSSL, and the handshakebuffers. A parked connection has its read VIO disabled, so the core cannot
notice the peer went away; nothing reaps it but
proxy.config.ssl.handshake_timeout_in, 30s by default. The effective ceilingis
proxy.config.net.connections_throttle, 30000 by default, each held for upto 30s. For a plugin whose purpose is to bound resource consumption under load,
that is inverted.
Cost of removal.
RateLimiter::remove()is linear in the queue depth, andbecause
_queueis astd::dequean erase from the middle is O(n) in elementmoves however the element is located.
Suggested fix: require the user to specify a queue size when a queue: block is specified. Consider a different data structure for the queue.