Skip to content

New rate limiting mechanism #3148

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

Merged
merged 46 commits into from
Aug 26, 2022
Merged

New rate limiting mechanism #3148

merged 46 commits into from
Aug 26, 2022

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Jul 8, 2022

closes #3018

Some comments:

  • I went with the aiolimiter library despite New Mechanism for Avoiding Flood Limits #3018 (comment). AFAIK the performance problems are only relevant for very high load and the built-in implementation is only supposed to be a minimal effort reference implementation …
  • I introduced aiolimiter as optional requirement, since using the AIORateLimiter is purely opt-in. This design goes in the direction that I'd like to discuss in the context of https://github.com/python-telegram-bot/python-telegram-bot/projects/7#card-77767364
  • I did not add rate_imit_args to shortcut methods. IMO that would add too much of a connection between tg and tg.ext and if users want to pass rate_limit_args they can always use the standard methods instead.

Checklist for PRs

  • Added .. versionadded:: version, .. versionchanged:: version or .. deprecated:: version to the docstrings for user facing changes (for methods/class descriptions, arguments and attributes)
  • Created new or adapted existing unit tests
  • Documented code changes according to the CSI standard
  • Added myself alphabetically to AUTHORS.rst (optional)
  • Added new classes & modules to the docs and all suitable __all__ s

@Bibo-Joshi Bibo-Joshi added enhancement 🛠 refactor change type: refactor labels Jul 8, 2022
@Bibo-Joshi Bibo-Joshi added this to the v20.0a3 milestone Jul 8, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Looks like you edited the requirements or the pre-commit hooks. I'm just a friendly reminder to keep the additional dependencies for the hooks in sync with the requirements :)

# Conflicts:
#	.github/workflows/pre-commit_dependencies_notifier.yml
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Looks like you edited README.rst or README_RAW.rst. I'm just a friendly reminder to apply relevant changes to both of those files :)

@Bibo-Joshi
Copy link
Member Author

I'll have to try & check how I can get the tests to work on unix. otherwise, this ready for a first round of review imo

@Bibo-Joshi Bibo-Joshi marked this pull request as ready for review July 14, 2022 05:53
@Bibo-Joshi

This comment was marked as off-topic.

@Bibo-Joshi
Copy link
Member Author

got tests to work on ubuntu. Skipped them on macOS, b/c I don't think that it's really worth the effort 😬

@harshil21 harshil21 self-requested a review August 2, 2022 20:04
@bharel
Copy link

bharel commented Aug 3, 2022

@Bibo-Joshi @harshil21 I created an asyncio limiter that doesn't have the httpx issue. In fact, the issue doesn't have anything to do with httpx at all. The interface is similar although a tad different, but it is considerably more efficient. You're welcome to test it and tell me if it's better for you :-)

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally we have a working rate limiter! Didn't test out the changes locally yet. Also have a gut feeling that there's probably a bug here and there which I didn't catch

Copy link
Member

@harshil21 harshil21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much awaited addition!

@Bibo-Joshi Bibo-Joshi merged commit 741a50a into master Aug 26, 2022
@Bibo-Joshi Bibo-Joshi deleted the rate-limiter branch August 26, 2022 04:50
@github-actions github-actions bot locked and limited conversation to collaborators Sep 2, 2022
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🔌 enhancement pr description: enhancement 🛠 refactor change type: refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Mechanism for Avoiding Flood Limits
3 participants