Integrate ActiveJob / DeliverLater / GlobalID with Rails #16485

Merged
merged 230 commits into from Aug 17, 2014

Projects

None yet
@dhh
Member
dhh commented Aug 12, 2014

WIP.

dhh and others added some commits May 18, 2014
@dhh dhh Skeleton gem a832bc3
@dhh dhh Run tests through Rake 60d13d6
@dhh dhh Simplest job and inline queue 08a2ba9
@dhh dhh Rename to InlineAdapter to match *Adapter form, even if the queue is …
…embedded in there too
30973e3
@dhh dhh Add ResqueAdapter and provide test infrastructure for the now multipl…
…e adapters
eed52c8
@dhh dhh Queue naming with a base, which requires a JobWrapper to comply to Re…
…sques expectation of a class variable
b32fdd5
@charliesome charliesome Clean up JobWrappers::ResqueWrapper.perform
This is not only easier to read, but it'll also properly raise an ArgumentError rather than a NoMethodError when called with no arguments.

It also allocates 4 fewer objects per call (8 down from 12), and is about 50% faster according to a quick benchmark.
2ac1a02
@dhh dhh Merge pull request #1 from charliesome/patch-1
Clean up JobWrappers::ResqueWrapper.perform
ce124a1
@seuros seuros Add Sidekiq adapter/wrapper fixes #3 a712c07
@seuros seuros Correct typo in version.rb 53f08f9
@seuros seuros Add Sucker Punch adapter/wrapper 68543de
@dhh dhh Merge pull request #5 from seuros/master
 Add Sidekiq adapter/wrapper fixes #3
12705fa
@dhh dhh With dependencies for Sidekiq and Sucker Punch e5feb8b
@dhh dhh List adapters supported and wanted d3bc449
@seuros seuros Lazy-load adapters, fixes #6 91461dc
@dhh dhh Merge pull request #10 from seuros/master
Lazy-load adapters, fixes #6
d7c3098
@dhh dhh Dont need the explicit error handling -- if the require fails, it wil…
…l raise exactly the error we want to communicate anyway. Also use the load path, so we can allow plugins, rather than requre_relative
fd1e61a
@cristianbica cristianbica Implemented delayed job d3ff144
@cristianbica cristianbica Modified readme 6e18a74
@dhh dhh Merge pull request #9 from cristianbica/delayed_job_adapter
Implemented delayed_job support
e06e64e
@dhh dhh Extract QueueAdapter module for setting and looking up adapters c334bea
@dhh dhh Merge branch 'master' of github.com:rails/activejob dc20f7d
@dhh dhh Fix for the new adapter setter 935f53b
@dhh dhh Extract QueueName into its own module 60d76c5
@dhh dhh Spacing 65bf5f1
@dhh dhh Add GlobalID support for serialization 211ce71
@dhh dhh Use markdown instead of rdoc f71147e
@dhh dhh Update README.md c14cc48
@dhh dhh Inline the job wrappers 501cc60
@dhh dhh No need to qualify the Parameters class with the namespace 37b13cd
@dhh dhh Merge branch 'master' of github.com:rails/activejob 079ff1f
@dhh dhh Move activemodel-globalid dependency to gemspec f3a9ad7
@dhh dhh Move to instance method and document usage afb3d4f
@seuros seuros Use case/when bc9bd57
@dhh dhh Merge pull request #15 from seuros/master
Use case/when
d3d5308
@dhh dhh Update README.md c47d6b4
@dhh dhh Update README.md f85ad8b
@seuros seuros Setting the adapter load the required gem. ca8b2fd
@dhh dhh Merge pull request #16 from seuros/master
Setting the adapter load the required gem.
60b8af4
@dhh dhh Add justification c69dda4
@dhh dhh Merge branch 'master' of github.com:rails/activejob c2cdb8b
@mperham mperham Pull in rake
For those that don't have it globally...
3688c6c
@rafaelfranca rafaelfranca Merge pull request #17 from mperham/patch-1
Pull in rake
572e60f
@mperham mperham Whitelist legal job parameter types 575a837
Rafael Mendonça França Use bundle gem tasks 704b17b
Rafael Mendonça França Update Gemfile.lock 7446403
Rafael Mendonça França ✂️ 430228e
@DouweM DouweM Fix typos in readme. edc8926
@rafaelfranca rafaelfranca Merge pull request #21 from DouweM/patch-1
Fix typos in readme.
3a932bf
@rafaelfranca rafaelfranca Merge pull request #18 from mperham/param_whitelist
Whitelist legal job parameter types
70419b5
@DouweM DouweM Have Sidekiq adapter take queue_name into account. 62aa8ac
@dhh dhh Merge pull request #22 from DouweM/patch-1
Have Sidekiq adapter take queue_name into account.
a9dc36c
@dhh dhh Styling e240566
Rafael Mendonça França Make sure Bignum can be serialized c086dfb
@DouweM DouweM Refactor Resque adapter to be more consistent with others 68793bf
@rafaelfranca rafaelfranca Merge pull request #23 from DouweM/patch-2
Refactor Resque adapter to be more consistent with others
8a05587
@larrylv larrylv Add doc for setting the queue adapter. 29c9dcc
@rafaelfranca rafaelfranca Merge pull request #28 from larrylv/patch-1
Add doc for setting the queue adapter.
7a2775b
@larrylv larrylv Fix `Person#==` method in test. 6889c61
@rafaelfranca rafaelfranca Merge pull request #29 from larrylv/patch-2
Fix `Person#==` method in test.
ec47b52
@larrylv larrylv Make tests for `Person` pass. 6198978
@rafaelfranca rafaelfranca Merge pull request #30 from larrylv/patch-3
Make tests for `Person` pass.
08d8600
@mperham mperham RDoc enqueue fc80f4e
@rafaelfranca rafaelfranca Merge pull request #32 from mperham/master
RDoc enqueue
179060a
@mperham mperham Update README.md
beanstalkd and rabbitmq are MQ servers, they aren't something that would integrate directly with Rails and ActiveJob.  That's like saying we want an adapter for Redis, when really you want adapters for Resque, Sidekiq, etc.

Sneakers is the latest Ruby job system using rabbitmq.  I don't know of a modern beanstalkd system for Ruby, Stalker is pretty dead these days.  I added QueueClassic because it's reasonably popular on Heroku for the "postgresql only" crowd.
eebec88
@rafaelfranca rafaelfranca Merge pull request #33 from mperham/patch-2
Update wanted list
ac930b8
@cristianbica cristianbica Implemented queue_classic adapter 5cad2c1
@rafaelfranca rafaelfranca Merge pull request #34 from cristianbica/queue_classic-adpater
queue_classic adapter
c6925f5
@mperham mperham Implement enqueue_at/enqueue_in
Delayed jobs are supported by all systems except QueueClassic.  For it I decided to raise NotImplementedError.

The inline implementation is a bit rough.
3648838
@mperham mperham remove debugging 23a8723
@Aesthetikx Aesthetikx Add Sneakers wrapper 1b71fe5
@mperham mperham Reimplement Sidekiq worker
This better integrates various Sidekiq features into AJ jobs.

Things like the JID will be set as expected and the user can use `sidekiq_options` in AJ::Base subclasses as usual to configure various features.
c813a30
@guilleiguaran guilleiguaran Merge pull request #37 from Aesthetikx/master
Add Sneakers wrapper
3a233d0
@larrylv larrylv Update README for currently supported adapters. 6f059b0
@guilleiguaran guilleiguaran Merge pull request #39 from larrylv/update-readme
Update README for currently supported adapters.
f5c85dc
@zhouguangming zhouguangming Make consistent code style fc062f6
@DouweM DouweM Determine full class name dynamically in QC adapter. 9b5562a
@seuros seuros Add Que Adapter/Wrapper 6d00950
@dhh dhh No need for the local variable fa47832
@dhh dhh Clarify that the other option for name_or_adapter is to be a class (t…
…he Adapter class)
9c8c6bf
@dhh dhh Merge pull request #31 from seuros/master
Add Que Adapter/Wrapper
7a6aa47
@dhh dhh Merge pull request #45 from DouweM/patch-2
Determine full class name dynamically in QC adapter.
f8fc6ca
@dhh dhh Merge pull request #42 from zhouguangming/refactor
Make consistent code style
9d34727
@dhh dhh Update README with queue_as example and the desire for a Resque 2.x a…
…dapter
34f0e42
@dhh dhh Add Sneakers test and inline setup/teardown 2d19c71
@mytrile mytrile Adds backburner adapter 243afc0
@cristianbica cristianbica Added logging capabilities 0227af9
@mperham mperham Remove all Sidekiq-specific stuff from job, enable retries by default 42f5ba3
@cristianbica cristianbica Moved log_subcriber dependency and cleanup a265011
@dhh dhh Merge pull request #46 from cristianbica/logging
Added logging capabilities
a8cfb3d
@DouweM DouweM Have Sneakers adapter take queue_name into account. cbfc8b3
@mperham mperham merge master b49d3f1
@dhh dhh Merge pull request #38 from mperham/rework_sidekiq
Reimplement Sidekiq adapter
3649ebc
@dhh dhh Reformat the logging line and ensure we are logging the serialized args 1d519aa
@mperham mperham Move past time check out of adapters 1d3ba68
@mperham mperham Add logging for enqueued_at and perform errors a88d9eb
@dhh dhh Styling b13a3cb
@rafaelfranca rafaelfranca Merge pull request #44 from DouweM/patch-1
Have Sneakers adapter take queue_name into account.
be3b805
@mperham mperham Fix boogs, stub not implemented adapters 2aa9024
@dhh dhh Merge pull request #43 from mytrile/master
Adding backburner(beanstalks client) adapter
207ca30
@rafaelfranca rafaelfranca and 4 others commented on an outdated diff Aug 13, 2014
actionmailer/lib/action_mailer/base.rb
@@ -549,7 +549,11 @@ def set_payload_for_mail(payload, mail) #:nodoc:
def method_missing(method_name, *args) # :nodoc:
if respond_to?(method_name)
- new(method_name, *args).message
+ if defined?(::ActiveJob) && action_methods.include?(method_name.to_s)
@rafaelfranca
rafaelfranca Aug 13, 2014 Member

Can we avoid checking ActiveJob here? This is a high coupling between the two frameworks.

@rafaelfranca
rafaelfranca Aug 13, 2014 Member

I believe we should invert this dependency. Active Job should inject behaviour at Action Mailer when loaded, not Action Mailer that have to depend on Active Job.

