udoprog / leaky-bucket

A token-based rate limiter based on the leaky bucket algorithm.
Apache License 2.0
92 stars 10 forks source link

Cannot create two rate limiters in the same app #5

Closed xnuter closed 4 years ago

xnuter commented 4 years ago

I'm trying to create different rate limiters, and it doesn't seem to work. E.g. a simple test:

#[tokio::test]
    async fn test_limited_frequent() {
        let rate_limiter = LeakyBucket::builder()
            .refill_amount(1)
            .refill_interval(Duration::from_millis(100))
            .build()
            .expect("LeakyBucket builder failed");
        let begin = Instant::now();
        for _ in 0..10 {
            rate_limiter.acquire_one().await.expect("No reason to fail");
        }
        let elapsed = Instant::now().duration_since(begin);
        println!("Elapsed: {:?}", elapsed);
        assert!((elapsed.as_secs_f64() - 1.).abs() < 0.1);
    }

    #[tokio::test]
    async fn test_limited_seldom() {
        let rate_limiter = LeakyBucket::builder()
            .refill_amount(1)
            .refill_interval(Duration::from_secs(2))
            .build()
            .expect("LeakyBucket builder failed");
        let begin = Instant::now();
        for _ in 0..2 {
            rate_limiter.acquire_one().await.expect("No reason to fail");
        }
        let elapsed = Instant::now().duration_since(begin);
        println!("Elapsed: {:?}", elapsed);
        // once per 2 seconds => 4 seconds for 2 permits
        assert!((elapsed.as_secs_f64() - 4.).abs() < 0.1);
    }

Each one passes if run independently. But if you run both, then the first one finishes successfully while the second one just hangs forever.

udoprog commented 4 years ago

Thanks! I'll check it out.

udoprog commented 4 years ago

So I'm like 90% sure this is because the static feature (LeakyBucket::builder() relies on) uses whatever runtime is available to spawn the coordinator background thread, and each test case annotated with tokio::test uses a separate runtime. Once the runtime is dropped, the coordinator task is dropped with it.

There's two readily available solutions:

At the very least I'll add a description to the documentation that this is a potential footgun with the static feature for now.

udoprog commented 4 years ago

Added your code using an explicit coordinator as a test case to #6.

xnuter commented 4 years ago

Thanks for the quick turn around!

BTW, if in my app I want to create and destroy LeakyBucket rate limiters, is there a way to tear it down? Or should I use the explicit coordinator approach, or it's OK to use the static one?

udoprog commented 4 years ago

Thanks for the quick turn around!

BTW, if in my app I want to create and destroy LeakyBucket rate limiters, is there a way to tear it down? Or should I use the explicit coordinator approach, or it's OK to use the static one?

You should use the explicit coordinator, and spawn it within the runtime as appropriate. I've added documentation to hopefully explain this, and released 0.8.0 with it along with some other changes.

Thanks for the report!

xnuter commented 4 years ago

Okay, I finally integrated the library and seems to work:

https://github.com/xnuter/perf-gauge/blob/2204689a29fc7bb6133372d952f5438ac0e525ef/src/rate_limiter.rs#L42

e.g. I can run a load test with an increasing ladder. But if I upgrade the library to 0.8 it panics:

$ perf-gauge -r 100 -d 5s http http://localhost:8088/10kb --conn_reuse 
thread 'tokio-runtime-worker' panicked at 'new task queue ended', /Users/xnuter/.cargo/registry/src/github.com-1ecc6299db9ec823/leaky-bucket-0.8.0/src/lib.rs:200:67
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'tokio-runtime-worker' panicked at 'Unexpected LeakyBucket.acquire error: "Failed to send task to coordinator"', src/bench_run.rs:96:18

P.S. BTW, it seems that the coordinator never exits, even on dropping the rate limiter, as these log messages are never printed: https://github.com/xnuter/perf-gauge/blob/2204689a29fc7bb6133372d952f5438ac0e525ef/src/rate_limiter.rs#L45-L50

udoprog commented 4 years ago

Ah, yeah. The coordinator was never designed to shut down 😄 . But now I see that's indeed possible since the coordinator task sheds the LeakyBucketsInner.

I've added some code to handle this in #7. I think this should fix it. Thanks again!

udoprog commented 4 years ago

Released in 0.8.1. Hope it helps!

xnuter commented 4 years ago

Great, thanks for the quick response! Now it works and I can see that the coordinator gracefully exits on dropping the rate limiter. But now a new issue, the rate is wrong. E.g. the same rate 2,000 per second works correctly in 0.7 but in 0.8 it never goes above 200 rps, no matter what rate I specify:

0.7:

➜  perf-gauge git:(master) ✗ cargo build --release && ./target/release/perf-gauge -c 2 -r 2000 -d 10s http http://localhost:8088/ --conn_reuse
Duration 10.002314489s 
Requests: 19978 
Request rate: 1997.338 per second
Success rate: 100.000%
Total bytes: 199.8 MB 
Bitrate: 159.787 Mbps

0.8.1:

➜  perf-gauge git:(master) ✗ cargo build --release && ./target/release/perf-gauge -c 2 -r 2000 -d 10s http http://localhost:8088/ --conn_reuse
Duration 10.002537332s 
Requests: 2000 
Request rate: 199.949 per second
Success rate: 100.000%
Total bytes: 20.0 MB 
Bitrate: 15.996 Mbps
udoprog commented 4 years ago

@xnuter moved to #9