apache / apache/trafficserver

rate_limit: a queue without a size defaults to UINT32_MAX entries

Open
#13,511 0 comments 0 reactions 1 assignee Claimed by @moonchen View on GitHub
Plugins rate_limit
Dominant language
C++
Stars
2k
Forks
874
Avg merge
6d 15h
Merged PRs (30d)
46

Description

In `plugins/experimental/rate_limit/limiter.h`, `RateLimiter::parseYaml()`:

```cpp
// 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_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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.