@seuros
seuros Aug 13, 2014 Member

We need to prepend the mixin in actionmailer/base. We found out that prepend is not supported in ruby 1.9.3.
Check this

@jeremy
jeremy Aug 13, 2014 Member

IMO it's Action Mailer's job to be able to delay email. Active Job shouldn't care or know about that.

Rather than doing this check, we can always return a future message delivery. DeliverLater::MailMessageWrapper is a mouthful. Call it something like a MessageDelivery.

When people want to deliver it later, they call .deliver_later and that's that. The message isn't rendered, it's enqueued.

When people want to deliver it now, they call .deliver which passes through to the delegate object, the underlying Message.

So we preserve existing behavior and don't need special conditionals.

@rafaelfranca
rafaelfranca Aug 13, 2014 Member

I think We can make it work easily.

First we need to move this definition of method_missing to a new module that we will included in the singleton class of ActionMailer::Base. After we only need to include the ActiveJob::DeliverLater module on ActionMailer::Base.singleton_class.

Here a pseudo-implementation:

diff --git a/actionmailer/lib/action_mailer/base.rb b/actionmailer/lib/action_mailer/base.rb
index df0c945..7598337 100644
--- a/actionmailer/lib/action_mailer/base.rb
+++ b/actionmailer/lib/action_mailer/base.rb
@@ -547,17 +547,20 @@ module ActionMailer
         payload[:mail]       = mail.encoded
       end

-      def method_missing(method_name, *args) # :nodoc:
-        if respond_to?(method_name)
-          if defined?(::ActiveJob) && action_methods.include?(method_name.to_s)
-            DeliverLater::MailMessageWrapper.new(self, method_name, *args)
-          else
+      module Foo
+        protected
+
+        def method_missing(method_name, *args) # :nodoc:
+          p "Not: #{method_name}"
+          if respond_to?(method_name)
             new(method_name, *args).message
+          else
+            super
           end
-        else
-          super
         end
       end
+
+      include Foo
     end

     attr_internal :message
diff --git a/actionmailer/lib/action_mailer/deliver_later.rb b/actionmailer/lib/action_mailer/deliver_later.rb
index 5609e35..92a8e92 100644
--- a/actionmailer/lib/action_mailer/deliver_later.rb
+++ b/actionmailer/lib/action_mailer/deliver_later.rb
@@ -5,5 +5,18 @@ module ActionMailer
     extend ActiveSupport::Autoload
     autoload :Job
     autoload :MailMessageWrapper
+
+    module Mixin
+
+      protected
+
+      def method_missing(method_name, *args)
+        if respond_to?(method_name)
+          DeliverLater::MailMessageWrapper.new(self, method_name, *args)
+        else
+          super
+        end
+      end
+    end
   end
-end
\ No newline at end of file
+end
diff --git a/actionmailer/test/deliver_later_test.rb b/actionmailer/test/deliver_later_test.rb
index 829c968..602e597 100644
--- a/actionmailer/test/deliver_later_test.rb
+++ b/actionmailer/test/deliver_later_test.rb
@@ -5,6 +5,8 @@ require 'abstract_unit'
 require 'minitest/mock'
 require_relative 'mailers/delayed_mailer'

+ActionMailer::Base.singleton_class.send(:include, ActionMailer::DeliverLater::Mixin)
+
 class MailerTest < ActiveSupport::TestCase

   setup do
@cristianbica
cristianbica Aug 13, 2014 Contributor

I'm with @jeremy on who knows what: ActiveJob doesn't know about ActionMailer.
@rafaelfranca nice workaround for ruby's override method in mixin issue but if ActionMailer knows about delivering later we can hook the logic in the ActionMailer::Base method_missing

@dhh
dhh Aug 13, 2014 Member

👍 to @jeremy's idea as well.

On Aug 13, 2014, at 10:45, Rafael Mendonça França notifications@github.com wrote:

In actionmailer/lib/action_mailer/base.rb:

@@ -549,7 +549,11 @@ def set_payload_for_mail(payload, mail) #:nodoc:

   def method_missing(method_name, *args) # :nodoc:
     if respond_to?(method_name)
  •      new(method_name, *args).message
    
  •      if defined?(::ActiveJob) && action_methods.include?(method_name.to_s)
    
    I like @jeremy's idea


Reply to this email directly or view it on GitHub.

@cristianbica
cristianbica Aug 13, 2014 Contributor

@jeremy so we make the DeliverLater::MailMessageWrapper a generic MessageDelivery and there we implement deliver_later which checks if ActiveJob is defined.
The initial implementation of the wrapper wasn't building the Mail::Message on creation (didn't had the __getobj__ in initialize). There were some tests failing because they were testing behaviour from the mailer methods (ex: assert_raises(RuntimeError) { LateInlineAttachmentMailer.welcome }) and then we created the Mail::Message anyway. There were like less than 10 test cases so we can fix them (maybe using the new #itself method :) ).
So to be clear: this implementation will change the behaviour of mailers. SomeMailer.some_email will return an ActionMailer::MessageDelivery which will be a Delegator around the Mail::Message which will be built lazy.

@jeremy
jeremy Aug 13, 2014 Member

@cristianbica MessageDelivery#deliver_later needn't even do the Active Job check. If Active Job isn't available and the app is calling deliver_later, that's a bug and we should just let it raise whatever exception bubbles up.

@seuros
seuros Aug 13, 2014 Member

@jeremy , I'm working on it now.

@matthewd matthewd and 1 other commented on an outdated diff Aug 14, 2014
...vejob/lib/active_job/queue_adapters/resque_adapter.rb
@@ -0,0 +1,38 @@
+require 'resque'
+require 'active_support/core_ext/enumerable'
+require 'active_support/core_ext/array/access'
+
+begin
+ require 'resque-scheduler'
+rescue LoadError
+ begin
+ require 'resque_scheduler'
+ rescue LoadError
+ $stderr.puts 'The ActiveJob resque adapter requires resque-scheduler. Please add it to your Gemfile and run bundle install'
+ raise e
@matthewd
matthewd Aug 14, 2014 Member

e?

Also, why the stderr message here, but not on the others?

@cristianbica
cristianbica Aug 14, 2014 Contributor

missing an => e ... I'll add that.
about why here: dhh added that but I think we stderr here because resque is the only adapter that has a separate gem for enqueue_at/_in and probably users will forget to add it.

@jeremy jeremy commented on the diff Aug 14, 2014
actionmailer/CHANGELOG.md
@@ -1,3 +1,9 @@
+* Make ActionMailer::Previews methods class methods. Previously they were
+ instance methods and ActionMailer tries to render a message when they
+ are called.
+
+ *Cristian Bica*
+
@jeremy
jeremy Aug 14, 2014 Member

Was this from another recent PR?

@seuros
seuros Aug 14, 2014 Member

No. We noticed the bug while working of this PR.

With the new wrapper, the email is lazy loaded.

@sgrif sgrif and 1 other commented on an outdated diff Aug 14, 2014
actionmailer/lib/action_mailer/message_delivery.rb
+ end
+
+ def message #:nodoc:
+ __getobj__
+ end
+
+ def deliver_later!(options={})
+ enqueue_delivery :deliver!, options
+ end
+
+ def deliver_later(options={})
+ enqueue_delivery :deliver, options
+ end
+
+ def method_missing(m, *args, &block)
+ __getobj__.__send__(m, *args, &block)
@sgrif
sgrif Aug 14, 2014 Member

Why even bother inheriting from Delegator, if we still need to do method_missing?

@cristianbica
cristianbica Aug 15, 2014 Contributor

Initially we didn't implement the method_missing here but it was an workaround for some failing tests. Something related to NullMail. I think we can fix the bugs in NullMailer and drop this. I'll take a look

@cristianbica
cristianbica Aug 15, 2014 Contributor

Removed the method_missing

@jeremy jeremy and 1 other commented on an outdated diff Aug 14, 2014
actionmailer/lib/action_mailer/delayed_delivery_job.rb
@@ -0,0 +1,11 @@
+require 'active_job'
+
+module ActionMailer
+ class DelayedDeliveryJob < ActiveJob::Base
@jeremy
jeremy Aug 14, 2014 Member

We aren't actually delaying deliveries. It's just a delivery job.

@seuros
seuros Aug 14, 2014 Member

I will rename it

@jeremy jeremy and 2 others commented on an outdated diff Aug 14, 2014
actionmailer/test/base_test.rb
@@ -4,6 +4,7 @@
require 'action_dispatch'
require 'active_support/time'
+require 'active_support/core_ext/object/itself'
@jeremy
jeremy Aug 14, 2014 Member

Where is #itself used here?

@seuros
seuros Aug 14, 2014 Member

I forgot this require.

We first used it in
LateInlineAttachmentMailer.welcome.itself but decided it make it more clear with .message

@dhh
dhh Aug 16, 2014 Member

Sounds like we can remove this require, then.

@seuros
seuros Aug 16, 2014 Member

Yes, i will do it as part of another commit.

@jeremy jeremy and 2 others commented on an outdated diff Aug 14, 2014
actionmailer/test/message_delivery_test.rb
+ test 'should enqueue a delivery with a delay' do
+ ret = ActionMailer::DelayedDeliveryJob.stub :enqueue_in, ->(*args){ args } do
+ @mail.deliver_later in: 600
+ end
+ assert_equal ret, [600, "DelayedMailer", "test_message", "deliver", 1, 2, 3]
+ end
+
+ test 'should enqueue a delivery at a specific time' do
+ later_time = Time.now.to_i + 3600
+ ret = ActionMailer::DelayedDeliveryJob.stub :enqueue_at, ->(*args){ args } do
+ @mail.deliver_later at: later_time
+ end
+ assert_equal ret, [later_time, "DelayedMailer", "test_message", "deliver", 1, 2, 3]
+ end
+
+end
@jeremy
jeremy Aug 14, 2014 Member

What are these tests telling us? Bunch of stubs?

Also, assertion arguments are expected, actual - so you'll want to flip the order.

@tenderlove
tenderlove Aug 14, 2014 Member

Agree with @jeremy. What is the point of these stubs? It looks like we're just testing stubs rather than testing our system. What is the point?

@jeremy jeremy and 1 other commented on an outdated diff Aug 14, 2014
activejob/lib/active_job/queue_name.rb
@@ -0,0 +1,18 @@
+module ActiveJob
+ module QueueName
+ extend ActiveSupport::Concern
+
+ module ClassMethods
+ mattr_accessor(:queue_base_name) { "active_jobs" }
@jeremy
jeremy Aug 14, 2014 Member

Would like this to be configurable in the Railtie and default to the application name.

@cristianbica
cristianbica Aug 15, 2014 Contributor

Actually I wanted to remove this and add a :default_queue_name. If the queue is specified in the job it will use it or else it will use the default_queue_name.
I see no point in having my queue prefixed by something. For some of the adapter you need to specify on which queue to listen for and for newcomers the default configuration will "not be so good" :). Also the default_queue should be default. I think most of the adapter listen on default so new users will have better defaults.

