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 :my_callback, :on => :create fires on update too #14493

Closed
basvk opened this issue Mar 27, 2014 · 58 comments
Closed

after_commit :my_callback, :on => :create fires on update too #14493

basvk opened this issue Mar 27, 2014 · 58 comments
Assignees
Labels

Comments

@basvk
Copy link
Contributor

@basvk basvk commented Mar 27, 2014

After upgrading from 4.0.0.beta1 to 4.1.0.rc2 my after_commit hooks are now getting triggered on update too, despite the :on => :create option.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 27, 2014

@basvk could you check with 4.0.4?

@rafaelfranca rafaelfranca added this to the 4.1.0 milestone Mar 27, 2014
@basvk
Copy link
Contributor Author

@basvk basvk commented Mar 27, 2014

Checked with Rails 4.0.4: I can confirm this happens in 4.0.4 as well.

I also realized that I'm actually updating the record inside the after_commit callback... (setting a status when the after_commit task was successful).

@rafaelfranca rafaelfranca removed this from the 4.1.0 milestone Mar 27, 2014
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 27, 2014

Could you create a gist reproducing the issue. You can found a template in the contributing guide.

@basvk
Copy link
Contributor Author

@basvk basvk commented Mar 27, 2014

@laurocaetano
Copy link
Contributor

@laurocaetano laurocaetano commented Mar 27, 2014

Hi @basvk, I think your gist is not doing what you have described in this issue. Could you check this one to see if it is correct?

@basvk
Copy link
Contributor Author

@basvk basvk commented Mar 27, 2014

@laurocaetano The issue apparently is caused by doing an update (at the db level) of the record inside the after_commit callback.
In my gist, I trigger that by means of the increment! call.
This was working fine in 4.0.0.beta1 and older, but stopped functioning after that.
(In real life, the business logic behind updating the record is much more complex than this particular example of course).

In your gist, you are not saving the updated attribute inside the after_commit, and the issue does not get triggered. Actually the updated attribute does not get persisted until the post.save line, so in real life the change to the attribute would actually be lost.

So as it is currently, it is not possible to update the record in an after_commit callback without getting into an infinite loop, despite the :on => :create...

@basvk
Copy link
Contributor Author

@basvk basvk commented Mar 27, 2014

BTW here is a real life example:

"After committing the new record to the db, a report file should be automatically created, and the url to the newly created report should be saved in that changed record."

@laurocaetano
Copy link
Contributor

@laurocaetano laurocaetano commented Mar 31, 2014

Hi @basvk. You are right.

See: 9d2146a. As you can see in the revert message, the fix introduced a bug and the behavior now is the same as 3.2.

It means this is a bug since 3.2. Also, I think this issue is similar to #8937, could you confirm?

@basvk
Copy link
Contributor Author

@basvk basvk commented Mar 31, 2014

#8937 sure looks related from the description, but I have no way to confirm for sure. I guess it is all connected to each other anyway.

I can only confirm that the issue described in this ticket did not occur in 4.0.0.beta1, I had it in production for a couple of months without problems.

@basvk
Copy link
Contributor Author

@basvk basvk commented Mar 31, 2014

The revert indeed fixes the issue for me, thanks.

@laurocaetano
Copy link
Contributor

@laurocaetano laurocaetano commented Apr 1, 2014

I think there is no easy way to make this work 😿

Correct me if I'm wrong, but when the after_commit callback is called the transaction is already finished. If you call save inside the callback, it will trigger another callback again and again and again.

The easy way to have this working, could be something like this:

class User
  after_commit :do_after_commit, on: :create

  def do_after_commit
    name = 'New name'
  end
end

user = User.create! # triggers the `after_commit` callback.

user.save # does the update here.
@abacha
Copy link

@abacha abacha commented Apr 8, 2014

+1

@basvk
Copy link
Contributor Author

@basvk basvk commented Apr 8, 2014

@laurocaetano When you use the :on => :create option on the after_commit callback, it should only run it once at creation of the record. Updating the record, even inside the callback, should not trigger that particular after_commit callback again, due to its :on => :create option.

Of course, if you omit the :on => :create, and you update the record inside the callback, you will indeed create an infinite loop as expected. This has always been the case.

So what you are proposing already exists (and functioned correctly until 4.0.0), but instead of :only => :create it is called :on => :create.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 8, 2014

@basvk a save inside a after_commit with on: :create is also inside the create lifecycle even if what it is doing is an update. The lifecycle only finish when the last after_commit callback runs, so we don't have a way to detect if that save is an update or not.

So @laurocaetano is correct, there is no way to make this work.

@basvk
Copy link
Contributor Author

@basvk basvk commented Apr 8, 2014

ok, so that's the flaw: after_commit implies that the commit is done and dealt with, but that apparently is not the case anymore.

@jherdman
Copy link

@jherdman jherdman commented Apr 8, 2014

I'd love to see some sort of solution to make this work. In the past I've done things like this to simulate the desired behaviour:

after_create :flag_that_this_is_a_create

after_commit :some_action_i_care_about

def flag_that_this_is_a_create
  @this_is_a_create = true
end

def some_action_i_care_about
  if @this_is_a_create
    some_side_effects
  end
end

Kind of ugly, and it feels like a hack.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 8, 2014

@basvk this is still true, the commit was already done, the create action still not since it only ends when the last callback end.

@jherdman unfortunately to make this work we have to change the callback lifecycle to put after_commit outside it, and this will change some expected behavior, so it can't be an automatic decision and need to be done very carefully.

I'm closing this issue now.

@laurocaetano
Copy link
Contributor

@laurocaetano laurocaetano commented Apr 8, 2014

So what you are proposing already exists (and functioned correctly until 4.0.0), but instead of :only => :create it is called :on => :create.

@basvk I'm sorry, it was a typo 😅

@laurocaetano When you use the :on => :create option on the after_commit callback, it should only run it once at creation of the record. Updating the record, even inside the callback, should not trigger that particular after_commit callback again, due to its :on => :create option.

It only run once at creation, the problem is the save inside the callback that will result in an infinite loop.

I'd love to see some sort of solution to make this work. In the past I've done things like this to simulate the desired behaviour:

@jherdman it will work the way I described, you don't need to use a variable to control the lifecycle.

@laurocaetano
Copy link
Contributor

@laurocaetano laurocaetano commented Apr 8, 2014

Related issues: #9588, #11118 and #8937

@basvk
Copy link
Contributor Author

@basvk basvk commented Apr 8, 2014

This is typical for Rails and makes dealing with it very painful. Mind you I'm using rails since 2005.

This functionality was working fine in earlier versions, I use it extensively in several projects. It breaks on a point release (it's always on a point release) and instead of it being fixed (well the revert fixed it, but I assume it won't stay that way for long), the ticket gets closed, which means that I will have to fix several projects, probably without being able to bill my clients for it, or stay with the previous version forever.

Sorry for the rant, and it's of course not a such a big deal, but it does feel like death by a 1000 paper cuts. I'm not using Rails anymore for new projects due to the pain with upgrades and the pain with deployments and setup. The high maintenance is killing.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 8, 2014

The revert changed the behavior back to the same behavior it had in Rails 3.2 so if something was broken is 4.0.0.beta1 (as you can see it is a beta release).

We still can fix it, but no promises.

@rafaelfranca rafaelfranca reopened this Apr 8, 2014
@basvk
Copy link
Contributor Author

@basvk basvk commented Apr 8, 2014

Thanks for reopening, I appreciate your open mind in this.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Apr 8, 2014

You are welcome, I should not have closed it. Sorry about that.

@rails-bot rails-bot closed this Aug 25, 2015
@rails rails unlocked this conversation Aug 25, 2015
@danielbonnell
Copy link

@danielbonnell danielbonnell commented Nov 9, 2015

Was this issue ever resolved? I'm on 4.0.2 and upgrading is not an option for me at the moment.

@siannopollo
Copy link

@siannopollo siannopollo commented Jan 30, 2016

This is still an issue as of rails 4.2.2. I can't say anything about newer releases, but I would imagine they still suffer from this as well.

@maclover7
Copy link
Member

@maclover7 maclover7 commented Jan 30, 2016

Ran reproduction script above, and Rails master is currently passing. Working on a bisect (and hopefully a solution) for 4-2-stable right now.

@maclover7 maclover7 removed the stale label Jan 30, 2016
@maclover7
Copy link
Member

@maclover7 maclover7 commented Jan 30, 2016

Ok, so it looks like this bug is not existent on master. Reproduction script I used can be found here. Another slight complication -- it appears as though this behavior has been in the 4.x.x series for a while, because git bisect is broken due to not being able to find a correct merge base to get me start. Not sure how to move forward here. cc @sgrif

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 30, 2016

Not sure how to move forward here.

We don't move forward. If it is fixed only on master that is fine.

@woniesong92
Copy link

@woniesong92 woniesong92 commented Feb 18, 2016

+1: Isn't it very common for someone to want to call save inside the after_commit callback?
I still have this problem with Rails 4.2.5.1

@dimoon
Copy link

@dimoon dimoon commented Feb 18, 2016

@woniesong92 Why not use update_columns instead of save: validations and callbacks are skipped. Works good inside after_commit on: :create in Rails 4.2.5.1

@pirj
Copy link

@pirj pirj commented Feb 20, 2016

@dimoon You mean that:

update_columns(changes.transform_values(&:last)) if changes.any?

This will work for most of the cases, but what about saving dependent models created in callback?

@eugeneius
Copy link
Member

@eugeneius eugeneius commented Mar 20, 2016

The script from #14493 (comment) passes on master, but only because it no longer reproduces the bug. increment! doesn't trigger callbacks any more since #11410 - if update! is used instead, it still fails.

