Skip to content

Commit

Permalink
Move background jobs to the 'jobs' branch until fully baked. Not ship…
Browse files Browse the repository at this point in the history
…ping with Rails 4.0.
  • Loading branch information
jeremy committed Dec 22, 2012
1 parent 10c0a3b commit f9da785
Show file tree
Hide file tree
Showing 25 changed files with 4 additions and 724 deletions.
2 changes: 0 additions & 2 deletions actionmailer/CHANGELOG.md
Expand Up @@ -28,8 +28,6 @@

* Raise an `ActionView::MissingTemplate` exception when no implicit template could be found. *Damien Mathieu*

* Asynchronously send messages via the Rails Queue *Brian Cardarella*

* Allow callbacks to be defined in mailers similar to `ActionController::Base`. You can configure default
settings, headers, attachments, delivery settings or change delivery using
`before_filter`, `after_filter` etc. *Justin S. Leitgeb*
Expand Down
1 change: 0 additions & 1 deletion actionmailer/lib/action_mailer.rb
Expand Up @@ -44,5 +44,4 @@ module ActionMailer
autoload :MailHelper
autoload :TestCase
autoload :TestHelper
autoload :QueuedMessage
end
11 changes: 2 additions & 9 deletions actionmailer/lib/action_mailer/base.rb
@@ -1,10 +1,8 @@
require 'mail'
require 'action_mailer/queued_message'
require 'action_mailer/collector'
require 'active_support/core_ext/string/inflections'
require 'active_support/core_ext/hash/except'
require 'active_support/core_ext/module/anonymous'
require 'active_support/queueing'
require 'action_mailer/log_subscriber'

module ActionMailer
Expand Down Expand Up @@ -361,8 +359,6 @@ module ActionMailer
#
# * <tt>deliveries</tt> - Keeps an array of all the emails sent out through the Action Mailer with
# <tt>delivery_method :test</tt>. Most useful for unit and functional testing.
#
# * <tt>queue</> - The queue that will be used to deliver the mail. The queue should expect a job that responds to <tt>run</tt>.
class Base < AbstractController::Base
include DeliveryMethods
abstract!
Expand All @@ -389,9 +385,6 @@ class Base < AbstractController::Base
parts_order: [ "text/plain", "text/enriched", "text/html" ]
}.freeze

class_attribute :queue
self.queue = ActiveSupport::SynchronousQueue.new

class << self
# Register one or more Observers which will be notified when mail is delivered.
def register_observers(*observers)
Expand Down Expand Up @@ -483,8 +476,8 @@ def set_payload_for_mail(payload, mail) #:nodoc:
end

def method_missing(method_name, *args)
if action_methods.include?(method_name.to_s)
QueuedMessage.new(queue, self, method_name, *args)
if respond_to?(method_name)
new(method_name, *args).message
else
super
end
Expand Down
37 changes: 0 additions & 37 deletions actionmailer/lib/action_mailer/queued_message.rb

This file was deleted.

2 changes: 0 additions & 2 deletions actionmailer/lib/action_mailer/railtie.rb
Expand Up @@ -19,8 +19,6 @@ class Railtie < Rails::Railtie # :nodoc:
options.javascripts_dir ||= paths["public/javascripts"].first
options.stylesheets_dir ||= paths["public/stylesheets"].first

options.queue ||= app.queue

# make sure readers methods get compiled
options.asset_host ||= app.config.asset_host
options.relative_url_root ||= app.config.relative_url_root
Expand Down
1 change: 0 additions & 1 deletion actionmailer/test/abstract_unit.rb
Expand Up @@ -11,7 +11,6 @@
require 'minitest/autorun'
require 'action_mailer'
require 'action_mailer/test_case'
require 'active_support/queueing'

silence_warnings do
# These external dependencies have warnings :/
Expand Down
13 changes: 0 additions & 13 deletions actionmailer/test/base_test.rb
Expand Up @@ -3,13 +3,11 @@
require 'set'

require 'action_dispatch'
require 'active_support/queueing'
require 'active_support/time'

require 'mailers/base_mailer'
require 'mailers/proc_mailer'
require 'mailers/asset_mailer'
require 'mailers/async_mailer'

class BaseTest < ActiveSupport::TestCase
def teardown
Expand Down Expand Up @@ -422,17 +420,6 @@ def teardown
assert_equal(1, BaseMailer.deliveries.length)
end

test "delivering message asynchronously" do
AsyncMailer.delivery_method = :test
AsyncMailer.deliveries.clear

AsyncMailer.welcome.deliver
assert_equal 0, AsyncMailer.deliveries.length

AsyncMailer.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
Expand Down
3 changes: 0 additions & 3 deletions actionmailer/test/mailers/async_mailer.rb

This file was deleted.

10 changes: 0 additions & 10 deletions actionpack/test/controller/assert_select_test.rb
Expand Up @@ -10,16 +10,6 @@
require 'action_mailer'
ActionMailer::Base.view_paths = FIXTURE_LOAD_PATH

class SynchronousQueue < Queue
def push(job)
job.run
end
alias << push
alias enq push
end

ActionMailer::Base.queue = SynchronousQueue.new

class AssertSelectTest < ActionController::TestCase
Assertion = ActiveSupport::TestCase::Assertion

Expand Down
105 changes: 0 additions & 105 deletions activesupport/lib/active_support/queueing.rb

This file was deleted.

27 changes: 0 additions & 27 deletions activesupport/test/queueing/synchronous_queue_test.rb

This file was deleted.

27 comments on commit f9da785

@Aupajo
Copy link
Contributor

@Aupajo Aupajo commented on f9da785 Dec 22, 2012

Choose a reason for hiding this comment

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

What constitutes “fully baked”?

@samgranieri
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @Aupajo on this. Was the queue system intended to be the underpinning to resque and sidekiq as rack is to rails or something else entirely? Wy bother with writing a new queue system when resque and sidekiq are the defaults ?
Personally, I'm switching to sidekiq.
/cc @hone @mperham

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

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

@Aupajo when it reaches a point that we think using it is natural, it still need a lot of work and it is a blocker for the release. We think is better to pospone.

@samgranieri nobody is writing a new queue system, Rails queue is a API can be used with sidekiq, resque, or any other queue system. Think about it as what Active Model is for ORMs.

@steveklabnik
Copy link
Member

Choose a reason for hiding this comment

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

Both Resque and Sidekiq already had integration with this interface, but there were some issues that were causing problems, and the feature isn't worth holding up the release of Rails 4 for.

@samgranieri
Copy link
Contributor

@samgranieri samgranieri commented on f9da785 Dec 22, 2012 via email

Choose a reason for hiding this comment

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

@Aupajo
Copy link
Contributor

@Aupajo Aupajo commented on f9da785 Dec 22, 2012

Choose a reason for hiding this comment

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

@rafaelfranca I get that it doesn't feel fully baked, but why doesn't it? What was the awkwardness/what were the issues? (Is there an existing thread on this, maybe?)

@rafaelfranca
Copy link
Member

Choose a reason for hiding this comment

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

There is no thread anywhere. The decision is based on discussion between the core members and experiments. We fell this is not ready and we still have a lot of doubts with the usage.

It need more experiment to get released.

@jeremy
Copy link
Member Author

@jeremy jeremy commented on f9da785 Dec 22, 2012

Choose a reason for hiding this comment

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

Slipped a knife in to test, and the API was still gooey in the center 😁

@krainboltgreene
Copy link
Contributor

Choose a reason for hiding this comment

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

Love them secret discussions on open source projects.

@dhh
Copy link
Member

@dhh dhh commented on f9da785 Dec 31, 2012

Choose a reason for hiding this comment

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

It's how people get shit done without having trolls spoil their appetite for the work.

@fxn
Copy link
Member

