Async actionmailer #6839

bcardarella · 2012-06-23T19:15:01Z

Adds support for ActionMailer to push messages to the Rails Queueing system and deliver async

Any ActionMailer class can be set to render and delier messages using the new Rails Queue. Some of this work was borrowed (with permission) from Nick Plante's (zapnap) reqsue_mailer gem.

foca · 2012-06-23T19:24:04Z

actionmailer/lib/action_mailer/async.rb

+
+    def method_missing(method_name, *args)
+      actual_message.send(method_name, *args)
+    end


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

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

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

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

tenderlove · 2012-06-23T19:25:04Z

I don't really like this. I envision people putting things on to the queue, not things happening to be put on the queue because of a configuration setting. I'm not against something similar to this, but I'd rather we have like a QueueMailer that people subclass, and those are automatically put on the queue. A global setting leaves a bad taste in my mouth. We have enough global settings already. :(

/cc @wycats @josevalim

bcardarella · 2012-06-23T19:27:49Z

@tenderlove the global setting is optional. You can call #async = true on the individual mailer classes or you can do the global config.

Credit goes to *Nicolás Sanguinetti* (foca) for this suggestion

bcardarella · 2012-06-23T20:11:57Z

I should also note that this implementation allows you to force message delivery by doing

Email.welcome.deliver(true)

or because everything else is delegated to the original message class you could call deliver!

As far as usecase, email async delivery is always the first background job I setup.

tenderlove · 2012-06-23T20:25:24Z

actionmailer/lib/action_mailer/async.rb

+    def initialize(mailer_class, method_name, *args)
+      @mailer_class = mailer_class
+      @method_name  = method_name
+      *@args        = *args


Why *@args?

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

tenderlove · 2012-06-23T20:39:01Z

actionmailer/lib/action_mailer/async.rb

+module ActionMailer::Async
+  def self.included(base)
+    base.extend(ClassMethods)
+  end


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.

yes, that is better. The extend ActionMailer::Async can be called from the async= setter on Base

doesn't this effect how the QueuedMessage class gets inherited into the parent class?

I don't understand. You mean the visibility of the constant?

tenderlove · 2012-06-24T02:29:24Z

I'm starting to warm up to this a lot. The only thing we need to figure out is that the job we push on to the queue can't contain a reference to the queue. Basically, we need to make sure the job pushed on to the queue can be marshal dump / loaded.

Credit goes to *Aaron Patterson* (tenderlove)

mauricio · 2012-06-24T03:55:23Z

@tenderlove technically you should send the generated email itself to the queue (and not the worker) since the TMail object can be encoded to string and it contains all the information you need (the only missing pieces is the ActionMailer config itself).

bcardarella · 2012-06-24T04:04:59Z

@mauricio while marshalling the rendered email itself would be best it might defeat the purpose of pushing the email to a queue. I have had cases where the rendering of the email is expensive. Personally, I would want everything having to do with the email pushed to the background.

mauricio · 2012-06-24T12:42:31Z

@bcardarella it might be a problem if you use resque as a queuen since all worker parameters have to be simple objects that can correctly be turned into JSON. If one of the parameters can't be made a JSON value it won't work for Resque and the worker would then just fail. Other background emailer solutions like ar_mailer just generated the email and stored it fully to avoid this kind of bug.

mauricio · 2012-06-24T13:43:12Z

I don't know how DelayedJob works so I'm going to do an example based on Resque and ResqueMailer alone.

I have this simple project and I call the mailer like this:

PostMailer.new_post( Post.last ).deliver

Since it uses Resque, the Post object (that is an AR object) will be turned to JSON like this:

{"body"=>"sample", "id"=>1, "title"=>"sample"}

There is no class information whatsoever here.

When I start Resque to start working on this queue this is what happens:

** [10:12:36 2012-06-24] 1408: Checking mailer
** [10:12:36 2012-06-24] 1408: Found job on mailer
** [10:12:36 2012-06-24] 1408: got: (Job{mailer} | PostMailer | ["post_created", {"body"=>"sample", "id"=>1, "title"=>"sample"}])
** [10:12:36 2012-06-24] 1408: resque-1.20.0: Forked 1431 at 1340543556
** [10:12:36 2012-06-24] 1431: resque-1.20.0: Processing mailer since 1340543556
** [10:12:36 2012-06-24] 1431: (Job{mailer} | PostMailer | ["post_created", {"body"=>"sample", "id"=>1, "title"=>"sample"}]) failed: #<NoMethodError: undefined method `title' for {"body"=>"sample", "id"=>1, "title"=>"sample"}:Hash>

Resque won't be able to figure out what this object was (a Post object) and all it sees is a hash, that doesn't have the title method (but has a title key, which is also important, as Resque turns Symbol objects into String, so, if your mailer parameters is a symbolized hash, all keys will become strings and you will also have to work around this).

A couple of years ago I even created a gem that delivers emails using Resque exactly because of this impossibility of using AR objects as parameters and using AR objects as mailer parameters seems to be a very common practice when using mailers.

So I think it would be nice at least to let the users know that they need to understand how their queue implementation works before turning something like this on or just ignore it and send the TMail object encoded as string over the wire.

bcardarella · 2012-06-24T13:51:45Z

@mauricio yes I agree with that, only simple objects should be used as args

tenderlove · 2012-06-24T17:27:29Z

You can use any object you want with resque if you marshal and base64 encode it. You just need to know to do that on the other end. ;-)

bcardarella · 2012-06-24T19:18:27Z

@tenderlove is there anything else you want to see in this PR or do you want to let it marinate until others weigh in?

tenderlove · 2012-06-24T20:54:00Z

actionmailer/lib/action_mailer/async.rb

+    end
+
+    # Will push the message onto the Queue to be processed
+    def deliver(force = false)


