implement in-memory limiter - #820
Open
Erik-Bard wants to merge 2 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Type
Feature
PR Checklist
cargo +nightly fmt).Overview
actix-limitationcurrently hard-wires Redis:Limiterowns aredis::Client,Builderholds aredis_url: String, and there is no way to rate limit without a running server. That cost shows upin this repo —
actix-limitationis excluded from the macOS and Windows CI jobs entirely, becauseits tests need a Redis service container.
This adds an opt-in
memory-storefeature providing a process-local fixed-window counter with thesame semantics as the Redis path, so local development, examples and tests need no Redis.
This is fully non-breaking.
redisstays a required dependency and the default, unchanged path.No existing public API is removed, gated, or altered.
New public API
MemoryStoreisClone + Default + Debug; clones share one set of counters. Counters expire on alazy sweep that runs on a write at most once a minute — there is no background task. The map is
unbounded by default; with
max_keysset the store fails open once full (new keys passuntracked, with a rate-limited warning) rather than evicting counters out from under clients already
being tracked.
Constructing a store logs one
WARNat startup, so an in-memory store shipped to production byaccident is visible in the logs without per-request noise.
Internal refactor
Limiter::track's Redis pipeline moved intosrc/store/redis.rs, unchanged apart from droppingtrailing comments that restated the Redis command next to them.
Limiternow holds a privatebackend: Backendenum andtrackis a two-arm dispatch.Builderholds a privateBackendSpec,so
Client::openstill happens inbuild()and a malformed URL is still anErrthere.Notes for review
Three decisions worth flagging up front, since each was made against a real alternative:
Private enum, no public
Storetrait. KeepsLimiternon-generic, so the middleware'stype-erased
app_data::<web::Data<Limiter>>()lookup is untouched. Third parties can't addbackends; that seemed the right trade for not changing the middleware's shape.
The two backends are deliberately co-compilable — no
compile_error!on the combination.Using two backends on one
Limiteris already unrepresentable (one enum variant, one constructoreach, no
Buildermethod to swap it), so an exclusion would guard nothing. It would, however,break
[package.metadata.docs.rs] all-features = true, forceactix-limitationonto the--all-featuresexclusion lists alongsidetracing-actix-webandactix-settings, and — worst —make downstream graphs unbuildable through feature unification when two unrelated crates each pick
a different backend. The reasoning is recorded in the
MemoryStorerustdoc so it isn't latermistaken for an oversight.
redisstays a required dependency. Gating it behind a default-onredis-storefeature wouldslim memory-only builds, but it is breaking for
default-features = falseconsumers andadditionally requires cfg-gating
Error::Client, the middleware match arm and thestatic_assertionsblock. Happy to do it as a follow-up on its own major bump if you'd preferthat direction — say the word and I'll fold it in here instead.
Tests, CI and docs
tests/memory.rs— 6 integration tests, no external service: counting,Error::LimitExceeded, two limiters sharing one store, and three end-to-end middleware runsthrough
test::init_service(limiting, unkeyed passthrough, window reset). The store's ownsemantics — fixed non-sliding window, key independence, sweep eviction, fail-open at
max_keys—are unit tested in
src/store/memory.rs..github/workflows/ci.yml—build_and_test_othergains a step running just those tests:This is the crate's first cross-platform test coverage; the Redis exclusion stays as-is.
tests/tests.rs→tests/redis.rs, to name what it actually needs. Its one bare#[test]on anasync fnis now an explicit#[actix_web::test]— behaviour is unchanged (the bare attributewas already resolving to
actix_web::testvia theuseon line 4), but the implicit shadowing iseasy to misread and would break confusingly if
testwere dropped from that import.Two examples:
memory(runs with nothing installed) andredis(same app, one line different atthe builder). Only
memorygets an[[example]]stanza, since only it hasrequired-features.The self-referencing dev-dependency follows the pattern at
actix-session/Cargo.toml:44, so theintegration tests need no
#[cfg]juggling:Drive-by doc fix:
Builder::build's comment claimed it "will connect to the Redis server to testits connection which is a synchronous operation". It doesn't —
Client::openonly parses theURL.
README gains a Backends section, and the stale
actix-limitation = "0.5"snippet is corrected to0.6. The cratedescriptionandkeywordsnow mention the in-memory backend.Verification
Also confirmed by hand with Redis stopped:
The process-local warning fires exactly once at startup.