@fxn fxn commented on f9da785 Dec 31, 2012

Choose a reason for hiding this comment

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

You have the code and the rationale, that is open source. While one is always open and thankful for feedback, the project is run by a team, not by a universal house.

@mikegee
Copy link
Contributor

@mikegee mikegee commented on f9da785 Jan 1, 2013

Choose a reason for hiding this comment

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

I trust you guys to made the right decision on this, but I also empathize with the sentiment expressed above. Perhaps some failing tests would make clear exactly what needs work and define "fully baked"?

@elskwid
Copy link

@elskwid elskwid commented on f9da785 Jan 1, 2013

Choose a reason for hiding this comment

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

We were surprised to see this get cut, although we had heard a little ahead of time. My buddy @phlipper and I had gotten the queue stuff backported to Rails 3.2+ and we're close to releasing the gem for it. https://github.com/probablywrong/rails-queue

Perhaps we can take this discussion and passion over there, get it working for Rails 4 & 3.2+, and work out some of the implementation details/wrinkles outside of the main Rails codebase. That would give us a focused place to discuss specific issues. Interested?

@jeremy
Copy link
Member Author

@jeremy jeremy commented on f9da785 Jan 1, 2013

Choose a reason for hiding this comment

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

@ElsKid - it's not cut; it's on the jobs branch. By all means continue work! (Releasing a gem named "rails queue" = confusion, though.)

@mikegee - if I could write a test for this API doesn't feel great in my app, I'd be first in line with 100% coverage ;)

Some quick background: I developed a new app that heavily relies on job processing. I used the Rails.queue API from end to end and, ultimately, I felt it's a regression over writing my Resque jobs directly.

  1. Rails.queue[:name] << job API style reads as "push this job onto this specific queue." What most calling code needs is "run this job later." The calling code shouldn't be concerned with where the job is headed or who executes it, so picking a queue by name is a poor distribution of responsibility (not to mention repetitive).
  2. Marshaling complete Job instances sounds great, but it's error-prone. It's too easy to suck in unexpected references to huge objects or procs that can't be marshaled. The API shouldn't attempt to make this marshaling boundary transparent since the wins are so quickly lost as soon as you track down a single marshaling bug. Passing record ids to a job rather than record instances should be the way we think about firing off a job. We're writing a job work order not packaging up a runnable job.
  3. A Rails.database wouldn't make sense and neither does Rails.queue. If anything, that serves in a backend/adapter role to jobs themselves.

On the plus side, as we coped with this issues, we did come up with some nice abstractions for application jobs. Notably, we'll jettison "queueing" from our vocabulary entirely. We want to present an API for submitting and executing jobs. We have more in common with a process scheduler than a task queue.

And, even more promisingly, we saw a couple of key spots where we'd often like to be able to defer work. I'm optimistic that these will have a greater payoff, overall, than a unified job submission/execution API.

  1. A before_commit callback. Russian Doll caching relies on chaining touch: true so changing a child record automatically touches its parent, which touches its grandparent, and so on. That means changing N child objects fires off N parent and grandparent touches when only the last touch was needed. So we'd like to batch up the touches and perform the updates in a before_commit.
  2. Post-request processing: the Merb-style perform_later. After the HTTP response has been sent, perform any work that didn't need to be completed before sending the response. Many tasks are short-running jobs like "send a notification email" and "update the search index" that needn't block a response, yet barely justify a full-on Job class. By performing this work immediately after a response, we sidestep the need to marshal jobs at all, write Job classes, or even run extra worker boxes. We shift those servers back to normal HTTP duties, leaving worker boxes for heavyweight long-running jobs that can get backed up. Much more efficient resource utilization from a total-cluster point of view. This effort hinges on some deep plumbing into our Rack interaction, since the Rack request handling style is not amenable to this currently.

TL;DR we did an experiment with Rails.queue and rejected the API yet learned a lot about what we Really Need. Stay tuned!