/cc @dhh

@cristianbica
cristianbica Aug 15, 2014 Contributor

Did the above at seuros/rails@94ae25e. @dhh wanna take a look please?

@tenderlove tenderlove and 1 other commented on an outdated diff Aug 14, 2014
actionmailer/test/message_delivery_test.rb
@@ -0,0 +1,82 @@
+# encoding: utf-8
+gem 'activejob'
+require 'active_job'
+require 'abstract_unit'
+require 'minitest/mock'
+require_relative 'mailers/delayed_mailer'
+
+class MessageDeliveryTest < ActiveSupport::TestCase
+
+ setup do
+ @previous_logger = ActiveJob::Base.logger
+ @previous_delivery_method = ActionMailer::Base.delivery_method
+ ActionMailer::Base.delivery_method = :test
+ ActiveJob::Base.logger = Logger.new('/dev/null')
@tenderlove
tenderlove Aug 14, 2014 Member

Logger.new(nil) will work instead and is cross platform.

@seuros
seuros Aug 14, 2014 Member

Thanks

@jeremy jeremy and 1 other commented on an outdated diff Aug 14, 2014
activejob/lib/rails/generators/job/templates/job.rb
@@ -0,0 +1,9 @@
+<% module_namespacing do -%>
+class <%= class_name %>Job < ActiveJob::Base
+ queue_as :<%= options[:queue] %>
+
+ def perform
@jeremy
jeremy Aug 14, 2014 Member

Should generate a job that takes some arguments so it's clear that's the typical usage. Schedule a job with an AR instance as an argument.

@cristianbica
cristianbica Aug 15, 2014 Contributor

Agree. I'll do that

@tenderlove tenderlove and 2 others commented on an outdated diff Aug 14, 2014
actionmailer/test/message_delivery_test.rb
+ setup do
+ @previous_logger = ActiveJob::Base.logger
+ @previous_delivery_method = ActionMailer::Base.delivery_method
+ ActionMailer::Base.delivery_method = :test
+ ActiveJob::Base.logger = Logger.new('/dev/null')
+ @mail = DelayedMailer.test_message(1, 2, 3)
+ ActionMailer::Base.deliveries.clear
+ end
+
+ teardown do
+ ActiveJob::Base.logger = @previous_logger
+ ActionMailer::Base.delivery_method = @previous_delivery_method
+ end
+
+ test 'should be a MessageDelivery' do
+ assert_equal @mail.class, ActionMailer::MessageDelivery
@tenderlove
tenderlove Aug 14, 2014 Member

Is this actually a requirement? Why does it have to be this class?

@seuros
seuros Aug 14, 2014 Member

ActionMailer::MessageDelivery is a wrapper around Mail::Message

@cristianbica
cristianbica Aug 15, 2014 Contributor

With AJ SomeMailer.some_method_mailer is a wrapper around whatever SomeMailer.some_method_mailer returns. We need to test that somehow. Either by the class name or by any methods implemented on the wrapper.

@tenderlove
tenderlove Aug 15, 2014 Member

Could you assert that the wrapped object doesn't equal the unwrapped one?

@seuros
seuros Aug 15, 2014 Member

assert_not_equal Mail::Message , @mail.class ?

@tenderlove
tenderlove Aug 15, 2014 Member

Is there no way to get the actual message object, not the wrapped one?

@seuros
seuros Aug 15, 2014 Member

@mail.message

@cristianbica
cristianbica Aug 15, 2014 Contributor

@tenderlove as the wrapper is a Delegator we cannot because delegator overrides == and compares the wrapped object (so @mail==@mail.message will be true). What we can assert is that the class of the wrapper is different from the class of the wrapped object: assert_not_equal @mail.class, @mail.message.class. Works for you?

@tenderlove tenderlove and 1 other commented on an outdated diff Aug 14, 2014
actionmailer/test/message_delivery_test.rb
+ ActiveJob::Base.logger = Logger.new('/dev/null')
+ @mail = DelayedMailer.test_message(1, 2, 3)
+ ActionMailer::Base.deliveries.clear
+ end
+
+ teardown do
+ ActiveJob::Base.logger = @previous_logger
+ ActionMailer::Base.delivery_method = @previous_delivery_method
+ end
+
+ test 'should be a MessageDelivery' do
+ assert_equal @mail.class, ActionMailer::MessageDelivery
+ end
+
+ test 'its object should be a Mail::Message' do
+ assert_equal @mail.__getobj__.class, Mail::Message
@tenderlove
tenderlove Aug 14, 2014 Member

Is this public API? Should we really be coupling the tests with internal implementation details like this?

@seuros
seuros Aug 14, 2014 Member

The message is lazy loaded now.
What about

assert_equal Mail::Message, @mail.message.class 
@tenderlove
tenderlove Aug 15, 2014 Member

I don't think you're following me. Why do you care what class it is? Just do assert @mail.message.

@tenderlove tenderlove commented on the diff Aug 14, 2014
activejob/Rakefile
+ errors = []
+
+ tasks.each do |task|
+ begin
+ Rake::Task[task].invoke
+ rescue Exception
+ errors << task
+ end
+ end
+
+ abort "Errors running #{errors.join(', ')}" if errors.any?
+end
+
+task default: :test
+
+ACTIVEJOB_ADAPTERS = %w(inline delayed_job qu que queue_classic resque sidekiq sneakers sucker_punch backburner)
@tenderlove
tenderlove Aug 14, 2014 Member

Are we going to have to update this in parallel with the Gemfile? :'(

@tenderlove
tenderlove Aug 15, 2014 Member

Every time this list changes, do we have to update this list as well?

@DouweM
DouweM Aug 15, 2014 Contributor

Every time tests for an adapter are added, yes.

@cristianbica
cristianbica Aug 15, 2014 Contributor

Well every time a new adapter is added or an adapter removed we have to:

  • update the Gemfile
  • update the activejob/Rakefile
  • update activejob/README.md
  • update the guides
  • and of course delete / add the adapter files for enqueueing and testing

Not very optional :). From the above we can remove most the README.md and point to the guides.

@tenderlove tenderlove commented on the diff Aug 14, 2014
activejob/lib/active_job/logging.rb
@@ -0,0 +1,88 @@
+require 'active_support/core_ext/string/filters'
+
+module ActiveJob
+ module Logging
+ extend ActiveSupport::Concern
+
+ included do
+ cattr_accessor(:logger) { ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(STDOUT)) }
@tenderlove
tenderlove Aug 14, 2014 Member
  1. Does this mean we don't use the logger from the application by default?
  2. If this references ActiveSupport::TaggedLogging and ActiveSupport::Logger, it should also require those files.
@DouweM
DouweM Aug 15, 2014 Contributor
  1. I think the railtie does that.
@seuros
seuros Aug 15, 2014 Member

yes it does

@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

Plus it might be better to use class_attribute, so that it can be overridden without affecting parents.

@cristianbica
cristianbica Aug 15, 2014 Contributor

Yep it set the railtie does it.
I see no need in being able to change the logger just for a ActiveJob::Base subclass. Anyway if anyone wants to do that they can override def logger in that subclass.

@tenderlove tenderlove commented on an outdated diff Aug 14, 2014
activejob/test/cases/job_serialization_test.rb
@@ -0,0 +1,15 @@
+require 'helper'
+require 'jobs/gid_job'
+require 'models/person'
+
+class JobSerializationTest < ActiveSupport::TestCase
+ setup do
+ $BUFFER = []
@tenderlove
tenderlove Aug 14, 2014 Member

Why are you using a global variable here?

@tenderlove tenderlove and 3 others commented on an outdated diff Aug 14, 2014
activejob/test/cases/logging_test.rb
+
+ class TestLogger < ActiveSupport::Logger
+ def initialize
+ @file = StringIO.new
+ super(@file)
+ end
+
+ def messages
+ @file.rewind
+ @file.read
+ end
+ end
+
+ def setup
+ super
+ $BUFFER = []
@tenderlove
tenderlove Aug 14, 2014 Member

Same question.

@seuros
seuros Aug 14, 2014 Member

The worker populate this Global variable there!

@dhh
dhh Aug 14, 2014 Member

I think a global is just fine. The job and the test aren't connected. The global is a stand-in for any other global in your system that the jobs would be working against, like a database.

@tenderlove
tenderlove Aug 15, 2014 Member

@dhh it's not thread or process safe. We should at least be using a thread local variable. Databases have locks for us to coordinate updates, global variables do not. :-)

@dhh
dhh Aug 15, 2014 Member

Why does it matter for these tests though? Is this in preparation for you adding a threaded runner :)?

On Aug 14, 2014, at 18:33, Aaron Patterson notifications@github.com wrote:

In activejob/test/cases/logging_test.rb:

  • class TestLogger < ActiveSupport::Logger
  • def initialize
  •  @file = StringIO.new
    
  •  super(@file)
    
  • end
  • def messages
  •  @file.rewind
    
  •  @file.read
    
  • end
  • end
  • def setup
  • super
  • $BUFFER = []
    @dhh it's not thread or process safe. We should at least be using a thread local variable. Databases have locks for us to coordinate updates, global variables do not. :-)


Reply to this email directly or view it on GitHub.

@tenderlove
tenderlove Aug 15, 2014 Member

Trying to reason about a global variables is extremely annoying -- even in tests. We could at least place a class method on the job itself and mutate that. I don't mind that global state is being changed, I mind that it's not hidden behind methods so we can lock (or do whatever).

@cristianbica
cristianbica Aug 15, 2014 Contributor

I'll try something. I was thinking of a TestingBuffer module that responds to add, include?, clear.

@tenderlove
tenderlove Aug 15, 2014 Member

@cristianbica that would work. I think a thread local variable would be totally fine for now.

@dhh
dhh Aug 15, 2014 Member

YAGNI, in my opinion. When a case arises where we do need to wrap this, we wrap it. It's only in test code and it's only a handful of lines.

