-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] Add simple transport based rate limiter to Messenger #41171
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
[Messenger] Add simple transport based rate limiter to Messenger #41171
Conversation
b097444
to
5a4e0e1
Compare
Hey! I think @tienvx has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
Any update on this? |
👍 on rate limiting for messenger transports 🙂 When using the Messenger component in order to asynchronously send emails via the Mailer component being able to limit the total amount of emails per time unit would be really helpful in hosting environments that impose a limit on how many emails you can send via their SMTP per hour or day for instance. |
5a4e0e1
to
98c48cb
Compare
I just rebased the changed on the latest 6.1 branch, but I'm really looking for some input on whether these changes are actually going in the right direction (and will be accepted) before I will be spending some time on tests. |
any news on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM past minor details. Please rebase also to resolve the conflict.
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Messenger/Event/WorkerRateLimitedEvent.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.php
Outdated
Show resolved
Hide resolved
9626d5f
to
8bdeac3
Compare
@nicolas-grekas Requested adjustments have been made, and I added two tests. (the failed checks do not seem to be related to this change) |
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
08ebb0f
to
6e479f2
Compare
Thank you @bobvandevijver. |
6e479f2
to
29a2585
Compare
@bobvandevijver I'm trying out the messenger/rate limiter integration in 6.2 RC1, it seems like I have a race condition when consuming a rate limited queue (RabbitMq) with multiple consumers. In the setup below, instead of 1 email every 2 secs, I get multiple emails/sec processed.
messenger config:
I have a lock setup as below, maybe this is the issue? Or perhaps there is some relation between the number of workers and the limits to be applied
This is all running on windows server 2019 😞 |
@gavinerickson I'm no expect on the implementation of the rate limiter/lock components, but I believe you might be using the wrong lock store for what you're looking as the Otherwise I would recommend opening an issue (you can tag me with it) to discuss the issue further. |
@bobvandevijver I'd tried flock initially too (and just sanity checked) - that's what I've used with rate limiter previously. I'll open an issue. |
…ijver) This PR was submitted for the 6.2 branch but it was squashed and merged into the 6.3 branch instead. Discussion ---------- [Messenger] Add messenger rate_limiter docs Documentation for symfony/symfony#41171 Commits ------- ec2c155 [Messenger] Add messenger rate_limiter docs
This PR adds the possibility to add a simple, transport based, rate limiter (as defined with the RateLimiter component) to the Messenger worker.