Transaction not rolling back object state when using .save instead of .save! #4566

Closed
rorcraft opened this Issue Jan 20, 2012 · 19 comments

Projects

None yet
@rorcraft
class MyModel < AR
 after_create do
   raise ActiveRecord::StatementInvalid.new("Dead locked")
 end
end

@model = MyModel.new
MyModel.transaction do
  @model.save
end
@model.new_record? == false

Shouldn't @model.new_record? still be true.

If I use a save!, it does roll back properly because save! doesn't wrap another level of transaction.

The issue is related to this line, where it only rolls back if it is in outermost transaction.
https://github.com/rails/rails/blob/3-2-stable/activerecord/lib/active_record/transactions.rb#L328

Is this expected behaviour, if so, what are the reasons for not rolling back .save methods.

@mdimas
mdimas commented Jan 25, 2012

This doesn't seem like expected behavior, and can cause a bug where you think you saved a model that doesn't actually get saved.

In the example above let's say the after_create error was only intermittent (happens the first time but not the second), so you try saving again.

begin
  MyModel.transaction do
    @model.save
  end
rescue
  # try again
  @model.save
end

In this case the second call to save will return true, but it actually issued a SQL UPDATE command and not an INSERT, so your record never gets saved.

@mdimas
mdimas commented Feb 4, 2012

It looks like this is because of the following chain of events when calling .save

  1. #save starts with a rollback_active_record_state! block, which calls remember_transaction_record_state

  2. This sets the @_start_transaction_state[:level] to 1

  3. with_transaction_returning_status is called, which calls add_to_transaction, which calls remember_transaction_record_state

  4. This sets the @_start_transaction_state[:level] to 2

  5. An exception is raised causing restore_transaction_record_state to be called

  6. This reduces the @_start_transaction_state[:level] to 1

  7. "if @_start_transaction_state[:level] < 1" is used to determine whether the state will actually be restored, so this is false

  8. The object state is never restored, so when .save is called again it no longer thinks it is a new record and issues an "UPDATE" instead of an "INSERT"

@cap10morgan
Contributor

Looks like this is fixed in master (as of Feb. 25, 2012). The following test passes:

def test_rollback_in_save_resets_new_record_state
  add_exception_raising_after_create_callback_to_topic

  topic = Topic.new
  begin
    topic.approved = true
    topic.save
  rescue
    assert topic.new_record?
  ensure
    remove_exception_raising_after_create_callback_to_topic
  end
end
@mdimas
mdimas commented Mar 1, 2012

Actually rorcraft's example still fails with rails master. I also walked through transactions.rb on master and the exact same code flow and problem as I outlined in my comment above still exists.

I don't understand why this isn't a bigger issue and how it isn't causing problems for lots of other people.

@cap10morgan
Contributor

Ah, the difference might be the "ModelClass.transaction do" part. I'll see if I can add a test that does that and hopefully that will expose the bug. Thanks for the update!

@isaacsanders
Contributor

Is this still an issue?

@steveklabnik
Member

It's been 7 months with no feedback. If you can provide us with a reproduction against master, I'll re-open. Thanks.

@nate
nate commented Dec 5, 2012

Hey guys! This problem still seems to exist! Here's a test case that works on master:

# put in active_record/test/schema/schema.rb
create_table :bad_mofos, :force => true do |t|
  t.string :name
  t.string :badness
end

# put in active_record/test/models/bad_mofo.rb
class BadMofo < ActiveRecord::Base
  after_create do
    raise ActiveRecord::StatementInvalid.new("MOFOS TOO BAD FOR DIS DB TO HANDLE")
  end
end

# put in active_record/test/cases/transactions_test.rb
def test_if_these_bad_mofos_break_transaction_rollbackness
  bad_mofo = BadMofo.new
  begin
    BadMofo.transaction do
      bad_mofo.save
    end
  rescue ActiveRecord::StatementInvalid
  end

  assert !bad_mofo.new_record?
end
# run with
cd active_record
bundle exec ruby -Itest test/cases/transactions_test.rb -n test_if_these_bad_mofos_break_transaction_rollbackness

Like @mdimas said, it seems to be that ActiveRecord::Transaction#restore_transaction_record_state is never called such that ((@_start_transaction_state[:level] || 0) - 1) is true or force is true.

It seems to me like restore_transaction_record_state should be called after each nested transaction is exited. Also, it seems like this might leak memory, especially when running test suites (maybe related to #3300?). And it seems like this isn't the only problem with this code (#7807).

Thanks for having another look!

@steveklabnik steveklabnik reopened this Dec 5, 2012
@rycfung
rycfung commented Jan 20, 2013

Is any one fixing this issue?

@vicli
vicli commented Mar 6, 2013

i can't seem to reproduce error, has it been fixed on master?

@nate
nate commented Mar 6, 2013

My test case no longer shows the issue on master, but I'm not sure if that means it's actually fixed.

@vicli
vicli commented Mar 6, 2013

@nate , in your test case, bad_mofo.new_record? should return true right? (based on above discussion). if so, you seem to be asserting that it is false

@nate
nate commented Mar 6, 2013

@vicli ah, right, so it should. in that case, this problem still seems to exist

@vicli
vicli commented Mar 6, 2013

hahaha good, i wasn't sure if i was misinterpreting the code. I'll look into this. thanks for the help!

@nate
nate commented Mar 18, 2013

Just to clarify since I received an email for a comment that's been deleted, the test I provided passes when it shouldn't. I should have provided a failing test case. Fixing the test at this point would just muddy the issue more, however, so I'll leave it be.

@neerajdotname
Member

This is fixed in master. Here is the test case.

class User < ActiveRecord::Base

  after_create do
   raise ActiveRecord::StatementInvalid.new("booom")
  end

  def self.lab
    @user = User.new

    begin
      User.transaction do
        @user.save
      end
    rescue ActiveRecord::StatementInvalid
    end

    puts @user.new_record? == false
  end

end

@pftg
Contributor
pftg commented Jul 14, 2013

in #9828 this test case is broken, still issue in the master

@pftg
Contributor
pftg commented Jul 14, 2013

I found the problem, adding fix.

@pftg pftg added a commit to jetthoughts/rails that referenced this issue Jul 14, 2013
@pftg pftg #4566: Remove extra decrement of transaction level
`rollback_active_record_state!` tries to restore model state on `Exception`
by invoking `restore_transaction_record_state` it decrement deep level by `1`.

After restoring it ensure that states to be cleared
and level decremented by invoking `clear_transaction_record_state`,
which cause the bug: because state already reduced in `restore_transaction_record_state`.

Removed double derement of transaction level
and removed duplicated code which clear transaction state for top level.
e0d59e6
@arunagw
Member
arunagw commented Jul 15, 2013

Closing this as #11434 is merged.

Please test this out.

thanks guys.

@arunagw arunagw closed this Jul 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment