Need to get the last sleepFor in the loop. #27
Closed
Conversation
This file contains 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
|
|
|
cannot limit anymore as you code |
rabbbit
added a commit
that referenced
this pull request
Oct 4, 2020
We currently have two different implementations. Currently: - they're in the same package - one of them is currently not tested - one has a bug related to slack (see #27, #23) - one has a broken constructor taking options affecting the other type We plan to add at least 3 more implementations: - one that addresses #22 - one that re-uses ratelimitfx atomic style - one that compares us against x/time/rate Even if we decide to eventually keep a single implementation, they'll be there for a while. And I imagine it'll get noisy. (A crazy idea would be to return different implementation based on some user params. But that's totally TBD). This diff tries to organize us a bit my moving the separate implementations to separate packages. Followups: - will try to refactor the tests into a shared test package & re-use them. This way we can write tests once and make sure new implementations are working as expected. - will try to make sure the implementations take equivalent configuration - will have to modify the root ratelimit package to have stronger type guarantees, and not just aliasing methods - that would require a separate /types package though (I think). Accidentally this diff is also changing signature of AfterFunc in internal/clock package. This is so that it implements it's own Clock interface. Looking at #3 though, I think we can simply drop AfterFunc from the interface in followup diffs, and re-use Clock from the root ratelimit package. (We currently have two Clock interfaces defined, we can merge them into a single one using a /types package trick) in a follow up.
rabbbit
added a commit
that referenced
this pull request
Dec 6, 2020
The ovarall goal is to be able to run the same tests on all possible implementations so that we can verify they're working. After #42 we can now configure limiters with the same options. After this diff, the tests only pass in those options in to the testRunner. A following diff will modify testRunner so that all tests are executed on both atomic & mutex based limiters. This in turn allows us to change the limiters in sync & unblocks resolving #27 & #23 (they are about the same bug) This diff also - changes the test to run in the same package. This is just so that I can access `option` without making it public, which seems unnecessary. - renames runner into testRunner since it appears the benchmarks already define a `runner`. We might be able to merge them together but that's for another diff.
rabbbit
added a commit
that referenced
this pull request
Dec 7, 2020
The ovarall goal is to be able to run the same tests on all possible implementations so that we can verify they're working. After #42 we can now configure limiters with the same options. After this diff, the tests only pass in those options in to the testRunner. A following diff will modify testRunner so that all tests are executed on both atomic & mutex based limiters. This in turn allows us to change the limiters in sync & unblocks resolving #27 & #23 (they are about the same bug) This diff also - changes the test to run in the same package. This is just so that I can access `option` without making it public, which seems unnecessary. - renames runner into testRunner since it appears the benchmarks already define a `runner`. We might be able to merge them together but that's for another diff.
rabbbit
added a commit
that referenced
this pull request
Dec 14, 2020
The ovarall goal is to be able to run the same tests on all possible implementations so that we can verify they're working. After #42 we can now configure limiters with the same options. After this diff, the tests only pass in those options in to the testRunner. A following diff will modify testRunner so that all tests are executed on both atomic & mutex based limiters. This in turn allows us to change the limiters in sync & unblocks resolving #27 & #23 (they are about the same bug) This diff also - changes the test to run in the same package. This is just so that I can access `option` without making it public, which seems unnecessary. - renames runner into testRunner since it appears the benchmarks already define a `runner`. We might be able to merge them together but that's for another diff.
rabbbit
added a commit
that referenced
this pull request
Jan 3, 2021
As reported in #23 and #27 slack is currently broken in the atomic implementation. Both #23 and #27 propose valid solutions, and I actually arrived at #23 when trying to solve this independently, but I find #27 slightly more readable/understandable, so going with that. Also, #23 seems to expose another bug in andres-erbsen/clock, with sleeping with negative time, so we should probably consider switching to benbjohnson/clock - maybe in a follow up.
Merged
rabbbit
added a commit
that referenced
this pull request
Jan 19, 2021
As reported in #23 and #27 slack is currently broken in the atomic implementation. Both #23 and #27 propose valid solutions, and I actually arrived at #23 when trying to solve this independently, but I find #27 slightly more readable/understandable, so going with that. Also, #23 seems to expose another bug in andres-erbsen/clock, with sleeping with negative time, so we should probably consider switching to benbjohnson/clock - maybe in a follow up.
rabbbit
added a commit
that referenced
this pull request
Jan 25, 2021
* Fix not working slack. As reported in #23 and #27 slack is currently broken in the atomic implementation. Both #23 and #27 propose valid solutions, and I actually arrived at #23 when trying to solve this independently, but I find #27 slightly more readable/understandable, so going with that. Also, #23 seems to expose another bug in andres-erbsen/clock, with sleeping with negative time, so we should probably consider switching to benbjohnson/clock - maybe in a follow up.
|
Solved in #60 - using your idea, but with tests/in a different file. |
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.
Need to get the last sleepFor in the loop, or slack will have no effect.