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

Async actionmailer #6839

Merged
merged 12 commits into from
Jun 26, 2012
49 changes: 49 additions & 0 deletions actionmailer/lib/action_mailer/async.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
module ActionMailer::Async
def self.included(base)
base.extend(ClassMethods)
end
Copy link
Member

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 the ClassMethods module and this method become unnecessary.

Copy link
Contributor Author

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 the async= setter on Base

Copy link
Contributor Author

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?

Copy link
Member

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?


module ClassMethods
def method_missing(method_name, *args)
if action_methods.include?(method_name.to_s)
QueuedMessage.new(self, method_name, *args)
else
super
end
end
end

class QueuedMessage
delegate :to_s, :to => :actual_message

def initialize(mailer_class, method_name, *args)
@mailer_class = mailer_class
@method_name = method_name
*@args = *args
Copy link
Member

Choose a reason for hiding this comment

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

Why *@args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it probably doesn't need to be. I was cribbing off of resque_mailer. I'll update.

end

def run
actual_message.deliver
end

# Will push the message onto the Queue to be processed
# To force message delivery dispite async pass `true`
# Emailer.welcome.deliver(true)
def deliver(force = false)
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

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

I think we can get rid of this parameter.

if force
run
else
Rails.queue << self
Copy link
Member

Choose a reason for hiding this comment

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

Why hardcode to Rails.queue? If the queue is an ivar, set in the constructor, then subclasses could change the queue depending on their needs (e.g. they need to deliver synchronously or not).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I guess this was a misunderstanding on how the Rails.queue was meant to be used. Are you suggesting something like:

def initializer(method_name, *args)
  ...
  @queue = Rails.queue
end

Copy link
Member

Choose a reason for hiding this comment

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

Yup, and thrown in an attr_reader for it. Rails.queue is the "default" queue. We (at work) need more than one queue (different emails, image processing, etc), so I'd like Rails.queue to be the "default" one, but something we can easily change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this introduces some complexity. When I was stubbing out Rails.queue in the test I didn't need the original Rails.queue method which is just really Rails.application.queue. It would appear I would need to build a test application so that I'm not getting a method called on nil error

Copy link
Member

Choose a reason for hiding this comment

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

Ya, that is pretty annoying. We can get around it by defining a private default_queue method:

require 'minitest/autorun'
require 'thread'

class DelayedMailer
  attr_reader :queue

  def initialize
    @queue = default_queue
  end

  private

  def default_queue
    Rails.queue
  end
end

class TestFoo < MiniTest::Unit::TestCase
  def test_init_calls_Rails_queue
    test_queue = Queue.new

    mailer = Class.new(DelayedMailer) {
      define_method :default_queue do
        test_queue
      end
    }.new

    assert_equal test_queue, mailer.queue
  end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very clever. I'll implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I'm not seeing a clear path to implementing this. In this code the QueuedMessage class is namespaced under AsyncMailer. The only time the instance of QueuedMessage is being created is to interrupt creating an instance of ActionMailer::Base. Moving the default_queue method to the class of ActionMailer::Base makes no sense because it defeats the purpose of having multiple queues. The same goes for moving the entire queue system to the class level of QueuedMessage.

I think this goes into a bigger issue of the Rails.queue not having explicit names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I might be over thinking this. Are you asking for:

Emailer.queue == QueueA
WelcomeMailer.queue == QueueA

or

Emailer.queue == QueueA
WelcomeMailer.queue == QueueB

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. Sorry for the confusion. I want the second one. We should just add a class method on the mailer to return the queue. This I can mix in a module to return the right queue depending on where I want to go.

Something like this.

end
end

# The original ActionMailer message
def actual_message
@actual_message ||= @mailer_class.send(:new, @method_name, *@args).message
end

def method_missing(method_name, *args)
actual_message.send(method_name, *args)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use method_missing instead of ruby's DelegateClass (from the stdlib's "delegate") which copies the public methods and exposes the public API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 QueuedMessage class is meant to behave in most ways identical to the original message class. this is quicker to delegate those methods to it than listing each one out. Unless I'm mistaken about the behavior of Delegate

Copy link
Contributor

Choose a reason for hiding this comment

The 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 method_missing, I would still use Delegator because it handles things like #respond_to_missing?, #public_methods, etc.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoa, that's way better. Thank you, updated and credited.

end
end
8 changes: 8 additions & 0 deletions actionmailer/lib/action_mailer/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,14 @@ def respond_to?(method, include_private = false) #:nodoc:
super || action_methods.include?(method.to_s)
end

# Will force ActionMailer to push new messages to the Rails.queue
def async=(truth)
if truth
require 'action_mailer/async'
include ActionMailer::Async
end
end

protected

def set_payload_for_mail(payload, mail) #:nodoc:
Expand Down
22 changes: 22 additions & 0 deletions actionmailer/test/base_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
require 'mailers/base_mailer'
require 'mailers/proc_mailer'
require 'mailers/asset_mailer'
require 'mailers/async_mailer'

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -419,6 +422,24 @@ def teardown
assert_equal(1, BaseMailer.deliveries.length)
end

test "delivering message asynchronously" do
Rails.stubs(:queue).returns(Rails::Queueing::TestQueue.new)
AsyncMailer.delivery_method = :test
AsyncMailer.deliveries.clear
AsyncMailer.welcome.deliver
assert_equal(0, AsyncMailer.deliveries.length)
Rails.queue.drain
assert_equal(1, AsyncMailer.deliveries.length)
end

test "forcing message delivery despite asynchronous" do
Rails.stubs(:queue).returns(Rails::Queueing::TestQueue.new)
Copy link
Member

Choose a reason for hiding this comment

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

If the queue were set as an ivar with a method on the mailer, we wouldn't have to stub Rails.queue here.

AsyncMailer.delivery_method = :test
AsyncMailer.deliveries.clear
AsyncMailer.welcome.deliver(true)
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
Expand All @@ -434,6 +455,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
Expand Down
1 change: 1 addition & 0 deletions actionmailer/test/fixtures/async_mailer/welcome.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Welcome
3 changes: 3 additions & 0 deletions actionmailer/test/mailers/async_mailer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class AsyncMailer < BaseMailer
self.async = true
end
Original file line number Diff line number Diff line change
Expand Up @@ -67,5 +67,8 @@ class Application < Rails::Application
# Version of your assets, change this if you want to expire all your assets.
config.assets.version = '1.0'
<% end -%>

# Enable app-wide asynchronous ActionMailer
# config.action_mailer.async = true
end
end