rails: after_commit order inconsistent with other callbacks

It seems that the commit callbacks (defined in https://github.com/rails/rails/blob/master/activerecord/lib/active_record/transactions.rb#L230) is defined differently from the rest of the callbacks (defined in https://github.com/rails/rails/blob/master/activemodel/lib/active_model/callbacks.rb#L103).

I experienced this difference in the order after_commit is executed. It seems that after_* callbacks are executed in reverse order, and after_commit doesn’t handle this:

class M < ActiveRecord::Base
  before_create :a
  before_create :b
  after_create :c
  after_create :d
  after_commit :e
  after_commit :f

  def a; puts 'a'; end
  def b; puts 'b'; end
  def c; puts 'c'; end
  def d; puts 'd'; end
  def e; puts 'e'; end
  def f; puts 'f'; end
end

M._create_callbacks.select { |c| c.kind == :before }.map(&:filter)
=> [:a, :b]
M._create_callbacks.select { |c| c.kind == :after }.map(&:filter)
=> [:d, :c] # Weird behavior in after_* callbacks
M._commit_callbacks.select { |c| c.kind == :after }.map(&:filter)
=> [:e, :f]

M.create
=> a
=> b
=> c
=> d
=> f # The result of after_commit not handling the weird behavior
=> e

after_* callbacks add a default (actually, they override) prepend: true to the options to resolve this (see here https://github.com/rails/rails/blob/master/activemodel/lib/active_model/callbacks.rb#L139), while after_commit doesn’t, thus the inconsistency.

The culprit seems to be the reverse execution in after_* callbacks and not really after_commit, I would say the code in after_* is just a patch to solve this. This patch actually prevents using the :prepend option in the after_* callbacks.

I would also change the definition of the transactions callbacks to use the same logic of the rest of the callbacks to prevent inconsistencies in the future.

About this issue

Original URL
State: closed
Created 9 years ago
Reactions: 11
Comments: 22 (9 by maintainers)

Commits related to this issue

Adjust TakePartPage class ordering for callback ordering Carrierwave now performs the storing of an image in an after_commit callback rather than after_save. Due to a percularity in Rails callback or... — committed to alphagov/whitehall by kevindew 4 years ago
Change ordering of callbacks in take part page model In carrierwave 2.x, the storing of images to their remote location has been moved from after_save to after_commit (but only on create and update, ... — committed to alphagov/whitehall by brucebolt 4 years ago
Change ordering of callbacks in take part page model In carrierwave 2.x, the storing of images to their remote location has been moved from after_save to after_commit (but only on create and update, ... — committed to alphagov/whitehall by brucebolt 4 years ago

Most upvoted comments

@djiang @odedniv @jeremy @maclover7 @synth Still getting reverse callback order. Affected 4.2.8, 5.0.1, 5.0.2, master not checked.

Bug test:

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  # Activate the gem you are reporting the issue against.
  gem "activerecord", github: "rails/rails", branch: "master"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# Ensure backward compatibility with Minitest 4
Minitest::Test = MiniTest::Unit::TestCase unless defined?(Minitest::Test)

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end
end

class Post < ActiveRecord::Base
  has_many :comments
  
  after_commit :foo_stuff, :bar_stuff
  
  def invoked_callbacks
    @invoked_callbacks ||= []
  end
  
  def foo_stuff
    invoked_callbacks << :foo
  end
  
  def bar_stuff
    invoked_callbacks << :bar
  end    
end

class BugTest < Minitest::Test
  def test_after_commit_callbacks_order
    post = Post.create!
    assert_equal %i( foo bar ), post.invoked_callbacks
  end
end

Output:

  1) Failure:
BugTest#test_after_commit_callbacks_order [after_commit_test.rb:52]:
Expected: [:foo, :bar]
  Actual: [:bar, :foo]

+11

yivo on Mar 15, 2017

Lol, after 6 years and 4 major versions of Rails of this bug, should we call this a #wontfix? 😁 While this is likely “easy” to fix, I imagine fixing it may break a lot of existing apps in non-obvious ways.

synth on Apr 25, 2022

Reopening because #23462 went stale.

georgeclaghorn on May 17, 2020

^ this is what https://github.com/rails/rails/pull/46992 does

ghiculescu on Apr 5, 2023

I kind of agree with @synth. We have 7 years of Rails apps that depend on the current implementation. I feel bad that older apps trying to upgrade hit this, but I’d guess if we “fix” one we’ll probably break another. It might be better if apps that are upgrading adopt the current behavior, and we just make sure not to change that behavior. wdyt @rafaelfranca?

tenderlove on Apr 4, 2023

None of my issues here ever got solved/pulled/acknowledged @djiang… Anyway that’s what I did (with a comment pointing here)

odedniv on Sep 30, 2016

^ this is what #46992 does

Perfect. Merged! Thank you!

tenderlove on Apr 7, 2023

I think we should still change the default for new application even though we never force existing application to change. If existing applications want to change, we can provide helpers that would list the callback order to users so they can do the migration.

rafaelfranca on Apr 5, 2023

This is still impacting Rails 7.0.2.3.

tbcooney on Apr 25, 2022

Still impacting 6.1.4.1

d-pfeif on Jan 25, 2022

@synth FWIW, this seems to be impacting us on rails 5.2.2.1

dbelling on May 4, 2020

FYI, I ran this against 5.1.7 and the test fails (ie wrong callback order). We are going to be upgrading to 5.2 and ultimately 6.x soon so I’ll post the test results against those versions as I do them.

synth on Mar 29, 2020