On Aug 14, 2014, at 19:17, Aaron Patterson notifications@github.com wrote:

In activejob/test/cases/logging_test.rb:

  • class TestLogger < ActiveSupport::Logger
  • def initialize
  •  @file = StringIO.new
    
  •  super(@file)
    
  • end
  • def messages
  •  @file.rewind
    
  •  @file.read
    
  • end
  • end
  • def setup
  • super
  • $BUFFER = []
    Trying to reason about a global variables is extremely annoying -- even in tests. We could at least place a class method on the job itself and mutate that. I don't mind that global state is being changed, I mind that it's not hidden behind methods so we can lock (or do whatever).


Reply to this email directly or view it on GitHub.

@cristianbica
cristianbica Aug 15, 2014 Contributor

Did a search for $BUFFER and replaced with Thread.current[:ajbuffer]

@tenderlove tenderlove commented on an outdated diff Aug 14, 2014
activejob/test/cases/queuing_test.rb
@@ -0,0 +1,44 @@
+require 'helper'
+require 'jobs/hello_job'
+require 'active_support/core_ext/numeric/time'
+
+
+class QueuingTest < ActiveSupport::TestCase
+ setup do
+ $BUFFER = []
@tenderlove tenderlove commented on an outdated diff Aug 14, 2014
activejob/test/cases/rescue_test.rb
@@ -0,0 +1,23 @@
+require 'helper'
+require 'jobs/rescue_job'
+
+require 'active_support/core_ext/object/inclusion'
+
+class RescueTest < ActiveSupport::TestCase
+ setup do
+ $BUFFER = []
@tenderlove tenderlove and 1 other commented on an outdated diff Aug 14, 2014
activejob/test/jobs/gid_job.rb
@@ -0,0 +1,6 @@
+class GidJob < ActiveJob::Base
+ def perform(person)
+ $BUFFER << "Person with ID: #{person.id}"
@tenderlove
tenderlove Aug 14, 2014 Member

Let's think of a different way to do this besides using a global!

@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

Could at least have a class attribute with the messages you want to store to test against it, it's more self-contained.

@tenderlove tenderlove commented on the diff Aug 14, 2014
activejob/test/jobs/rescue_job.rb
@@ -0,0 +1,20 @@
+class RescueJob < ActiveJob::Base
+ class OtherError < StandardError; end
+
+ rescue_from(ArgumentError) do
@tenderlove
tenderlove Aug 14, 2014 Member

What does this rescue from? When does it rescue? Why does it rescue? Can we just use normal Ruby?

@seuros
seuros Aug 14, 2014 Member

It rescue when perform is called with 'david' as params

@dhh
dhh Aug 14, 2014 Member

rescue_from allows us to declare exceptional handling behavior at the class level. Just like we do with controllers in Action Pack. Same justification.

@DouweM
DouweM Aug 15, 2014 Contributor

It also enables rescuing from deserialization errors (for example a model referenced using global id no longer existing), which wouldn't be possible with begin/rescue inside perform.

@tenderlove
tenderlove Aug 15, 2014 Member

@dhh if this code is from AP, then what are we testing? If we know rescue_from works, why are we testing it?

Besides that, it seems like a great way to accidentally create an infinite loop:

require 'active_job'

class RescueJob < ActiveJob::Base
  rescue_from(ArgumentError) do
    arguments[0] = "DIFFERENT!"
    retry_now
  end

  def perform(name)
    my_helper # oops! I forgot the arg
  end

  def my_helper(name)
    puts "Hello: #{name}"
  end
end

RescueJob.new.execute(SecureRandom.uuid)

@DouweM I don't actually see a place where deserialization will raise an exception, but presumably you could implement execute and call super with a begin / rescue.

@DouweM
DouweM Aug 15, 2014 Contributor

We're testing retry_now and proper falling through of OtherError (there used to be abut there).

Accidental infinite loop because of retry_now, you mean? Doesn't the same go for the retry keyword? Rescuing from ArgumentError is a terrible idea anyway, and just here to illustrate rescue_from's re-enqueueing behavior.

Deserialization calls ActiveModel::GlobalLocator.locate, which calls Model.find, which could definitely raise ActiveRecord::RecordNotFound (https://github.com/rails/activemodel-globalid/blob/master/lib/active_model/global_locator.rb#L7). This is common enough that we don't want people to have to override the internal method execute, and besides, people know rescue_from from AC. Also see the discussion here: rails/activejob#25 which went into another direction as for implementation but concerns the same issue.

@tenderlove
tenderlove Aug 15, 2014 Member

Accidental infinite loop because of retry_now, you mean? Doesn't the same go for the retry keyword? Rescuing from ArgumentError is a terrible idea anyway, and just here to illustrate rescue_from's re-enqueueing behavior.

Yes, retry would have the same behavior. But which of these might look like there's an infinite loop?

def perform(name)
  my_helper # oops! I forgot the arg
end

Or:

def perform(name)
  my_helper # oops! I forgot the arg
rescue ArgumentError
  retry
end

Sorry, I think rescue_from is a bad idea in this context. Surely rescuing some particular exception can't have the same logic for every single method in your object. I don't mind if rescue_from is available in the job class, but we won't be using it in our apps. I'd rather do it the OO way and inherit / call super. If an exception is happening in an unexpected place, we should handle it in that one place, not some disconnected global way.

I'm unsure how a an AJ Job is so similar to a controller that it warrants the same justification for this feature.

@DouweM
DouweM Aug 15, 2014 Contributor

Yes, retry would have the same behavior. But which of these might look like there's an infinite loop?

If we're leaving in the rescue_from block, I think it's about equal, with rescue error one line away from the retry action and just a couple more from the culprit code.

rescue_from(ArgumentError) do
  retry_now
end

def perform(name)
  my_helper # oops! I forgot the arg
end

Both examples are kind of dumb though, because you'd never rescue ArgumentError and retry. In actual non-test jobs, the developer has made the conscious decision the error they're explicitly rescuing from can be safely retried, so an "accidental infinite loop" seems very unlikely to sneak in.

Surely rescuing some particular exception can't have the same logic for every single method in your object.

Good point. I was working under the assumption a job would only have a single #perform method (like most of mine do) and no other methods, so very much like controller actions. In that case rescue_from makes sense, but less so in the situation you describe.

I don't think I'm alone in preferring simple jobs with a #perform that doesn't do much more than calling a handful of methods on another class or a provided model, so I would really like to keep rescue_from in with easy rescuing from ActiveRecord::RecordNotFound, but of course this still leaves people with more complex jobs (like you) free to inherit and call super from execute.

@tenderlove
tenderlove Aug 15, 2014 Member

My point is two fold, 1) if you're looking at the implementation of a method, why would you consider some block that is not part of that method to have any impact on it? and 2) if you're only implementing perform why bother with using some special syntax for rescuing exceptions?

I can see rescue_from to be handy in controllers where you have a bunch of methods that do about the same thing (finding records), but I don't see the parallel between controllers and jobs.

@DouweM
DouweM Aug 15, 2014 Contributor

The whole issue started because I pointed out deserialization with GlobalID could raise ActiveRecord::RecordNotFound, which would lead to an infinitely requeued job on at least Sidekiq (rails/activejob#25). After a little bit of discussion, loaning rescue_from from AC was suggested by @dhh because it would do the trick and be familiar to users.

I'm not married to #rescue_from and I agree that it introduces a couple of new issues/potentially confusing behaviour. In the end, all I would really like is a way to handle those RecordNotFound errors that is straightforward and easy enough to be recommended to and used by less experienced AJ users who use it with GlobalID models.

If we're moving away from rescue_from, we're pretty much restarting that discussion. Do you have any input on the problem described there, any other ideas on ways to handle it?

@tenderlove
tenderlove Aug 15, 2014 Member

Maybe this sample code will drive home the point why I think rescue_from for handling argument deserialization is a bad idea:

class RescueJob < ActiveJob::Base
  rescue_from(ActiveRecord::RecordNotFound) do
    # Was the record not found when deserializing the
    # arguments?  Or did I make a mistake in `perform`?
    retry_now
  end

  def perform(person)
    # oops, I meant friend_id, but I used parent_id, so this may
    # generate a record not found error
    Friend.find person.parent_id
  end
end

I should mention that if you assume the RecordNotFound error originated from deserializing arguments rather than a mistake you made, then you will have another fun case of an infinite loop.

If we want a way for dealing with deserialization errors, we should add a more specific way of doing that. Maybe something like this:

diff --git a/activejob/lib/active_job/execution.rb b/activejob/lib/active_job/execution.rb
index 78ada3d..fbbc378 100644
--- a/activejob/lib/active_job/execution.rb
+++ b/activejob/lib/active_job/execution.rb
@@ -11,7 +11,11 @@ module ActiveJob

     def execute(job_id, *serialized_args)
       self.job_id    = job_id
+      begin
       self.arguments = Arguments.deserialize(serialized_args)
+      rescue => e
+        deserialization_error e
+      end

       run_callbacks :perform do
         perform *arguments
@@ -23,5 +27,9 @@ module ActiveJob
     def perform(*)
       raise NotImplementedError
     end
+
+    def deserialization_error(exception)
+      raise exception
+    end
   end
 end
@dhh
dhh Aug 15, 2014 Member

We are just testing the integration of that library. Hence why there are no tests for all the permutations of reacue_from, just a single test for it.

On Aug 14, 2014, at 19:03, Aaron Patterson notifications@github.com wrote:

In activejob/test/jobs/rescue_job.rb:

@@ -0,0 +1,20 @@
+class RescueJob < ActiveJob::Base

  • class OtherError < StandardError; end
  • rescue_from(ArgumentError) do
    @dhh if this code is from AP, then what are we testing? If we know rescue_from works, why are we testing it?

Besides that, it seems like a great way to accidentally create an infinite loop:

require 'active_job'

class RescueJob < ActiveJob::Base
rescue_from(ArgumentError) do
arguments[0] = "DIFFERENT!"
retry_now
end

def perform(name)
my_helper # oops! I forgot the arg
end

def my_helper(name)
puts "Hello: #{name}"
end
end

RescueJob.new.execute(SecureRandom.uuid)
@DouweM I don't actually see a place where deserialization will raise an exception, but presumably you could implement execute and call super with a begin / rescue.


Reply to this email directly or view it on GitHub.

@cristianbica
cristianbica Aug 15, 2014 Contributor

How about we leave the rescue_from and on deserialization error we raise an ActionJob::DeserializationError in which we give the actual exception--
Cristian Bica

On Fri, Aug 15, 2014 at 7:06 PM, Aaron Patterson notifications@github.com
wrote:

@@ -0,0 +1,20 @@
+class RescueJob < ActiveJob::Base

  • class OtherError < StandardError; end
  • rescue_from(ArgumentError) do
    Maybe this sample code will drive home the point why I think rescue_from for handling argument deserialization is a bad idea:
class RescueJob < ActiveJob::Base
  rescue_from(ActiveRecord::RecordNotFound) do
    # Was the record not found when deserializing the
    # arguments?  Or did I make a mistake in `perform`?
    retry_now
  end
  def perform(person)
    # oops, I meant friend_id, but I used parent_id, so this may
    # generate a record not found error
    Friend.find person.parent_id
  end
end

If we want a way for dealing with deserialization errors, we should add a more specific way of doing that. Maybe something like this:

diff --git a/activejob/lib/active_job/execution.rb b/activejob/lib/active_job/execution.rb
index 78ada3d..fbbc378 100644
--- a/activejob/lib/active_job/execution.rb
+++ b/activejob/lib/active_job/execution.rb
@@ -11,7 +11,11 @@ module ActiveJob
 def execute(job_id, *serialized_args)
   self.job_id    = job_id
  •  begin
    

    self.arguments = Arguments.deserialize(serialized_args)

  •  rescue => e
    
  •    deserialization_error e
    
  •  end
    

    run_callbacks :perform do
    perform arguments
    @@ -23,5 +27,9 @@ module ActiveJob
    def perform(
    )
    raise NotImplementedError
    end
    +

  • def deserialization_error(exception)

  •  raise exception
    
  • end
    end
    end

---
Reply to this email directly or view it on GitHub:
https://github.com/rails/rails/pull/16485/files#r16300021
@tenderlove
tenderlove Aug 15, 2014 Member

@cristianbica that sounds great. Now I wonder, if there's only one method that can raise this exception, what's the point of using rescue_from? I don't mind if we leave it in so people can use it, but I'd like to add this patch:

diff --git a/activejob/lib/active_job/execution.rb b/activejob/lib/active_job/execution.rb
index 78ada3d..fd78a5b 100644
--- a/activejob/lib/active_job/execution.rb
+++ b/activejob/lib/active_job/execution.rb
@@ -11,7 +11,7 @@ module ActiveJob

     def execute(job_id, *serialized_args)
       self.job_id    = job_id
-      self.arguments = Arguments.deserialize(serialized_args)
+      self.arguments = deserialize_arguments serialized_args

       run_callbacks :perform do
         perform *arguments
@@ -23,5 +23,11 @@ module ActiveJob
     def perform(*)
       raise NotImplementedError
     end
+
+    private
+
+    def deserialize_arguments(serialized_args)
+      Arguments.deserialize(serialized_args)
+    end
   end
 end

so that in our job we can deal with fixing the arguments independently from errors in perform:

require 'active_job'

class RescueJob < ActiveJob::Base
  def perform(person)
    # oops, I meant friend_id, but I used parent_id, so this may
    # generate a record not found error, but it will raise an exception
    # and the job stops
    Friend.find person.parent_id
  end

  private

  def deserialize_arguments(args)
    super
  rescue ActionJob::DeserializationError
    fix_the_args(args)
  end
end
@dhh
dhh Aug 15, 2014 Member

I'll follow up with full arguments later, but I remain happy with the inclusion of rescue_from. Nobody is forcing you to use it, just like in controllers. Anyway, I'll expand the argument later, but don't remove this for now.

On Aug 15, 2014, at 9:01, Douwe Maan notifications@github.com wrote:

In activejob/test/jobs/rescue_job.rb:

@@ -0,0 +1,20 @@
+class RescueJob < ActiveJob::Base

  • class OtherError < StandardError; end
  • rescue_from(ArgumentError) do
    The whole issue started because I pointed out deserialization with GlobalID could raise ActiveRecord::RecordNotFound, which would lead to an infinitely requeued job on at least Sidekiq (rails/activejob#25). After a little bit of discussion, loaning rescue_from from AC was suggested by @dhh because it would do the trick and be familiar to users.

I'm not married to #rescue_from and I agree that it introduces a couple of new issues. In the end, all I would really like is a way to handle those RecordNotFound errors that is straightforward and easy enough to be recommended to and used by less experienced AJ users who use it with GlobalID models.

If we're moving away from rescue_from, we're pretty much restarting that discussion. Do you have any input on the problem described there, any other ideas on ways to handle it?


Reply to this email directly or view it on GitHub.

@seuros
seuros Aug 15, 2014 Member

@tenderlove you can add your commit to the actual PR.

@tenderlove
tenderlove Aug 15, 2014 Member

@dhh as long as we have an appropriate methods (like my patch above) then I don't particularly care if rescue_from is included. I do look forward to rebutting your full arguments though. :trollface:

@dhh
dhh Aug 16, 2014 Member

I don't think that's a good change. We should change GlobalID to raise a specific error, so you can rescue that without rescuing generic ArgumentErrors, but it doesn't make sense to me to force people implement their own handling like what's being proposed here.

You don't need rescue_from when you're rescuing app exceptions that stem from your own code that's being run in that #perform method. But it's a great pattern when you're rescuing errors from services outside of your control. The deserialization is one example, here's another we're using pre-AJ in Basecamp (so I've just rewritten as pseudo code here):

class ApplicationJob
  rescue_from(Mysql2::Error) do
    if e.message =~ /can't connect to mysql server/i
      retry_in 3.minutes
    else
      raise
    end  
  end

We have some similar rescues around errors that can come from our CleverSafe file storage system that our jobs interface with. We don't want to rescue those explicitly in #perform.

This is exactly the same motivation we have for rescue_from in AP. You rescue general classes of generic errors, like ActiveRecord::RecordNotFound, and give them generic errors like 404. You then combine that with specific rescues in your action methods, if those are specific to that action.

@tenderlove
tenderlove Aug 16, 2014 Member

force people implement their own handling like what's being proposed here.

I will quote you, "nobody is forcing you to use it". I am proposing that 1) we add a new exception ActionJob::DeserializationError (as @cristianbica suggests), and 2) extract argument deserialization to a method. Nobody is forcing you to implement the extracted method, but you can if you want, and you can still use rescue_from.

As a selfish argument, we'll need this method to migrate custom arg serialization (we're using marshal not JSON). We could end up in infinite loops with rescue_from if we don't have this hook method to distinguish between arg deserialization and job processing.

@dhh
dhh Aug 16, 2014 Member

I'm 👍 on #1, but not really in favor of #2. But if you feel so strongly about it, then alright, let's do that.

@tenderlove
tenderlove Aug 16, 2014 Member

@dhh ❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️❤️😍

@sgrif sgrif commented on the diff Aug 14, 2014
activejob/README.md
@@ -0,0 +1,141 @@
+# Active Job -- Make work happen later
@sgrif
sgrif Aug 14, 2014 Member

Sounds more like a lazy job than an active one. :trollface:

@sgrif sgrif commented on the diff Aug 14, 2014
activejob/lib/active_job/arguments.rb
+ arguments.map { |argument| deserialize_argument(argument) }
+ end
+
+ private
+ def self.serialize_argument(argument)
+ case argument
+ when ActiveModel::GlobalIdentification
+ argument.global_id.to_s
+ when *TYPE_WHITELIST
+ argument
+ when Array
+ serialize(argument)
+ when Hash
+ Hash[ argument.map { |key, value| [ serialize_hash_key(key), serialize_argument(value) ] } ]
+ else
+ raise "Unsupported argument type: #{argument.class.name}"
@sgrif
sgrif Aug 14, 2014 Member

Does this mean that there is no way to provide arguments to a job that aren't primitives? Shouldn't any marshalable object be allowed here?

@seuros
seuros Aug 14, 2014 Member

Yes and No. Sidekiq and Resque serialize objects to json and send it to redis, Qu do that with redis and mongodb.

@sgrif
sgrif Aug 14, 2014 Member

Seems like something we should leave up to the queueing adapters, rather than disallowing entirely, similarly to enqueue_at.

@seuros
seuros Aug 14, 2014 Member

I don't remember well why we did that .
cc @DouweM @dhh @mperham @cristianbica @rafaelfranca

@dhh
dhh Aug 14, 2014 Member

That would prevent you from switching between adapters. The whole point here is to provide a uniform interface, so I don't think that's a good idea.

@sgrif
sgrif Aug 15, 2014 Member

Don't we already do that by providing enqueue_at, which isn't supported universally? The same argument could be made in Active Record for disallowing anything specific to a single database. While it's awesome to provide a common interface, and abstract away what we can, it doesn't make sense to me that we would hamstring basic functionality that is implemented in a similar enough fashion by almost all supported backends.

@dhh
dhh Aug 15, 2014 Member

It's not implemented by all supported backends. Anyone using JSON as the storage format, like Resque, can't deal with arbitrary serialization. Much harder to entangle later than simply dealing with serialization up front if you are to remain queue agnostic -- which is the whole point of ActiveJob.

@sgrif
sgrif Aug 15, 2014 Member

From my point of view, I'd see the use case the same as Active Record, allowing code that provides common functionality to be structured the same way and put in the same place, regardless of the back end being used. Possibly also allowing the decision of which back end to use to be delayed. However, similarly to switching between database implementations later, it seems that switching between queueing implementations after the fact would be less common. If the user wants to be stuck with only queues that use YAML or Marshal for serialization, why shouldn't we let them? Why does this argument not apply to enqueue_at, which also isn't supported by all queueing backends?

@dhh
dhh Aug 15, 2014 Member

I don't think it's the same situation at all. This is something much more likely to trip you up. Some adapters take some params, others don't. It's much clearer to say "delayed job scheduling" is supported here and not there. Way too easy to write code that didn't need to use a custom class and that works on the random queue you're on, then doesn't work on the next. And there's just not enough gain.

@sgrif
sgrif Aug 15, 2014 Member

And there's just not enough gain.

I suppose the relative value depends on how much you're using non-primitives. Forcing users to deal with serializing/deserializing likely removes all ability to duck type arguments to jobs.

@dhh
dhh Aug 15, 2014 Member

I'd be happy to see some job code using lots of non-primitives to reconsider. This is an extraction, and from the usage I've seen so far, I haven't found any that couldn't just as well be primitives plus GID'ables.

@sgrif
sgrif Aug 15, 2014 Member