#18367 is a proposed fix which does make the script pass, even using update!.

There's some confusion in the thread, so to clarify: this bug is present in every stable release of Rails since at least 3.2. It was fixed in 4.0.0.beta1, but the fix was reverted before 4.0.0.rc1.

@h0jeZvgoxFepBQ2C
Copy link

@h0jeZvgoxFepBQ2C h0jeZvgoxFepBQ2C commented Mar 21, 2016

So maybe we should open this issue again?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 21, 2016

No, we shouldn't #14493 (comment)

@h0jeZvgoxFepBQ2C
Copy link

@h0jeZvgoxFepBQ2C h0jeZvgoxFepBQ2C commented Mar 21, 2016

@rafaelfranca But then I would suggest to remove the on: :update/:create option entirely? If it is broken and won't be repaired? From my point of view this is not a good solution to just let it there in a broken state? What are your thoughts about removing the options?

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 21, 2016

It works for the use case it exists. It only don't work when you do a update action inside a callback that is running because an create action started. In this case Rails consider all actions create.

@hegwin
Copy link

@hegwin hegwin commented Apr 29, 2017

I encounter this infinite loop issue when using after_create_commit callback in Rails 5.0.0.

Currently I use update_columns instead of update_attributes to avoid this problem. So will this be fixed?

@zakwanhaj
Copy link

@zakwanhaj zakwanhaj commented Aug 22, 2017

I did the following hack and It works with me:

  after_commit :do_after_commit, on: :create

  def do_after_commit
    self.class.find(id).update(name: 'New name')
    reload
  end
@SampsonCrowley
Copy link
Contributor

@SampsonCrowley SampsonCrowley commented Nov 15, 2017

I just want to point out this is still an issue with rails 5. zakwanhaj's work around works for me but this really should be fixed as it can be quite dangerous for those that find the bug the hard way.

@SampsonCrowley
Copy link
Contributor

@SampsonCrowley SampsonCrowley commented Nov 15, 2017

and using update_columns to skip callbacks isn't a proper solution. a lot of users depend on those call backs for things like auditing

@syedmahmad
Copy link

@syedmahmad syedmahmad commented Feb 1, 2018

For rails version <= 4.1 use
after_commit :do_after_commit, :only => :create
For rails version > 4.1 use
after_commit :do_after_commit, :on => :create.

This solve my problem.

@SampsonCrowley
Copy link
Contributor

@SampsonCrowley SampsonCrowley commented Feb 1, 2018

@syedmahmad the syntax for how to limit to on: :create is not the issue, and using the proper syntax for your rails version does NOT fix the save after commit issue

@SampsonCrowley
Copy link
Contributor

@SampsonCrowley SampsonCrowley commented Feb 1, 2018

calling anything in after_commit should be part of a new transaction, not the transaction before committing. that's why it's call AFTER commit

@h0jeZvgoxFepBQ2C
Copy link

@h0jeZvgoxFepBQ2C h0jeZvgoxFepBQ2C commented Feb 20, 2018

I'm stumbling again and again over this issue every year, it's really a big pain and it's sad that this doesn't get fixed - this is clearly a bug in my opinion. Having this issue now with latest Rails 5 version.

@SampsonCrowley
Copy link
Contributor

@SampsonCrowley SampsonCrowley commented Feb 20, 2018

The problem seems to boil down to the object not being loaded properly in after_commit, i.e. the following function:

if self.group_id.blank? && group_id.blank?
  self.class.find(self.id).update(group_id: relations.where.not(group_id: nil).pluck(:group_id).first || get_athlete_or_coach.user_id)
  reload
end

I would expect the above after_commit (regardless of if it was set to create or allowed to run after any commit) to only run once since the group_id should be set after the first time this code runs.

However, if I remove reload from inside the if statement, the code runs infinitely. For some reason the loaded instance doesn't get the newly updated attribute set between the time that the row is updated and the time after_commit gets called. Even using self.class.find doesn't keep me from needing to add reload since self.class.find seems to return the cached version

@augustosamame
Copy link

@augustosamame augustosamame commented Mar 12, 2018

I keep being bitten by this again and again. Each time I eventually reach this thread and I remember WHY this happens, but unfortunately the counterintuitive nature of the after_commit :on => :create is problematic to say the least.
I would suggest AT LEAST firing a warning in logger when a save is fired inside the callback. In this way it will be obvious what's happening and we can a) change our logic or b) use the update_columns workaround.
IMO the somewhat obscure, counterintuitive behaviour in a common use case is the main problem.

@atitan
Copy link

@atitan atitan commented Jun 11, 2018

Having the same problem with Rails 5.2.0 too.
The solution I found is to use this gem in conjunction with after_create.

It looks like this, and everything works fine.

class A < ApplicationRecord
  include AfterCommitEverywhere

  after_create :do_something

  def do_something
    after_commit do
      self.update(....)
    end
  end
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.