I think we can get rid of this parameter.

tenderlove · 2012-06-24T20:54:43Z

Just remove that one param, and I'm happy. I'll merge after that! Thanks for writing this! ❤️❤️❤️❤️

carlosantoniodasilva · 2012-06-24T21:02:45Z

actionmailer/test/base_test.rb

@@ -7,6 +7,9 @@
 require 'mailers/base_mailer'
 require 'mailers/proc_mailer'
 require 'mailers/asset_mailer'
+require 'mailers/async_mailer'
+


No need for extra empty line.

carlosantoniodasilva · 2012-06-24T21:05:23Z

@bcardarella thanks for that :), please don't forget to squash your commits after reviewing @tenderlove's comment :)

bcardarella · 2012-06-24T21:17:45Z

@tenderlove all set :)

bcardarella · 2012-06-24T21:35:42Z

I'll also be sure to do a write up for the guides

bcardarella · 2012-06-25T20:22:50Z

@tenderlove any holds up on this?

Async actionmailer

arunagw · 2012-06-26T07:10:18Z

Y U NO SQUASH COMMITS.

bcardarella · 2012-06-26T07:18:29Z

@arunagw I'm that guy now, sorry

arunagw · 2012-06-26T07:19:52Z

@bcardarella No probs at all :P This happend with me many times ;-)

Thanks for Pull Request.

Cheers,
Arun

rafaelfranca · 2012-06-26T13:51:34Z

@bcardarella would be great if you can review the Action Mailer guide and explain how this new mailer will work.

I think @fxn will appreciate.

bcardarella · 2012-06-26T13:59:56Z

@rafaelfranca yup, I'm going to update the guides :)

bcardarella added 5 commits June 23, 2012 14:36

Asynchronous ActionMailer

dc7fd82

Any ActionMailer class can be set to render and delier messages using the new Rails Queue. Some of this work was borrowed (with permission) from Nick Plante's (zapnap) reqsue_mailer gem.

AsyncMailer documention

0cb154b

Add commented out ActionMailer async config option

b6792e9

Force message delivery despite async

dc9f6fc

Updated Changelogs

d1d6c36

foca reviewed Jun 23, 2012
View reviewed changes

Use Delegator for a cleaner QueuedMessage class

5337149

Credit goes to *Nicolás Sanguinetti* (foca) for this suggestion

tenderlove reviewed Jun 23, 2012
View reviewed changes

Removed unecessary splatting

dee0b23

tenderlove reviewed Jun 23, 2012
View reviewed changes

Support for custom queues on the mailer

812d1e8

Credit goes to *Aaron Patterson* (tenderlove)

bcardarella added 2 commits June 24, 2012 04:21

Forcing the message sending is no longer necessary

33334d0

Better documentation for ActionMailer.async

aee4eec

Better documentation for ActionMailer.async

a11fcd9

tenderlove reviewed Jun 24, 2012
View reviewed changes

carlosantoniodasilva reviewed Jun 24, 2012
View reviewed changes

Some final syntax fixes

35717a9

tenderlove added a commit that referenced this pull request Jun 26, 2012

Merge pull request #6839 from bcardarella/async-actionmailer

ee74366

Async actionmailer

tenderlove merged commit ee74366 into rails:master Jun 26, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async actionmailer #6839

Async actionmailer #6839

bcardarella commented Jun 23, 2012

foca Jun 23, 2012

bcardarella Jun 23, 2012

foca Jun 23, 2012

bcardarella Jun 23, 2012

tenderlove commented Jun 23, 2012

bcardarella commented Jun 23, 2012

bcardarella commented Jun 23, 2012

tenderlove Jun 23, 2012

bcardarella Jun 23, 2012

tenderlove Jun 23, 2012

bcardarella Jun 23, 2012

bcardarella Jun 23, 2012

tenderlove Jun 24, 2012

tenderlove commented Jun 24, 2012

mauricio commented Jun 24, 2012

bcardarella commented Jun 24, 2012

mauricio commented Jun 24, 2012

mauricio commented Jun 24, 2012

bcardarella commented Jun 24, 2012

tenderlove commented Jun 24, 2012

bcardarella commented Jun 24, 2012

tenderlove Jun 24, 2012

tenderlove commented Jun 24, 2012

carlosantoniodasilva Jun 24, 2012

carlosantoniodasilva commented Jun 24, 2012

bcardarella commented Jun 24, 2012

bcardarella commented Jun 24, 2012

bcardarella commented Jun 25, 2012

arunagw commented Jun 26, 2012

bcardarella commented Jun 26, 2012

arunagw commented Jun 26, 2012

rafaelfranca commented Jun 26, 2012

bcardarella commented Jun 26, 2012

Async actionmailer #6839

Async actionmailer #6839

Conversation

bcardarella commented Jun 23, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenderlove commented Jun 23, 2012

bcardarella commented Jun 23, 2012

bcardarella commented Jun 23, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenderlove commented Jun 24, 2012

mauricio commented Jun 24, 2012

bcardarella commented Jun 24, 2012

mauricio commented Jun 24, 2012

mauricio commented Jun 24, 2012

bcardarella commented Jun 24, 2012

tenderlove commented Jun 24, 2012

bcardarella commented Jun 24, 2012

Choose a reason for hiding this comment

tenderlove commented Jun 24, 2012

Choose a reason for hiding this comment

carlosantoniodasilva commented Jun 24, 2012

bcardarella commented Jun 24, 2012

bcardarella commented Jun 24, 2012

bcardarella commented Jun 25, 2012

arunagw commented Jun 26, 2012

bcardarella commented Jun 26, 2012

arunagw commented Jun 26, 2012

rafaelfranca commented Jun 26, 2012

bcardarella commented Jun 26, 2012