Merge #12

12: Fix off-by-one error in check_all's capacity check r=antifuchs a=antifuchs

This fixes #11:

I'd missed that the "weight" variable is _additional weight_ that gets
added to the first cell, so check_all (when given one more element
than fit in the burst capacity) would return a "denied" result instead
of an "insufficient capacity" result - effectively preventing futures
from ever completing.

Now, we treat additional weight as what it is:
* rename "weight" to "additional_weight" to make it clear what is
  going on
* Add a comment over the capacity check, clarifying the off-by-one
  error potential
* Add a test about for more capacity-excession scenarios, including
  the off-by-one, to prevent regressions.

Thanks to @jean-airoldie for discovering the bug & reporting it!

Co-authored-by: Andreas Fuchs <[email protected]>

Author: bors[bot]

.circleci/config.yml

@@ -1,4 +1,4 @@
-# TemplateCIConfig { bench: BenchEntry(MatrixEntry { run: true, run_cron: false, version: "stable", install_commandline: None, commandline: "cargo bench" }), clippy: ClippyEntry(MatrixEntry { run: true, run_cron: false, version: "stable", install_commandline: Some("rustup component add clippy"), commandline: "cargo clippy -- -D warnings" }), rustfmt: RustfmtEntry(MatrixEntry { run: true, run_cron: false, version: "stable", install_commandline: Some("rustup component add rustfmt"), commandline: "cargo fmt -v -- --check" }), additional_matrix_entries: {"cargo_deny": CustomEntry(MatrixEntry { run: true, run_cron: false, version: "stable", install_commandline: Some("cargo install cargo-deny"), commandline: "cargo deny check all" }), "no_std_stable": CustomEntry(MatrixEntry { run: true, run_cron: false, version: "stable", install_commandline: None, commandline: "cargo test --no-default-features --features no_std" }), "no_std_nightly": CustomEntry(MatrixEntry { run: true, run_cron: false, version: "nightly", install_commandline: None, commandline: "cargo +nightly test --no-default-features --features no_std" })}, cache: "cargo", os: "linux", dist: "xenial", versions: ["stable", "nightly"], test_commandline: "cargo test --verbose --all", scheduled_test_branches: ["master"], test_schedule: "0 0 * * 0" }
+# TemplateCIConfig { bench: BenchEntry(MatrixEntry { run: true, run_cron: false, version: "nightly", install_commandline: None, commandline: "cargo bench", timeout: None }), clippy: ClippyEntry(MatrixEntry { run: true, run_cron: false, version: "stable", install_commandline: Some("rustup component add clippy"), commandline: "cargo clippy -- -D warnings", timeout: None }), rustfmt: RustfmtEntry(MatrixEntry { run: true, run_cron: false, version: "stable", install_commandline: Some("rustup component add rustfmt"), commandline: "cargo fmt -v -- --check", timeout: None }), additional_matrix_entries: {"cargo_deny": CustomEntry(MatrixEntry { run: true, run_cron: false, version: "stable", install_commandline: Some("cargo install cargo-deny"), commandline: "cargo deny check all", timeout: None }), "no_std_nightly": CustomEntry(MatrixEntry { run: true, run_cron: false, version: "nightly", install_commandline: None, commandline: "cargo +nightly test --no-default-features --features no_std", timeout: None }), "no_std_stable": CustomEntry(MatrixEntry { run: true, run_cron: false, version: "stable", install_commandline: None, commandline: "cargo test --no-default-features --features no_std", timeout: None })}, cache: "cargo", os: "linux", dist: "xenial", versions: ["stable", "nightly"], test_commandline: "cargo test --verbose --all", scheduled_test_branches: ["master"], test_schedule: "0 0 * * 0" }
 version: "2.1"
 
 executors:
@@ -95,7 +95,7 @@ jobs:
       - run:
           name: cargo deny check all
           command: cargo deny check all
-  no_std_stable:
+  no_std_nightly:
     parameters:
       version:
         type: executor
@@ -107,9 +107,9 @@ jobs:
     steps:
       - checkout
       - run:
-          name: cargo test --no-default-features --features no_std
-          command: cargo test --no-default-features --features no_std
-  no_std_nightly:
+          name: cargo +nightly test --no-default-features --features no_std
+          command: cargo +nightly test --no-default-features --features no_std
+  no_std_stable:
     parameters:
       version:
         type: executor
