Allow mailer classes to customize the deliver_later queue name #47408
Conversation
The `deliver_later_queue_name` is already configurable on ActionMailer::Base, however the value is inherited by all subclasses. Use a class-inheritable attribute instead, so that subclasses can override. Refs: - rails#18587 (comment)
packagethief
force-pushed
the
jh/action-mailer-deliver-later-queue
branch
from
45a8c44
to
fa17c9a
Compare
|
|
||
| included do | ||
| class_attribute :delivery_job, default: ::ActionMailer::MailDeliveryJob | ||
| class_attribute :deliver_later_queue_name, default: :mailers |
There was a problem hiding this comment.
@packagethief shouldn't this be default: -> { ActionMailer::Base.deliver_later_queue_name }? Since Rails 6.1 mailers queue is nil / :default instead of :mailers.
There was a problem hiding this comment.
This is defining ActionMailer::Base.deliver_later_queue_name so it can't have a default of itself.
Good catch though @javierav. I'd missed that in the 6.1+ configuration defaults, the value is being set to nil:
rails/railties/lib/rails/application/configuration.rb
Lines 212 to 214 in c15093c
The effect is the same – we changed from a cattr_accessor to a class_attribute, but both had the same default of :mailers. In configurations < 6.1 this will be respected, and in later configurations nillified.
But it means the documentation I added about the :mailers name in this PR is wrong, since that won't be the default in newly-generated configurations. I'll create a new PR to update that 👍
There was a problem hiding this comment.
Updated the documentation in #47519
…rrent defaults Since Rails 6.1, the default configuration has been to use Active Job's default queue, achieved by setting the queue name to `nil`. Refs: - rails#47408 - rails#47408 (comment)
| extend ActiveSupport::Concern | ||
|
|
||
| included do | ||
| class_attribute :delivery_job, default: ::ActionMailer::MailDeliveryJob |
There was a problem hiding this comment.
Why did we extract this module for two lines?
There was a problem hiding this comment.
It seemed clearer at the time to locate all queued-delivery concerns in a single place, but I agree that in retrospect it seems a bit much. Happy to move to ActionMailer::Base if that's preferred.
There was a problem hiding this comment.
Got it, I think I'm fine. It is not causing any trouble.
| @@ -9,7 +9,10 @@ module ActionMailer | |||
| # | |||
| # Exceptions are rescued and handled by the mailer class. | |||
| class MailDeliveryJob < ActiveJob::Base # :nodoc: | |||
| queue_as { ActionMailer::Base.deliver_later_queue_name } | |||
| queue_as do | |||
| mailer_class = arguments.first.constantize | |||
There was a problem hiding this comment.
Should not we be calling the mailer_class method here?
There was a problem hiding this comment.
Yes, you're right. The comment suggested it was specifically for use with exception handling but perhaps that comment can be removed.
There was a problem hiding this comment.
It is used to any code that can be executed before the arguments are deserialzied, or that could fail.
There was a problem hiding this comment.
Thanks. I'll update to use mailer_class then, and adjust the comment for clarity.
The
deliver_later_queue_nameis already configurable onActionMailer::Base, however the value is inherited by all subclasses. Use a class-inheritable attribute instead, so that subclasses can override.This can be handy if you're using named queues for concurrency control. For example, a queue being processed by many workers can deliver a lot of mail concurrently, which may not be desirable. With a high-volume mailer, too much concurrency can lead to rate-limiting and blocking at mail providers. Using a queue that's assigned fewer workers (or a single worker) is one way to prevent this.
To make this easier, this PR allows the queue used by the default delivery job to be configured on a per-mailer basis.
Example:
References: