Skip to content

Commit

Permalink
Do a major refactor of the library (#9)
Browse files Browse the repository at this point in the history
* Do a major refactor of the library

I think there was a significant complexity in the implementation
of the library. I get that we tried to aim to reuse some code
for sync and async implementation but it made the hard to follow,
and I couldn't really tell what was public API and what was not, behind
these long inheritance chains.

I also believe that the way the public API is structured was wrong.
Due to the way the code was structured, we had to duplicate some
docstrings & code in various places.

So, I changed the public API significantly and made it similar
to the TypeScript implementation, where we only export the Ratelimit
class, which you can configure with different rate limiters. That
API sounds much more natural to me and makes it simple to declare &
document what is public.

I have also changed some parameter names, from things like
max_number_of_tokens to max_tokens, to make it more idiomatic.

Also, I have renamed metadata methods like remaining and reset
to get_remaining and get_reset. Without the get prefix, I believe
the names of these methods were confusing.

I have also updated the README and examples with these changes.

Also, I have improved the tests. There were no tests for the sliding
window, and existing tests were failing from time to time. I tried
to make them more robust.

I have also enabled the mypy checks, after fixing the issues.

I also found the return value of the reset method in case the
identifier not rate limited confusing. The value of -1 can
cause problems if the docs were not read carefully.

Lastly, I have changed the rate limit algorithms we had slightly, as
they had some bugs.

For sliding window:
- The previous window calculation was wrong. After quantizing the
current time and determining the current window, the previous window
is current window - 1, not current window - window duration.
- The return value for the remaining tokens was wrong. It was
not taking the number of requests * weight from the previous window
into account. Because of that, it was returning a remaining token
count larger than the actual value.

For token bucket:
- The bucket was not evicted at all. That was essentially a data leak.
The implementation in the TypeScript implementation is also wrong,
it is evicting the token in fixed intervals no matter what. What
we need is not TTL but max idle with the time equal to the number
of intervals required to fill the bucket to max tokens. Then,
after this interval, the bucket can be safely evicted as after
that point never evicting the bucket and recreating the bucket
becomes the same thing: Bucket always includes token equal
to the max tokens. Since redis does not have max idle feature
for keys, we have to implement it with TTL by resetting the
TTL each time we access the key.

* do not use generic generator type for python 3.8 compatibility

* address review comments

* fix mypy errors

* relax the test
  • Loading branch information
mdumandag authored Jul 10, 2023
1 parent 2ac3981 commit a72444f
Show file tree
Hide file tree
Showing 67 changed files with 2,140 additions and 2,225 deletions.
7 changes: 2 additions & 5 deletions .github/workflows/release.yaml
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
name: Release

on:
push:
branches:
- main
on: workflow_dispatch

jobs:
release:
Expand All @@ -23,7 +20,7 @@ jobs:

- name: Build and publish
run: |
poetry config pypi-token.pypi "${{secrets.PYPI_TOKEN}}"
poetry config pypi-token.pypi "${{ secrets.PYPI_TOKEN }}"
poetry build
poetry publish --no-interaction
Expand Down
8 changes: 6 additions & 2 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,13 @@ jobs:
run: |
poetry cache clear PyPI --all
poetry install --no-root
- name: Run mypy
run: |
poetry run mypy --show-error-codes .
- name: Run tests
run: |
export UPSTASH_REDIS_REST_URL="${{secrets.UPSTASH_REDIS_REST_URL}}"
export UPSTASH_REDIS_REST_TOKEN="${{secrets.UPSTASH_REDIS_REST_TOKEN}}"
export UPSTASH_REDIS_REST_URL="${{ secrets.UPSTASH_REDIS_REST_URL }}"
export UPSTASH_REDIS_REST_TOKEN="${{ secrets.UPSTASH_REDIS_REST_TOKEN }}"
poetry run pytest
4 changes: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -159,4 +159,6 @@ cython_debug/
# option (not recommended) you can uncomment the following to ignore the entire idea folder.
#.idea/

.DS_Store
.DS_Store

.vscode
Loading

0 comments on commit a72444f

Please sign in to comment.