Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi thread check & update #203

Closed
wants to merge 7 commits into from
Closed

Conversation

didierofrivia
Copy link
Contributor

@didierofrivia didierofrivia commented Aug 24, 2023

This PR is yet another step closer to proper multi thread Limitador.

However, the changes introduced makes de check_and_update "racy", since different requests will check and update at the same time:

  1. It could/will happen that one thread will load a counter value at the "check" phase and at the same time, a different thread will store a value at the "update" phase to the same counter.
  2. Some counters will end up being updated while others won't if one ends up being limited in the update phase.
  3. Those competing threads might end limiting a bit late (overshooting).

Regarding the bench, there's a regression of 3% but it's kind of a lie, since we are not benching for multi-threading requests.

@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2023

Codecov Report

Merging #203 (5d94409) into main (490826c) will increase coverage by 0.51%.
Report is 6 commits behind head on main.
The diff coverage is 98.98%.

@@            Coverage Diff             @@
##             main     #203      +/-   ##
==========================================
+ Coverage   73.46%   73.98%   +0.51%     
==========================================
  Files          30       30              
  Lines        5058     5124      +66     
==========================================
+ Hits         3716     3791      +75     
+ Misses       1342     1333       -9     
Files Changed Coverage Δ
limitador/src/storage/in_memory.rs 89.60% <98.98%> (+7.29%) ⬆️

... and 5 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@didierofrivia didierofrivia marked this pull request as ready for review September 4, 2023 09:12
@didierofrivia didierofrivia marked this pull request as draft October 9, 2023 17:42
@alexsnaps alexsnaps added this to the Limitador v0.6.0 milestone Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants