Navigation Menu

Skip to content
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

after_commit uses incorrect final object when changing same object twice while reloading it from the DB #27342

Closed
marktermaat opened this issue Dec 13, 2016 · 6 comments

Comments

@marktermaat
Copy link

Steps to reproduce

Say you have a simple model User with a name:string.
Additionally, say this model looks like the following:

class User < ActiveRecord::Base
  after_commit :print_user

  def print_user
    puts "Current user: #{self.inspect}"
  end
end

Executing the following code in a console:

user = User.find(1)
User.transaction do
  user.name = 'new name 1'
  user.save

  user.name = 'new name 2'
  user.save
end

Will print the following user:
Current user: #<User id: 1, name: "new name 2">

However, executing the following code:

User.transaction do
  user = User.find(1)
  user.name = 'new name 1'
  user.save

  user = User.find(1)
  user.name = 'new name 2'
  user.save
end

results in the following user:
Current user: #<User id: 1, username: "admin", name: "new name 1">

Expected behavior

I would have expected to see the after_commit method called with the final state of the object (so in this case: 'new name 2')

Actual behavior

Instead, when reloading the object from the database it will send an intermediate version of the object to the after_commit method (I suspect the state of the object after the first time it's saved).

System configuration

Rails version: 4.2.7.1

Ruby version: 2.2.2

@eugeneius
Copy link
Member

Each transaction maintains a list of records that were saved while it was open. Duplicates are removed before running callbacks, to avoid running the same after_commit callback multiple times.

We could fix the problem reported here by running the callbacks on the most recently saved copy of each record instead of the first one.

However, what should happen in this case?

User.transaction do
  user1 = User.find(1)
  user2 = User.find(1)

  user1.name = 'new name'
  user1.save

  user2.email = 'new email'
  user2.save
end

Both records now have partially out of date information. Depending on what the callback does, using either record could give the wrong result.

I'm not sure if changing this behaviour - with a deprecation cycle, and potentially breaking people's apps - is worth it if the fix works for a simple example but not a complex one.

@serggl
Copy link

serggl commented Mar 23, 2017

@eugeneius faced exactly the same issue in our Rails 5 application yesterday. For your question I would answer that in this case we need to do a reload before invoking after_commit

@rails-bot rails-bot bot added the stale label Jun 21, 2017
@rails-bot
Copy link

rails-bot bot commented Jun 21, 2017

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@jcarlson
Copy link
Contributor

jcarlson commented Apr 3, 2019

I am still able to reproduce this with rails 5.2.2.1. I agree with @serggl: wouldn't it make sense for Rails to internally call reload on a record before invoking the after_commit hook?

@dylanahsmith
Copy link
Contributor

I think this was fixed in rails 6.0 by #36190

@eugeneius
Copy link
Member

#36190 ensures that duplicate records have their state updated correctly (e.g. attribute changes are rolled back if the transaction is rolled back), but after_commit callbacks are still deduplicated.

See #39400 for a newer, still open report of this same problem.

Automatically calling reload won't fly, as it would throw away any unsaved changes on the record, which the user might not expect. We could load a new copy of the record to use instead, but I still don't think that's the right solution: another transaction could have updated the record in between, and the reload would pick up changes unrelated to the transaction the callback is about.

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

No branches or pull requests

6 participants