@@ -121,8 +121,8 @@ jobs:
     steps:
       - checkout
       - run:
-          name: cargo +nightly test --no-default-features --features no_std
-          command: cargo +nightly test --no-default-features --features no_std
+          name: cargo test --no-default-features --features no_std
+          command: cargo test --no-default-features --features no_std
 
   ci_success:
     docker:
@@ -196,7 +196,7 @@ workflows:
   }
 }
       - bench:
-          version: stable
+          version: nightly
           filters: {
   "branches": {
     "ignore": [
@@ -213,14 +213,14 @@ workflows:
           name: "cargo_deny"
           version: stable
           version_name: stable
-      - no_std_stable:
-          name: "no_std_stable"
-          version: stable
-          version_name: stable
       - no_std_nightly:
           name: "no_std_nightly"
           version: nightly
           version_name: nightly
+      - no_std_stable:
+          name: "no_std_stable"
+          version: stable
+          version_name: stable
       - ci_success:
           requires:
           - test-stable
@@ -229,8 +229,8 @@ workflows:
           - clippy
           - bench
           - cargo_deny
-          - no_std_stable
           - no_std_nightly
+          - no_std_stable
   scheduled_tests:
     jobs:
       - test:

Cargo.toml

@@ -26,7 +26,7 @@ dev-version-ext = "dev"
 
 [package.metadata.template_ci.bench]
 run = true
-version = "stable"
+version = "nightly"
 
 [package.metadata.template_ci.additional_matrix_entries]
 

src/gcra.rs

@@ -113,15 +113,18 @@ impl GCRA {
         let t0 = t0.duration_since(start);
         let tau = self.tau;
         let t = self.t;
-        let weight = t * (n.get() - 1) as u64;
-        if weight > tau {
+        let additional_weight = t * (n.get() - 1) as u64;
+
+        // check that we can allow enough cells through. Note that `additional_weight` is the
+        // value of the cells *in addition* to the first cell - so add that first cell back.
+        if additional_weight + t > tau {
             return Err(NegativeMultiDecision::InsufficientCapacity(
                 (tau.as_u64() / t.as_u64()) as u32,
             ));
         }
         state.measure_and_replace(key, |tat| {
             let tat = tat.unwrap_or_else(|| self.starting_state(t0));
-            let earliest_time = (tat + weight).saturating_sub(tau);
+            let earliest_time = (tat + additional_weight).saturating_sub(tau);
             if t0 < earliest_time {
                 Err(NegativeMultiDecision::BatchNonConforming(
                     n.get(),
@@ -132,7 +135,7 @@ impl GCRA {
                     },
                 ))
             } else {
-                Ok(((), cmp::max(tat, t0) + t + weight))
+                Ok(((), cmp::max(tat, t0) + t + additional_weight))
             }
         })
     }

tests/direct.rs

@@ -116,13 +116,25 @@ fn rejects_too_many_all() {
     // After 3 and 20 seconds, it should not allow 15 on that bucket either:
     clock.advance(ms * 3 * 1000);
     assert_ne!(Ok(()), lb.check_all(nonzero!(15u32)));
+}
 
-    clock.advance(ms * 17 * 1000);
-    let result = lb.check_all(nonzero!(15u32));
-    match result {
-        Err(NegativeMultiDecision::InsufficientCapacity(n)) => assert_eq!(n, 5),
-        _ => panic!("Did not expect {:?}", result),
-    }
+#[test]
+fn all_capacity_check_rejects_excess() {
+    let clock = FakeRelativeClock::default();
+    let lb = RateLimiter::direct_with_clock(Quota::per_second(nonzero!(5u32)), &clock);
+
+    assert_eq!(
+        Err(NegativeMultiDecision::InsufficientCapacity(5)),
+        lb.check_all(nonzero!(15u32))
+    );
+    assert_eq!(
+        Err(NegativeMultiDecision::InsufficientCapacity(5)),
+        lb.check_all(nonzero!(6u32))
+    );
+    assert_eq!(
+        Err(NegativeMultiDecision::InsufficientCapacity(5)),
+        lb.check_all(nonzero!(7u32))
+    );
 }
 
 #[test]