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

Refund unused rate limit on downloads #17417

Closed
wants to merge 3 commits into from
Closed

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Jul 8, 2024

Fixes #17394 (I think/hope)

A more complete fix likely involves the use of a Linearizer over the download endpoint, limiting the number of concurrent requests to ~6 from any one client. We would then only count against the requester's bucket once we know the size for sure instead of decrementing by max_size. Instead, this PR tries to refund the difference back to the rate limit, allowing the user to request more media at the cost of being rate limited if they have several (assumed) large concurrent requests on the go. The hope is this is uncommon, and that downloads complete quick enough to refund the bucket.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct
    (run the linters)

@@ -0,0 +1 @@
Relax rate limiting behaviour on federated media downloads of unknown size.
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not confident enough to say the issue is fully fixed, but it should be better for RC/release purposes.

@turt2live
Copy link
Member Author

Closing in favour of #17439

@turt2live turt2live closed this Aug 7, 2024
@turt2live turt2live deleted the travis/fix-beeper-media branch August 7, 2024 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remote media fetches fail from beeper.com since 1.110.0rc1
1 participant