-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Async actionmailer #6839
Async actionmailer #6839
Changes from 11 commits
dc7fd82
0cb154b
b6792e9
dc9f6fc
d1d6c36
5337149
dee0b23
812d1e8
33334d0
aee4eec
a11fcd9
35717a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
require 'delegate' | ||
|
||
module ActionMailer::Async | ||
def method_missing(method_name, *args) | ||
if action_methods.include?(method_name.to_s) | ||
QueuedMessage.new(queue, self, method_name, *args) | ||
else | ||
super | ||
end | ||
end | ||
|
||
def queue | ||
Rails.queue | ||
end | ||
|
||
class QueuedMessage < ::Delegator | ||
attr_reader :queue | ||
|
||
def initialize(queue, mailer_class, method_name, *args) | ||
@queue = queue | ||
@mailer_class = mailer_class | ||
@method_name = method_name | ||
@args = args | ||
end | ||
|
||
def __getobj__ | ||
@actual_message ||= @mailer_class.send(:new, @method_name, *@args).message | ||
end | ||
|
||
def run | ||
__getobj__.deliver | ||
end | ||
|
||
# Will push the message onto the Queue to be processed | ||
def deliver(force = false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the purpose of this? Synchronicity of the queue should depend on the queue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, now that the code is supporting multiple queues this is no longer necessary There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can get rid of this parameter. |
||
@queue << self | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't I have to explicitly state which methods I want to delegate? This There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes and no, I just noticed that you don't know to which class you're delegating until runtime, in that case DelegateClass won't do, since you need to know the delegated class when you define the object. That said, even if also uses Something like class QueuedMessage < Delegator
def __getobj__
@actual_message ||= @mailer_class.send(:new, @method_name, *@args).message
end
def run
__getobj__.deliver
end
end Then you don't need #method_missing, #actual_message, or #to_sin that implementation :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoa, that's way better. Thank you, updated and credited. |
||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,9 @@ | |
require 'mailers/base_mailer' | ||
require 'mailers/proc_mailer' | ||
require 'mailers/asset_mailer' | ||
require 'mailers/async_mailer' | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for extra empty line. |
||
require 'rails/queueing' | ||
|
||
class BaseTest < ActiveSupport::TestCase | ||
def teardown | ||
|
@@ -419,6 +422,26 @@ def teardown | |
assert_equal(1, BaseMailer.deliveries.length) | ||
end | ||
|
||
def stub_queue klass, queue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we keep the |
||
Class.new(klass) { | ||
extend Module.new { | ||
define_method :queue do | ||
queue | ||
end | ||
} | ||
} | ||
end | ||
|
||
test "delivering message asynchronously" do | ||
testing_queue = Rails::Queueing::TestQueue.new | ||
AsyncMailer.delivery_method = :test | ||
AsyncMailer.deliveries.clear | ||
stub_queue(AsyncMailer, testing_queue).welcome.deliver | ||
assert_equal(0, AsyncMailer.deliveries.length) | ||
testing_queue.drain | ||
assert_equal(1, AsyncMailer.deliveries.length) | ||
end | ||
|
||
test "calling deliver, ActionMailer should yield back to mail to let it call :do_delivery on itself" do | ||
mail = Mail::Message.new | ||
mail.expects(:do_delivery).once | ||
|
@@ -434,6 +457,7 @@ def teardown | |
end | ||
|
||
test "should raise if missing template in implicit render" do | ||
BaseMailer.deliveries.clear | ||
assert_raises ActionView::MissingTemplate do | ||
BaseMailer.implicit_different_template('missing_template').deliver | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Welcome |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
class AsyncMailer < BaseMailer | ||
self.async = true | ||
end |
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.
In this case, couldn't we just expect people to say
extend ActionMailer::Async
in their class? Then theClassMethods
module and this method become unnecessary.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.
yes, that is better. The
extend ActionMailer::Async
can be called from theasync=
setter onBase
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.
doesn't this effect how the QueuedMessage class gets inherited into the parent class?
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.
I don't understand. You mean the visibility of the constant?