New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make AJ::Base#enqueue return false if the job wasn't enqueued #33992

Merged
merged 1 commit into from Dec 5, 2018

Conversation

Projects
None yet
5 participants
@kirs
Copy link
Member

kirs commented Sep 26, 2018

Before this patch, there's no way to know if the job was really enqueued if there's a possibility that callback chain was aborted from before_enqueue:

class AbortBeforeEnqueueJob < ActiveJob::Base
  before_enqueue { throw(:abort) }
end

> AbortBeforeEnqueueJob.new.enqueue
=> #<AbortBeforeEnqueueJob:0xXXXXXX @arguments=[], @job_id="0d5ef7b9-d32b-4c83-b8cf-44ec31e9d6d9" ...

After this patch, enqueue may return false if the job wasn't enqueued:

> AbortBeforeEnqueueJob.new.enqueue
=> false

This follows ActiveSupport::Callbacks design:

If the callback chain was halted, returns false. Otherwise returns the result of the block, nil if no callbacks have been set, or true if callbacks have been set but no block is given.

I understand it could be considered as a breaking change because it modifies #enqueue return result, but I think it's worth it as it will allow us to control things better.

@Edouard-chin @rafaelfranca

@rails-bot

This comment has been minimized.

Copy link

rails-bot commented Sep 26, 2018

r? @kamipo

(@rails-bot has picked a reviewer for you, use r? to override)

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Sep 26, 2018

It does make sense to me but it is an annoying breaking change to deal with. I wonder if there is a way to us to deprecate the return of self in case the callback chain was halted. Maybe:

  def enqueue(options = {})
       self.scheduled_at = options[:wait].seconds.from_now.to_f if options[:wait]
       self.scheduled_at = options[:wait_until].to_f if options[:wait_until]
       self.queue_name   = self.class.queue_name_from_part(options[:queue]) if options[:queue]
       self.priority     = options[:priority].to_i if options[:priority]
       successfully_enqueued = false
       run_callbacks :enqueue do
         if scheduled_at
           self.class.queue_adapter.enqueue_at self, scheduled_at
        else
          self.class.queue_adapter.enqueue self
        end
        successfully_enqueued = true
      end
      if successfully_enqueued
        self
      else
        if some_config_that_tell_people_is_expecting_false
          false
        else
          Deprecation.warn "this will return false, enable some_config_that_tell_people_is_expecting_false to remove deprecation."
          self
        end
      end
    end

@gmcgibbon gmcgibbon added the activejob label Sep 27, 2018

@kirs kirs force-pushed the kirs:enqueue-return-false branch from 8114810 to ee9fc12 Oct 28, 2018

@rails-bot rails-bot bot added the railties label Oct 28, 2018

@kirs

This comment has been minimized.

Copy link
Member

kirs commented Oct 28, 2018

@rafaelfranca thanks for the suggestion. I updated the PR and new framework defaults too.

@kirs

This comment has been minimized.

Copy link
Member

kirs commented Nov 3, 2018

@rafaelfranca rafaelfranca added this to the 6.0.0 milestone Nov 8, 2018

@rafaelfranca rafaelfranca merged commit 299a213 into rails:master Dec 5, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment