Skip to content

[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

Merged
merged 1 commit into from
Aug 19, 2022

Conversation

bobvandevijver
Copy link
Contributor

@bobvandevijver bobvandevijver commented May 11, 2021

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #36808
License MIT
Doc PR -

This PR adds the possibility to add a simple, transport based, rate limiter (as defined with the RateLimiter component) to the Messenger worker.

@carsonbot
Copy link

Hey!

I think @tienvx has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@kamil-karkus
Copy link

Any update on this?

@fabpot fabpot modified the milestones: 5.4, 6.1 Oct 30, 2021
@fritzmg
Copy link
Contributor

fritzmg commented Jan 6, 2022

👍 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.

@bobvandevijver bobvandevijver force-pushed the 36808-messenger-ratelimiter branch from 5a4e0e1 to 98c48cb Compare January 6, 2022 14:55
@bobvandevijver
Copy link
Contributor Author

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.

@mtaylor567
Copy link

any news on this?

@carsonbot carsonbot changed the title Add simple transport based rate limiter to Messenger [Messenger] Add simple transport based rate limiter to Messenger Feb 21, 2022
Copy link
Member

@nicolas-grekas nicolas-grekas left a 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.

@bobvandevijver bobvandevijver force-pushed the 36808-messenger-ratelimiter branch 2 times, most recently from 9626d5f to 8bdeac3 Compare February 28, 2022 09:20
@bobvandevijver bobvandevijver requested review from nicolas-grekas and removed request for sroze February 28, 2022 11:22
@bobvandevijver
Copy link
Contributor Author

bobvandevijver commented Feb 28, 2022

@nicolas-grekas Requested adjustments have been made, and I added two tests.

(the failed checks do not seem to be related to this change)

@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
@fabpot
Copy link
Member

fabpot commented Aug 19, 2022

Thank you @bobvandevijver.

@fabpot fabpot force-pushed the 36808-messenger-ratelimiter branch from 6e479f2 to 29a2585 Compare August 19, 2022 07:38
@fabpot fabpot merged commit 4c438af into symfony:6.2 Aug 19, 2022
@bobvandevijver bobvandevijver deleted the 36808-messenger-ratelimiter branch August 19, 2022 08:33
@fabpot fabpot mentioned this pull request Oct 24, 2022
@gavinerickson
Copy link

gavinerickson commented Nov 26, 2022

@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.

framework:
  rate_limiter:
    email_output_limit:
      #Office 365 email limit is 30 mails per minute (per account)
      policy: 'fixed_window'
      limit: 1
      interval: '2 seconds'

messenger config:

Email_Job:
    dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
    failure_transport: failed_output
    rate_limiter: email_output_limit
    options:
        queues:
            email_job:
                arguments:
                    x-max-priority: 10
        exchange:
            name: email_job
            type: direct
    retry_strategy:
        max_retries: 3
        multiplier: 2

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

###> symfony/lock ###
# Choose one of the stores below
LOCK_DSN='sqlite:///%kernel.project_dir%/var/lock.db'
###

This is all running on windows server 2019 😞
I've also tried token_bucket and mysql. Is it correct not to see a value in the db for tokens when the consumers are running? I assumed there would be an entry of some sort, but seems like an empty table

@bobvandevijver
Copy link
Contributor Author

@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 DoctrineDbal lock store does not support blocking locks (see this table). Maybe you could try whether it works using a Flock based store, as you're using a local based lock anyways looking at the DSN you shared?

Otherwise I would recommend opening an issue (you can tag me with it) to discuss the issue further.

@gavinerickson
Copy link

@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.

OskarStark added a commit to symfony/symfony-docs that referenced this pull request Dec 14, 2023
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support delay in Mailer when using Messenger
8 participants