Preview decision and remove individual keys

[jmfrank63]

Hi Andreas,

This might be a weird PR, but I have a use case for it. I am rate-limiting login failures, and thus would like to know in advance before I send a request to the rather costly authentication, what the decision would be, so in case of a failure I reject right away. I considered wrapping first, but that would have meant to deal with the whole rate limiter for an individual decision. Also, I wanted to have the ability to remove a key from the rate limiter, if some condition happens. The use case here is a user login. If it fails too often it gets rate limited, but if it succeeds it is reset.

I am a rather average coder, so I basically duplicated the code. But I am happy to take suggestions. Two things come to mind: Call check_key for peek_check_key instead of duplicating the code. Use fetch_and_update instead of the compare_exchange_weak while loop.

I added more tests, as I intend to use this code in production. Please let me know what you think.

[antifuchs]

I meant to do this, as well! Thanks so much for the PR (and sorry for not getting to reviewing it&the previous PR earlier - work combined with some sickness). I think having a peek method is great & promised to write one in some other issues (#131 at least!), so this is much appreciated (:

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (83345b1) 98.15% compared to head (f715e04) 98.60%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #146 +/- ## ========================================== + Coverage 98.15% 98.60% +0.45% ========================================== Files 31 31 Lines 2225 2949 +724 ========================================== + Hits 2184 2908 +724 Misses 41 41 ``` | [Files](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs) | Coverage Δ | | |---|---|---| | [governor/src/clock/quanta.rs](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs#diff-Z292ZXJub3Ivc3JjL2Nsb2NrL3F1YW50YS5ycw==) | `98.50% <100.00%> (-0.03%)` | :arrow_down: | | [governor/src/clock/with\_std.rs](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs#diff-Z292ZXJub3Ivc3JjL2Nsb2NrL3dpdGhfc3RkLnJz) | `100.00% <100.00%> (ø)` | | | [governor/src/errors.rs](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs#diff-Z292ZXJub3Ivc3JjL2Vycm9ycy5ycw==) | `100.00% <100.00%> (ø)` | | | [governor/src/gcra.rs](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs#diff-Z292ZXJub3Ivc3JjL2djcmEucnM=) | `100.00% <100.00%> (ø)` | | | [governor/src/jitter.rs](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs#diff-Z292ZXJub3Ivc3JjL2ppdHRlci5ycw==) | `100.00% <100.00%> (ø)` | | | [governor/src/state.rs](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs#diff-Z292ZXJub3Ivc3JjL3N0YXRlLnJz) | `100.00% <100.00%> (ø)` | | | [governor/src/state/direct.rs](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs#diff-Z292ZXJub3Ivc3JjL3N0YXRlL2RpcmVjdC5ycw==) | `100.00% <100.00%> (ø)` | | | [governor/src/state/direct/future.rs](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs#diff-Z292ZXJub3Ivc3JjL3N0YXRlL2RpcmVjdC9mdXR1cmUucnM=) | `100.00% <100.00%> (ø)` | | | [governor/src/state/in\_memory.rs](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs#diff-Z292ZXJub3Ivc3JjL3N0YXRlL2luX21lbW9yeS5ycw==) | `100.00% <100.00%> (ø)` | | | [governor/src/state/keyed.rs](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs#diff-Z292ZXJub3Ivc3JjL3N0YXRlL2tleWVkLnJz) | `100.00% <100.00%> (ø)` | | | ... and [8 more](https://app.codecov.io/gh/antifuchs/governor/pull/146?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andreas+Fuchs) | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jmfrank63]

I rebased but since I first had tried to resolve the conflicts via the web GUI (an awful idea) rebasing was a bit messy. I hope I didn't break anything, at least the test suite passed.

[jmfrank63]

@antifuchs Hi Andreas, I think I could need some help here. My tests are failing if no standard library is used. Could you please give me a hint on what to do?

[jmfrank63]

@antifuchs OK, I found the issue and fixed it. I added more tests to get the pipeline "green" as well as increasing code coverage slightly. Since we are using the preview in our code, we currently have a fork hosted, which we have to keep in sync. Therefore, I have a strong interest to get back to an external reference of the crate. Please let me know if there is anything else for me to do.

[jmfrank63]

@antifuchs Happy new year, Andreas. I just wanted to ask if there is anything else I could do. I don't seem to have any control over bors, so if you could have a look, that would be great.

Thanks, Johannes

[antifuchs]

Sorry for taking so long with this. I'm coming into a good bit of spare time soon, so I'll review this in depth ASAP!

[Nutomic]

I also need this. Would be great if it could be merged soon, because there seems to be no other Rust crate with this functionality.

[jmfrank63]

Hi @antifuchs,

I was just revisiting this, it does not seem to need a rebase. Is there any other work you want me to do to get this merged? I also saw, others seem to be interested in this, too.

Thanks, Johannes

[jmfrank63]

This does look great! Besides a rebase, I highlighted a few items that still need work (mostly coverage-related, which will prevent a merge... but also missing docs on public methods).

I have owed you this review for a long time, thanks for picking up work on this change again!

@antifuchs Hi Andreas,

thank you so much for this opportunity. I think I have addressed all the issues. I also factored out two tests in their own functions and added one. For the collision, I changed the current test, it now succeeds really fast, I had successes with 4 retries. I have also removed the flaky timing tests on the proceed functions, all that is necessary is the fact that they proceed, not how fast. If you don't like anything, just let me know, I'll change this. If you know how to get the coverage report, like when it is failing, give me a hint. I couldn't find it :-) Thanks again Johannes

[jmfrank63]

Hi Andreas,

Finally, full coverage, since I found the coverage report again. I also found that the M1 bug is now fixed, so I activated this test as well. I think you will decide on the version, so I left the version untouched.

[jmfrank63]

@antifuchs Hi Andreas, I am not sure if I got all your concerns. The long time in between visiting this accumulated a lot of changes. I hope I have addressed all of them. While I am not at the company I used the code for actively in production, I care about my former colleagues. Currently, they have to maintain the fork. It would be nice if they could just return to a simple cargo dependency. If I should do additional work on this, let me know. Squashing might be an option, but this often makes commenting difficult, so I only squash on request as merging does squash anyway.

[jmfrank63]

@antifuchs Hi Andreas, I saw you are preparing for the next release. I would be happy if my PR could make it in there. I have increased my code coverage and resolved all issues I could find. All tests passed. Please let me know if I should address anything else. Thanks, Johannes

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (f0cb09c) 98.25% compared to head (ae82c95) 98.71%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #146 +/- ## ========================================== + Coverage 98.25% 98.71% +0.45% ========================================== Files 31 31 Lines 2182 2957 +775 ========================================== + Hits 2144 2919 +775 Misses 38 38 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[jmfrank63]

@antifuchs Dear Andreas, Happy new year 2024. I've rebased to reflect the latest changes, but I cannot do this forever. I am not invested into this any more as I have left the company, but for my colleagues it is painful to keep a local copy of an earlier version. I have added additional tests to increase coverage and have, to my best ability, addressed all concerns. Therefore, I kindly ask you to consider merging now, so we can close this PR. You might want to review again, that's fine, I will try to address additional concerns ASAP.

Thanks, Johannes