@elskwid
Copy link

@elskwid elskwid commented on f9da785 Jan 1, 2013

Choose a reason for hiding this comment

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

@jeremy, thank you for the detailed write-up. Our name was to help ease confusion about what it was that we backported. When we wrote it we thought it as around to stay. heh. Still might have some use as a place to kick around ideas.

We found similar strangeness when testing and using the code as well. Specifically the syntax didn't really "fit"with what we wanted either. And the marshaling broke down for us too.

I think the biggest benefit of the Rails.queue idea was that you always had one and areas that need background execution, no matter the size/scope/expense, you could just fire them. You could then say, do this in dev, do something else in test, and something completely different in production but I have one API to just throw jobs into. That said, a better API to do that would be amazing.

Thanks again!

@Aupajo
Copy link
Contributor

@Aupajo Aupajo commented on f9da785 Jan 1, 2013

Choose a reason for hiding this comment

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

@jeremy Wow, it sounds like the direction this is heading is great. Thanks for the write-up.

@stream7
Copy link

@stream7 stream7 commented on f9da785 Jan 1, 2013

Choose a reason for hiding this comment

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

@jeremy if Rails provides the adapter changing the API should not be a big deal. See https://gist.github.com/351550

@sobrinho
Copy link
Contributor

Choose a reason for hiding this comment

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

For the people who wants an abstraction over the available queues, take a look on https://github.com/fnando/qe

@erickrause
Copy link
Contributor

Choose a reason for hiding this comment

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

@jeremy Thanks for the write up. "rejected the API yet learned a lot about what we Really Need" I liked the idea of the Rails.queue, but love the before_commit and post request ideas.

@jeremy
Copy link
Member Author

@jeremy jeremy commented on f9da785 Jan 3, 2013

Choose a reason for hiding this comment

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

@sobrinho qe looks great - this is precisely where the Job backend are heading.

@jrochkind
Copy link
Contributor

Choose a reason for hiding this comment

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

@jeremy thanks SO MUCH for that writeup, explains why it was decided to be not fully baked, with a convincing and reasonable argument, and also explain what you are thinking about future directions.

It is VERY useful for us Rails-using developers to have this, thanks for realizing such and providing.

(Also, thanks very much to Rails core for not releasing an API as part of Rails when you realize the design is not right, we indeed count on your active judgement of these things to keep Rails great. Throwing in half-baked things will help nobody.)

@kenn
Copy link
Contributor

@kenn kenn commented on f9da785 Jan 5, 2013

Choose a reason for hiding this comment

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

@jeremy thanks for the write up, that reasoning is really convincing and enlightening.

Post-request processing is clever - I've been using EM.next_tick when I use an eventmachine-based web server like thin, which works great. No marshaling is necessary, and binding objects in the local scope are available - which simplifies things tremendously. I'd be very happy if we'd get the utility of EM.next_tick without depending on EM.

Anonymous jobs FTW!

@jason-rutherford
Copy link

Choose a reason for hiding this comment

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

I'm looking forward to submitting job work orders. If you combine this with the power of something like hstore to store arbitrary variables for later when the job executes... that's what I'm talking about.

@SirRawlins
Copy link

Choose a reason for hiding this comment

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

The Russian Doll :touch issue is something which has been giving me troubles today - look forward to something coming of this discussion.

@dhh
Copy link
Member

@dhh dhh commented on f9da785 Aug 5, 2013 via email

Choose a reason for hiding this comment

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

@SirRawlins
Copy link

Choose a reason for hiding this comment

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

@dhh Hi David - it's mentioned in Jeremy's comment above f9da785#commitcomment-2370489 I was also referred to this post discussion from another issue recoded here: #8759

I don't think there is a specific link between the two challenges, but Jeremy seems to have come up against the :touch challenges at some point in his adventure here.

@deependersingla
Copy link

Choose a reason for hiding this comment

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

@SirRawlins Any update?

Please sign in to comment.