ActiveJob perform_now no longer returns result of perform

[cpruitt]

Recently, ActiveJob's .perform_now method was updated in #37950 to return a boolean value indicating success or failure. Prior to the change, .perform_now returned the result of .run_callbacks, which in turn returned the result of perform(*arguments).

See: 9eb4b4e#diff-141fdcf8e028bfeca079b4ebb5cb00a3R41

It looks like the change was made to support improvements to deprecation warnings. Unfortunately it is causing problems at GitHub as we have quite a few tests (and a small number of methods in production) that rely on .perform_now returning the result of .perform. It also seems pretty likely that dependence on the previous return value isn't unique to GitHub.

The following reproduction script can be used to run an example using Rails 6.0 and 6.1

$ ruby activejob_returnval.rb && RAILS_MASTER=1 ruby activejob_returnval.rb

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  if ENV['RAILS_MASTER'] == "1"
    gem "rails", github: "rails/rails"
  else
    gem "rails", "6.0.0"
  end
end

require "active_job"
require "minitest/autorun"

class User
  attr_accessor :id
  def initialize(id)
    @id = id
  end

  def self.all
    (1..10).collect {|i| User.new(i)}
  end

  def has_warning?
    self.id.even?
  end

  def send_warning
    # Send a notification
  end
end

class BuggyJob < ActiveJob::Base
  def perform
    user_ids_with_warnings = []
    User.all.each do |user|
      if user.has_warning?
        user.send_warning
        user_ids_with_warnings << user.id
      end
    end
    user_ids_with_warnings
  end
end

class BuggyJobTest < ActiveJob::TestCase
  def test_return_value_returns_perform_result
    result = BuggyJob.perform_now
    assert_equal [2,4,6,8,10], result
  end
end

[cpruitt]

Oops, too quick on the submit.
CC @Edouard-chin as the author of the referenced PR and @eileencodes for visibility

[cpruitt]

Some additional information and another related problem introduced by #37950. This line:

9eb4b4e#diff-141fdcf8e028bfeca079b4ebb5cb00a3R43

always returns true from the block instead of the result of perform as was previously returned. This results in true being the return value from yield in callbacks. The following works with Rails 6.0 but not with 6.1:

class StatsLoggedJob < ActiveJob::Base
  around_perform :log_results
  def log_results
    result = yield
    FakeStatsLogger.log_stats({ stats_entry: result })
  end
  # ... [snip]
end

[cpruitt]

Here is an updated version of the above reproduction script which shows both of the described issues:

$ ruby activejob_returnval.rb && RAILS_MASTER=1 ruby activejob_returnval.rb

activejob_returnval.rb

# frozen_string_literal: true

require "bundler/inline"

gemfile(false) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  if ENV['RAILS_MASTER'] == "1"
    gem "rails", github: "rails/rails"
  else
    gem "rails", "6.0.0"
  end
end

require "active_job"
require "minitest/autorun"

class User
  attr_accessor :id
  def initialize(id)
    @id = id
  end

  def self.all
    (1..10).collect {|i| User.new(i)}
  end

  def has_warning?
    self.id.even?
  end

  def send_warning
    # Send a notification
  end
end

class FakeStatsLogger
  cattr_accessor :logs
  def self.log_stats(value)
    self.logs ||= []
    self.logs << value
  end
end

class BuggyJob < ActiveJob::Base
  around_perform :log_results

  def log_results
    result = yield
    FakeStatsLogger.log_stats({ stats_entry: result })
  end

  def perform
    user_ids_with_warnings = []
    User.all.each do |user|
      if user.has_warning?
        user.send_warning
        user_ids_with_warnings << user.id
      end
    end
    user_ids_with_warnings
  end
end

class BuggyJobTest < ActiveJob::TestCase
  def test_return_value_returns_perform_result
    result = BuggyJob.perform_now
    assert_equal [2,4,6,8,10], result
  end

  def test_around_perform_yield_returns_perform_result
    result = BuggyJob.perform_now
    assert_equal ({ stats_entry: [2,4,6,8,10] }), FakeStatsLogger.logs.last
  end
end

[kaspth]

I'll let @eileencodes make the final call but from what I could see in the documentation we never documented the return value of perform_now and I'd consider relying on it private API. Ref: https://api.rubyonrails.org/classes/ActiveJob/Execution.html#method-i-perform_now

Is the BuggyJob sample you gave above representative of the actual failing job at GitHub? Otherwise if the job is just collecting the user ids in memory without either logging them or putting them in a Redis somewhere, it seems like quite the edge case to me.

[Edouard-chin]

I think my change introduced a regression. It is a documented feature in AS::Callbacks

rails/activesupport/lib/active_support/callbacks.rb

Lines 650 to 651 in 1421ad5

	# Around callbacks can access the return value from the event, if it
	# wasn't halted, from the +yield+ call.

so one could legitimately expect around_perform { |, _block| result = block.call } that result would be whatever perform returned