Example use case from my current project: A job which applies a set of transformations to a file, and moves it to the appropriate location. Arguments are the path of the current file (which is on S3), the expected location (on S3), the bucket name, and an array of transformations (would have been a single object if YAML dumping SimpleDelegator subclasses wasn't such a pain). Simplest case is just providing an encoder which gzips the file, but there are also uses which will include things such as converting a JSON based animation file format to a binary one, minifying ruby code, and losslessly compressing image files.

Allowing me to restrict myself to YAML/Marshal based queues would let me pass in the objects directly. This restriction forces a serializer/deserializer combo, which is tightly coupled to certain implementation details of these objects. The ones that take constructor arguments become especially tricky, and overall I'm forced to switch from object oriented to class oriented, since now all I have to work with are primitives and class objects, which means I need a consistent interface for the constructors of unrelated objects where previously I could just duck type.

@dhh
dhh Aug 15, 2014 Member

Please do post some of this code somewhere, if you can.

@sgrif
sgrif Aug 15, 2014 Member

I don't have a before/after comparison, since we're not using ActiveJob and are using a queueing library that supports YAML, but I can pull out some of the relevant bits as they are today.

@sgrif
sgrif Aug 15, 2014 Member

Here's the job itself, and the basic interface of the most important transformers that we use. https://gist.github.com/sgrif/5920ca3d46a76b9286a0

@sgrif
sgrif Aug 15, 2014 Member

Digging deeper, do any queues use something other than JSON besides Delayed Job and Sucker Punch?

@sgrif sgrif commented on the diff Aug 14, 2014
activejob/lib/active_job/queue_adapter.rb
@@ -0,0 +1,24 @@
+require 'active_job/queue_adapters/inline_adapter'
+require 'active_support/core_ext/string/inflections'
+
+module ActiveJob
+ module QueueAdapter
+ mattr_reader(:queue_adapter) { ActiveJob::QueueAdapters::InlineAdapter }
+
+ def queue_adapter=(name_or_adapter)
+ @@queue_adapter = \
+ case name_or_adapter
+ when Symbol, String
+ load_adapter(name_or_adapter)
+ when Class
@sgrif
sgrif Aug 14, 2014 Member

Seems like providing an object that isn't an instance of Class isn't too far fetched. Maybe change this to else?

@seuros
seuros Aug 15, 2014 Member

You are right. Maybe we should inherit all adapters from AbstractAdapter .

@sgrif
sgrif Aug 15, 2014 Member

Would that mean providing a new adapter would also have to inherit from AbstractAdapter? Is there a problem with just letting any object through that I'm missing?

@seuros
seuros Aug 15, 2014 Member

The adapter should respond enqueue and enqueue_at

@DouweM
DouweM Aug 15, 2014 Contributor

Not a big fan of abstract classes—duck typing and all that. I suggest simply explaining those methods and assuming adapter implementors are sane.

@sgrif
sgrif Aug 15, 2014 Member

👍 for duck typing.

@sgrif sgrif commented on the diff Aug 14, 2014
...b/lib/active_job/queue_adapters/backburner_adapter.rb
@@ -0,0 +1,25 @@
+require 'backburner'
+
+module ActiveJob
+ module QueueAdapters
+ class BackburnerAdapter
+ class << self
+ def enqueue(job, *args)
+ Backburner::Worker.enqueue JobWrapper, [ job.name, *args ], queue: job.queue_name
+ end
+
+ def enqueue_at(job, timestamp, *args)
+ raise NotImplementedError
@sgrif
sgrif Aug 14, 2014 Member

Maybe we should provide an interface to ask about supported features, as well, to ease the burden on plugin maintainers?

@DouweM
DouweM Aug 15, 2014 Contributor

Agreed. Would AJ calling respond_to?(:enqueue_at) on the adapter and raising if false be sufficient? That way adapters wouldn't need to include non-supported methods at all.

@sgrif sgrif commented on the diff Aug 14, 2014
...vejob/lib/active_job/queue_adapters/inline_adapter.rb
@@ -0,0 +1,15 @@
+module ActiveJob
+ module QueueAdapters
+ class InlineAdapter
+ class << self
+ def enqueue(job, *args)
+ job.new.execute *args
+ end
+
+ def enqueue_at(*)
+ raise NotImplementedError.new("Use a queueing backend to enqueue jobs in the future. Read more at https://github.com/rails/activejob")
@sgrif
sgrif Aug 14, 2014 Member

Or just we could just implement with sleep. :trollface:

@seuros
seuros Aug 15, 2014 Member

That will drive users crazy.

@DouweM
DouweM Aug 15, 2014 Contributor

And was discussed in the dedicated repo and ultimately decided against :)

@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

I don't think hardcoding the url here is necessary, just the message about the backend should do (it could also be a bit more explicit)

@phstc
phstc Nov 30, 2016 edited Contributor

@seuros I was wondering if enqueue_at could just delegate to enqueue ignoring the timeout settings.

My motivation is because in development, I would like to change only the adapter from shoryuken to inline, but because of NotImplementedError, I can't. I would need to create my own Inline or add ifs in the code.

If delegating from enqueue_at to enqueue is too , we could also log a warning message saying it's ignoring the timeout settings.

WDYT?

@sgrif sgrif and 2 others commented on an outdated diff Aug 14, 2014
...vejob/lib/active_job/queue_adapters/resque_adapter.rb
@@ -0,0 +1,38 @@
+require 'resque'
+require 'active_support/core_ext/enumerable'
+require 'active_support/core_ext/array/access'
+
+begin
+ require 'resque-scheduler'
+rescue LoadError
+ begin
+ require 'resque_scheduler'
+ rescue LoadError => e
+ $stderr.puts 'The ActiveJob resque adapter requires resque-scheduler. Please add it to your Gemfile and run bundle install'
@sgrif
sgrif Aug 14, 2014 Member

Should we allow use without resque-scheduler for users who aren't using enqueue_at?

@dhh
dhh Aug 14, 2014 Member

I'd be OK with a version that basically raised like it does in the not-implemented cases.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Aug 14, 2014
actionmailer/lib/action_mailer/base.rb
@@ -548,8 +548,8 @@ def set_payload_for_mail(payload, mail) #:nodoc:
end
def method_missing(method_name, *args) # :nodoc:
- if respond_to?(method_name)
- new(method_name, *args).message
+ if action_methods.include?(method_name.to_s)
+ MessageDelivery.new(self, method_name, *args)
@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Aug 14, 2014
actionmailer/lib/action_mailer/delayed_delivery_job.rb
@@ -0,0 +1,11 @@
+require 'active_job'
+
+module ActionMailer
+ class DelayedDeliveryJob < ActiveJob::Base
+ queue_as :mailers
+
+ def perform(mailer, mail_method, delivery_method, *args)
+ mailer.constantize.send(mail_method, *args).send(delivery_method)
@carlosantoniodasilva
carlosantoniodasilva Aug 14, 2014 Member

Can we rely on public_send instead?

@carlosantoniodasilva carlosantoniodasilva commented on the diff Aug 14, 2014
actionmailer/lib/action_mailer/message_delivery.rb
@@ -0,0 +1,47 @@
+module ActionMailer
+ class MessageDelivery < Delegator
@carlosantoniodasilva
carlosantoniodasilva Aug 14, 2014 Member

It might be good to require the delegate.

@carlosantoniodasilva carlosantoniodasilva commented on the diff Aug 14, 2014
actionmailer/lib/action_mailer/message_delivery.rb
@@ -0,0 +1,47 @@
+module ActionMailer
+ class MessageDelivery < Delegator
+ def initialize(mailer, mail_method, *args)
+ @mailer = mailer
+ @mail_method = mail_method
+ @args = args
+ end
+
+ def __getobj__
+ @obj ||= @mailer.send(:new, @mail_method, *@args).message
@carlosantoniodasilva
carlosantoniodasilva Aug 14, 2014 Member

public_send? (or can't you just call new?)

@seuros
seuros Aug 15, 2014 Member

private_class_method :new #:nodoc:

@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

Well, it's probably private for a good reason then, and we shouldn't mess up with it :)

@cristianbica
cristianbica Aug 15, 2014 Contributor

That method is private for 10 years (it's from the first public commit :) ). The method is private because AM creates an instance internally in method_missing. Now the AM instance is created from the MessageDelivery class.

@sgrif
sgrif Aug 15, 2014 Member

Why not just make it public?

@seuros
seuros Aug 15, 2014 Member

We could!
ping @dhh

@dhh
dhh Aug 16, 2014 Member

I think the reason we don't to expose new is that all other calls go through the singleton. You're supposed to call NotificationMailer.my_action(arg).deliver -- not NotificationMailer.new.my_action(arg).deliver. So I don't think we should mess with that pattern for now.

@sgrif
sgrif Aug 16, 2014 Member

Isn't it reasonable that if we have this need for our new additions, other plugin makers may need the same thing?

@dhh
dhh Aug 16, 2014 Member

I’ve never heard of that need in the 10 years so far. So I’d like to see a real case before going public with it.

On Aug 16, 2014, at 10:23 AM, Sean Griffin notifications@github.com wrote:

In actionmailer/lib/action_mailer/message_delivery.rb:

@@ -0,0 +1,47 @@
+module ActionMailer

  • class MessageDelivery < Delegator
  • def initialize(mailer, mail_method, *args)
  •  @mailer = mailer
    
  •  @mail_method = mail_method
    
  •  @args = args
    
  • end
  • def getobj
  •  @obj ||= @mailer.send(:new, @mail_method, *@args).message
    
    Isn't it reasonable that if we have this need for our new additions, other plugin makers may need the same thing?


Reply to this email directly or view it on GitHub.

@sgrif
sgrif Aug 16, 2014 Member

This case doesn't count?

@dhh
dhh Aug 16, 2014 Member

It counts for 1. I usually like to see at least 2, preferably more.

@sgrif sgrif commented on the diff Aug 14, 2014
activejob/lib/active_job/arguments.rb
+ arguments.map { |argument| serialize_argument(argument) }
+ end
+
+ def self.deserialize(arguments)
+ arguments.map { |argument| deserialize_argument(argument) }
+ end
+
+ private
+ def self.serialize_argument(argument)
+ case argument
+ when ActiveModel::GlobalIdentification
+ argument.global_id.to_s
+ when *TYPE_WHITELIST
+ argument
+ when Array
+ serialize(argument)
@sgrif
sgrif Aug 14, 2014 Member

Where is the serialize method coming from?

@DouweM
DouweM Aug 15, 2014 Contributor

Check line 8.

@sgrif
sgrif Aug 15, 2014 Member

...I appear to be blind.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Aug 14, 2014
actionmailer/lib/action_mailer/message_delivery.rb
+ def method_missing(m, *args, &block)
+ __getobj__.__send__(m, *args, &block)
+ end
+
+ private
+ def enqueue_delivery(delivery_method, options={})
+ args = @mailer.name, @mail_method.to_s, delivery_method.to_s, *@args
+ enqueue_method = :enqueue
+ if options[:at]
+ enqueue_method = :enqueue_at
+ args.unshift options[:at]
+ elsif options[:in]
+ enqueue_method = :enqueue_in
+ args.unshift options[:in]
+ end
+ ActionMailer::DelayedDeliveryJob.send enqueue_method, *args
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Aug 14, 2014
actionmailer/lib/action_mailer/message_delivery.rb
+ end
+
+ def deliver_later!(options={})
+ enqueue_delivery :deliver!, options
+ end
+
+ def deliver_later(options={})
+ enqueue_delivery :deliver, options
+ end
+
+ def method_missing(m, *args, &block)
+ __getobj__.__send__(m, *args, &block)
+ end
+
+ private
+ def enqueue_delivery(delivery_method, options={})
@carlosantoniodasilva
carlosantoniodasilva Aug 14, 2014 Member

Options are always passed, there's no need to make this arg optional.

@carlosantoniodasilva carlosantoniodasilva commented on the diff Aug 14, 2014
activejob/README.md
+The main point is to ensure that all Rails apps will have a job infrastructure
+in place, even if it's in the form of an "immediate runner". We can then have
+framework features and other gems build on top of that, without having to worry
+about API differences between Delayed Job and Resque. Picking your queuing
+backend becomes more of an operational concern, then. And you'll be able to
+switch between them without having to rewrite your jobs.
+
+
+## Usage
+
+Set the queue adapter for Active Job:
+
+``` ruby
+ActiveJob::Base.queue_adapter = :inline # default queue adapter
+# Adapters currently supported: :backburner, :delayed_job, :qu, :que, :queue_classic,
+# :resque, :sidekiq, :sneakers, :sucker_punch
@carlosantoniodasilva
carlosantoniodasilva Aug 14, 2014 Member

Could just point to the list of adapters below, which could include the related symbols, so that we have a single update place.

@sgrif sgrif and 3 others commented on an outdated diff Aug 14, 2014
activejob/test/models/person.rb
@@ -0,0 +1,19 @@
+require 'active_model/global_identification'
+
+class Person
+ include ActiveModel::GlobalIdentification
+
+ attr_reader :id
+
+ def self.find(id)
+ new(id)
@sgrif
sgrif Aug 14, 2014 Member

Unannounced feature of 4.2, we found Waldo!

@dhh
dhh Aug 16, 2014 Member

Maybe that was used in an earlier test. If nothing fails when you remove, feel free to cut.

@dhh
dhh Aug 16, 2014 Member

Actually, this is used. It's just a stub. We don't need to have this actually backed by anything to test what we need to test. So this is as it should be.

@carlosantoniodasilva carlosantoniodasilva and 2 others commented on an outdated diff Aug 14, 2014
activejob/activejob.gemspec
+ s.summary = 'Job framework with pluggable queues.'
+ s.description = 'Declare job classes that can be run by a variety of queueing backends.'
+
+ s.required_ruby_version = '>= 1.9.3'
+
+ s.license = 'MIT'
+
+ s.author = 'David Heinemeier Hansson'
+ s.email = 'david@loudthinking.com'
+ s.homepage = 'http://www.rubyonrails.org'
+
+ s.files = Dir['CHANGELOG.md', 'MIT-LICENSE', 'README.md', 'lib/**/*']
+ s.require_path = 'lib'
+
+ s.add_dependency 'activesupport', version
+ s.add_dependency 'activemodel-globalid'
@carlosantoniodasilva
carlosantoniodasilva Aug 14, 2014 Member

Is this supported, or required?

@DouweM
DouweM Aug 15, 2014 Contributor

We want to promote this way of passing models along with a job as much as possible, so I'd say required to have it not take the extra step of adding a requirement to the Gemfile.

@dhh
dhh Aug 15, 2014 Member

Required. Definitely. Big part of the appeal of ActiveJob in general.

@sgrif
Member
sgrif commented Aug 14, 2014

I might have missed it, but does this set the queueing backend to :inline by default in the test environment? Have we considered what testing will look like for apps that use enqueue_at?

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Aug 14, 2014
activejob/lib/active_job/arguments.rb
+require 'active_model/global_identification'
+
+module ActiveJob
+ class Arguments
+ TYPE_WHITELIST = [ NilClass, Fixnum, Float, String, TrueClass, FalseClass, Bignum ]
+
+ def self.serialize(arguments)
+ arguments.map { |argument| serialize_argument(argument) }
+ end
+
+ def self.deserialize(arguments)
+ arguments.map { |argument| deserialize_argument(argument) }
+ end
+
+ private
+ def self.serialize_argument(argument)
@carlosantoniodasilva
carlosantoniodasilva Aug 14, 2014 Member

These are not actually private, you must either move them inside a class << self for private to work, or use private_class_method :serialize_argument

@carlosantoniodasilva
carlosantoniodasilva Aug 14, 2014 Member

Actually, why is this a class if we just make use of class methods? Could either be a module, or actually use a class by instantiating it to do the heavy lifting.

@DouweM
DouweM Aug 15, 2014 Contributor

Agreed, it could/should be a module. I think previously it was used to instantiate models as well.

@carlosantoniodasilva carlosantoniodasilva and 1 other commented on an outdated diff Aug 14, 2014
activejob/lib/active_job/callbacks.rb
+ def around_perform(*filters, &blk)
+ set_callback(:perform, :around, *filters, &blk)
+ end
+
+
+ def before_enqueue(*filters, &blk)
+ set_callback(:enqueue, :before, *filters, &blk)
+ end
+
+ def after_enqueue(*filters, &blk)
+ set_callback(:enqueue, :after, *filters, &blk)
+ end
+
+ def around_enqueue(*filters, &blk)
+ set_callback(:enqueue, :around, *filters, &blk)
+ end
@carlosantoniodasilva
carlosantoniodasilva Aug 14, 2014 Member

It might be good to document those too.

@cristianbica
cristianbica Aug 15, 2014 Contributor

added

@carlosantoniodasilva carlosantoniodasilva commented on the diff Aug 15, 2014
activejob/lib/active_job/enqueuing.rb
+ #
+ # Returns an instance of the job class queued with args available in
+ # Job#arguments and the timestamp in Job#enqueue_at.
+ def enqueue_in(interval, *args)
+ enqueue_at interval.seconds.from_now, *args
+ end
+
+ # Enqueue a job to be performed at an explicit point in time.
+ #
+ # enqueue_at(Date.tomorrow.midnight, "mike")
+ #
+ # Returns an instance of the job class queued with args available in
+ # Job#arguments and the timestamp in Job#enqueue_at.
+ def enqueue_at(timestamp, *args)
+ new(args).tap do |job|
+ job.enqueued_at = timestamp
@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

This attribute seems a bit misleading to me, enqueued_at gives me the understanding of the time it's being enqueued, eg Time.current or created_at, different from enqueue_at. Does that make sense?

@DouweM
DouweM Aug 15, 2014 Contributor

Agreed. How about enqueued_for? Reads like "enqueued for tomorrow 3am" rather than "enqueued at noon today" which sounds like the time it was enqueued.

@seuros
seuros Aug 15, 2014 Member

What about aliasing both enqueued_for and enqueued_at

@cristianbica
cristianbica Aug 15, 2014 Contributor

Enqueue for later is supported by 3 adapters:

  • delayed_job: Something.delay(run_at: 10.hours.from_now).a_method
  • resque: Resque.enqueue_at_with_queue job.queue_name, timestamp...
  • sidekiq: Sidekiq::Client.push at: timestamp, class:....

Pretty obvious where the _at came from :)

@DouweM
DouweM Aug 15, 2014 Contributor

Sure, but for a timestamp attribute it's confusing vs created_at etc. I think aliasing is a good compromise.

@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

Why not just rename it to the actual argument: enqueue_at

@dhh
dhh Aug 16, 2014 Member

I think enqueue_at makes perfect sense. There's no guarantee that the queue will run the job precisely at that second. It'll simply enqueue it for processing then, which depending on how busy it is, may well mean processing some time later. So 👎 on rename or alias.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Aug 15, 2014
activejob/lib/active_job/execution.rb
+require 'active_job/arguments'
+
+module ActiveJob
+ module Execution
+ extend ActiveSupport::Concern
+
+ included do
+ include ActiveSupport::Rescuable
+ end
+
+ def execute(job_id, *serialized_args)
+ self.job_id = job_id
+ self.arguments = Arguments.deserialize(serialized_args)
+
+ run_callbacks :perform do
+ perform *arguments
@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

I believe this yields a ruby warning, so might be good to wrap with ()

@carlosantoniodasilva carlosantoniodasilva commented on the diff Aug 15, 2014
activejob/lib/active_job/identifier.rb
@@ -0,0 +1,15 @@
+require 'active_job/arguments'
@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

I guess you don't need this require, but you do need the one for secure random

@carlosantoniodasilva carlosantoniodasilva commented on the diff Aug 15, 2014
activejob/lib/active_job/identifier.rb
@@ -0,0 +1,15 @@
+require 'active_job/arguments'
+
+module ActiveJob
+ module Identifier
+ extend ActiveSupport::Concern
+
+ included do
+ attr_writer :job_id
+ end
@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

There's no need for AS::Concern to just add a writer, you can do it on the module itself:

require 'securerandom'

module Identifier
  attr_writer :job_id

  def job_id
    @job_id ||= SecureRandom.uuid
  end
end

class Zomg
  include Identifier
end

z = Zomg.new
puts z.job_id
z.job_id = "lol"
puts z.job_id

=begin
1086b0a6-ba23-4d30-96d5-b3f7523b7fb3
lol
=end
@carlosantoniodasilva carlosantoniodasilva commented on the diff Aug 15, 2014
activejob/lib/active_job/logging.rb
+ extend ActiveSupport::Concern
+
+ included do
+ cattr_accessor(:logger) { ActiveSupport::TaggedLogging.new(ActiveSupport::Logger.new(STDOUT)) }
+
+ around_enqueue do |_, block, _|
+ tag_logger do
+ block.call
+ end
+ end
+
+ around_perform do |job, block, _|
+ tag_logger(job.class.name, job.job_id) do
+ payload = {adapter: job.class.queue_adapter, job: job.class, args: job.arguments}
+ ActiveSupport::Notifications.instrument("perform_start.active_job", payload.dup)
+ ActiveSupport::Notifications.instrument("perform.active_job", payload) do
@DouweM
DouweM Aug 15, 2014 Contributor

We want to log before and after performing, check lines 60 and 64. Looks to me like this is the only way.

@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

As far as I'm aware of, log subscriber already has code to deal with this and you can use the duration to do the logging. I'd rather not have two of these per job being performed.

@tenderlove might be able to tell us more about it.

@DouweM
DouweM Aug 15, 2014 Contributor

As far as I can see from the source, LogSubscriber does not call any method on itself when instrumentation starts, just when it finishes: source (start and finish are called by Notifications before and after the block is yielded). We want a separate log message when the job starts, which at this point requires a separate instrumentation.

I wouldn't mind changing LogSubscriber.start to call #{name}_start if that method exists, but that's not backward compatible if people are already using their own _start event.

/cc @tenderlove

@carlosantoniodasilva carlosantoniodasilva commented on the diff Aug 15, 2014
activejob/lib/active_job/queue_adapter.rb
+ mattr_reader(:queue_adapter) { ActiveJob::QueueAdapters::InlineAdapter }
+
+ def queue_adapter=(name_or_adapter)
+ @@queue_adapter = \
+ case name_or_adapter
+ when Symbol, String
+ load_adapter(name_or_adapter)
+ when Class
+ name_or_adapter
+ end
+ end
+
+ private
+ def load_adapter(name)
+ require "active_job/queue_adapters/#{name}_adapter"
+ "ActiveJob::QueueAdapters::#{name.to_s.camelize}Adapter".constantize
@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

You could call this method with a to_s argument to avoid more strings here.

@carlosantoniodasilva carlosantoniodasilva commented on the diff Aug 15, 2014
activejob/lib/active_job/queue_adapters/qu_adapter.rb
@@ -0,0 +1,30 @@
+require 'qu'
+
+module ActiveJob
+ module QueueAdapters
+ class QuAdapter
+ class << self
+ def enqueue(job, *args)
+ Qu::Payload.new(klass: JobWrapper, args: [job.name, *args]).tap do |payload|
+ payload.instance_variable_set(:@queue, job.queue_name)
@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

This seems a little awkward, having to metaprogram like that to set the queue name. Doesn't it provide an interface for doing so?

@cristianbica
cristianbica Aug 15, 2014 Contributor

Yep. When I did some integration testing I noticed that for Qu all the jobs were pushed to the default queue. When Qu pushes the job on the queue it calls (klass.instance_variable_get(:@queue) || 'default').to_s to determine the queue.

@carlosantoniodasilva carlosantoniodasilva commented on the diff Aug 15, 2014
...vejob/lib/active_job/queue_adapters/resque_adapter.rb
@@ -0,0 +1,38 @@
+require 'resque'
+require 'active_support/core_ext/enumerable'
+require 'active_support/core_ext/array/access'
@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

It seems these requires could be ✂️

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Aug 15, 2014
activejob/test/helper.rb
@@ -0,0 +1,28 @@
+require File.expand_path('../../../load_paths', __FILE__)
+
+require 'active_job'
+
+@adapter = ENV['AJADAPTER'] || 'inline'
@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

There is no need for this to be an instance variable.

@carlosantoniodasilva carlosantoniodasilva commented on the diff Aug 15, 2014
...ejob/test/support/delayed_job/delayed/backend/test.rb
+
+# An in-memory backend suitable only for testing. Tries to behave as if it were an ORM.
+module Delayed
+ module Backend
+ module Test
+ class Job
+ attr_accessor :id
+ attr_accessor :priority
+ attr_accessor :attempts
+ attr_accessor :handler
+ attr_accessor :last_error
+ attr_accessor :run_at
+ attr_accessor :locked_at
+ attr_accessor :locked_by
+ attr_accessor :failed_at
+ attr_accessor :queue
@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

Just pass all of them into the same accessor call.

@carlosantoniodasilva
carlosantoniodasilva Aug 15, 2014 Member

Hm now I noticed it was a copy from DJ 😢

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Aug 15, 2014
guides/source/active_job_basics.md
+ <td>WIP</td>
+ <td>No</td>
+ <td>No</td>
+ <td>No</td>
+ </tr>
+ <tr>
+ <td><strong>Active Job Inline</strong></td>
+ <td>No</td>
+ <td>Yes</td>
+ <td>N/A</td>
+ <td>N/A</td>
+ <td>N/A</td>
+ <td>N/A</td>
+ </tr>
+ </tbody>
+</table>
@tenderlove tenderlove commented on the diff Aug 15, 2014
activejob/lib/active_job/gem_version.rb
@@ -0,0 +1,15 @@
+module ActiveJob
+ # Returns the version of the currently loaded ActiveJob as a <tt>Gem::Version</tt>
+ def self.gem_version
+ Gem::Version.new VERSION::STRING
+ end
@tenderlove
tenderlove Aug 15, 2014 Member

Is this actually necessary? You can get the currently loaded version of a gem from RubyGems:

irb(main):001:0> require 'nokogiri'
=> true
irb(main):002:0> Gem.loaded_specs['nokogiri'].version
=> #<Gem::Version "1.6.3.1">
irb(main):003:0>

Also, what does anyone use this for?

@chancancode
chancancode Aug 15, 2014 Member

This was added in #14101. IIRC the motivation was to make comparison easy for gem/plugin authors (if ActiveRecord.gem_version > ...)

@tenderlove
tenderlove Aug 15, 2014 Member

@chancancode ya, but that was before we knew that you could get the info from RubyGems. I think we should encourage people to query RG for the loaded gem info and rm this code.

@chancancode chancancode and 2 others commented on an outdated diff Aug 15, 2014
activejob/lib/active_job/enqueuing.rb
@@ -0,0 +1,71 @@
+require 'active_job/arguments'
+
+module ActiveJob
+ module Enqueuing
+ extend ActiveSupport::Concern
+
+ module ClassMethods
+ # Push a job onto the queue. The arguments must be legal JSON types
+ # (string, int, float, nil, true, false, hash or array) or
+ # ActiveModel::GlobalIdentication instances. Arbitrary Ruby objects
@chancancode
chancancode Aug 15, 2014 Member

Global Identification?

@DouweM
DouweM Aug 15, 2014 Contributor
assert_equal "Identification", "Identication" #=> fail
@tenderlove
Member

Where does the test harness set up redis queues and pg tables for testing the different queues? I can't seem to find it.

@cristianbica
Contributor

@tenderlove We have integration tests rails/activejob#102 but:
I wanted to drop them seuros/actionmailer-deliver_later#6 (comment)
@ddh said no :) seuros/actionmailer-deliver_later#6 (comment)
@rafaelfranca said to make separate PR with them https://github.com/seuros/rails/issues/1#issuecomment-51945362
So I'll make the PR once we get this PR merged

@dhh

I'd prefer to leave the default queue name to be 'active_jobs'. 'default' doesn't give you any clue where it came from, and you didn't define it yourself, so you are not expecting it.

@DouweM
Contributor
DouweM commented Aug 16, 2014

Not related to the PR itself, but I was wondering: what makes some part of Rails "active_" or "action_"? "Active record" was an existing term of course, but what makes ActionMailer and ActiveJob, action and active, relatively?

@dhh
Member
dhh commented Aug 16, 2014

I've used Action for anything that is frontend aimed (controller/views), Active for backend (models etc).

On Aug 16, 2014, at 12:20, Douwe Maan notifications@github.com wrote:

Not related to the PR itself, but I was wondering: what makes some part of Rails "active_" or "action_"? "Active record" was an existing term of course, but what makes ActionMailer and ActiveJob, action and active, relatively?


Reply to this email directly or view it on GitHub.

@DouweM
Contributor
DouweM commented Aug 16, 2014

All right, curiosity satisfied :)

@dhh dhh merged commit 49c9f85 into rails:master Aug 17, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
@jeremy
Member
jeremy commented Aug 18, 2014

🤘

@bf4
Contributor
bf4 commented on 080296b Aug 18, 2014

Hey buddy. Neat looking project, eh?

@chancancode
Member

@seuros is this a bug fix? do we need to backport this to 4-1-stable as well? I noticed that it already works as a class method there without this patch, so I'm a bit confused. Do you remember what this was about?

Member

Backported 😄

06b185c

cc @pixeltrix

Member

👍

Member

@seuros @chancancode is there an issue for this ?

Member

in 4.1 it's not an actual bug as the mailers are working and calling .register_preview_interceptor will eventually call the instance method. It's not optimal as it creates an ActionMailer::Base instance and tries to render an email
in 4.2 with the introduction of activejob mailer methods are no longer called until it's needed so the tests for register_preview_interceptor started failing and we noticed that bug
@cristianbica

Member

@pixeltrix nope this was part of the Active Job pull request. I checked that the register_interceptors method is also defined as a class method, so I assumed this is "working by accident" (that it happens to go through method missing, creates an instance and call the same method on it). Feel free to revert or amend if I'm wrong.

Member

@chancancode no, you're right - looks like I missed the class << self when looking at the existing register_interceptors code and it worked by accident.

@cristianbica
Contributor

@tenderlove here are the integration tests
#16541

Cristian Bica

On Fri, Aug 15, 2014 at 5:07 AM, Aaron Patterson notifications@github.com
wrote:

Where does the test harness set up redis queues and pg tables for testing
the different queues? I can't seem to find it.


Reply to this email directly or view it on GitHub
#16485 (comment).

@jGRUBBS

@mperham why did you remove the later method and raise NIE? I haven't specifically tested this myself, but the later method seems to be appropriate here.

Contributor

Find the associated PR and conversation for this commit.

@zhouguangming
Contributor

👍

@jGRUBBS
jGRUBBS commented Aug 27, 2014

@mperham I am looking at the pull request here rails/activejob#35
I still don't see any rationale behind removing the later method in the sucker_punch_adapter unless you have any reason as to why it was removed, could it be added back?

@cristianbica
Contributor

@jGRUBBS it was proposed again in rails/rails #16643 but because Sucker Punch does not have a persistent store we decided not to accept it.

@jGRUBBS
jGRUBBS commented Aug 27, 2014

@cristianbica thanks for the explanation, now I see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment