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

ActiveRecord transaction mixes objects up after rollback #20994

Closed
hisenb3rg opened this issue Jul 23, 2015 · 2 comments
Closed

ActiveRecord transaction mixes objects up after rollback #20994

hisenb3rg opened this issue Jul 23, 2015 · 2 comments

Comments

@hisenb3rg
Copy link

Transaction rollback doesn't work correctly for duped objects. Somehow the start state of object is mixed with the original. Consequently the rollback leaves us with faulty objects which can lead to all sorts of errors...

This happens in Rails 4.1.12, 4.0.13, but it doesn't happen in 4.2.3.

Below is an example demoing the issue. Key thing is that we dupe an object inside a transaction, save duped object, and the original too. And then we roll back.

#Activate the gem you are reporting the issue against.                           
gem 'activerecord', '4.1.12'                                                     
require 'active_record'                                                          
require 'logger'                                                                 

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

# Create table for Person model                                                  
ActiveRecord::Schema.define do                                                   
  create_table :people, force: true  do |t|                                      
    t.string :name                                                               
  end                                                                            
end                                                                              

class Person < ActiveRecord::Base                                                
  before_create { true }                                                         
end                                                                              


# script to demo transaction rollback issue                                      
original = Person.create {|p| p.name = 'test' }                                  
copy = nil                                                                       

Person.transaction do                                                            
  copy = original.dup                                                            
  copy.save!                                                                     
  original.save!                                                                 

  puts original.inspect                                                          
  puts original.hash                                                             
  puts copy.inspect                                                              
  puts copy.hash                                                                 

  raise ActiveRecord::Rollback                                                   
end                                                                              

puts original.inspect                                                            
puts original.hash                                                               
puts copy.inspect                                                                
puts copy.hash

The output:

-- create_table(:people, {:force=>true})
D, [2015-07-23T10:53:15.420264 #59878] DEBUG -- :    (0.2ms)  CREATE TABLE "people" ("id" INTEGER PRIMARY KEY AUTOINCREMENT NOT NULL, "name" varchar(255)) 
   -> 0.0049s
D, [2015-07-23T10:53:15.432679 #59878] DEBUG -- :    (0.0ms)  begin transaction
D, [2015-07-23T10:53:15.434723 #59878] DEBUG -- :   SQL (0.1ms)  INSERT INTO "people" ("name") VALUES (?)  [["name", "test"]]
D, [2015-07-23T10:53:15.434975 #59878] DEBUG -- :    (0.0ms)  commit transaction
D, [2015-07-23T10:53:15.435146 #59878] DEBUG -- :    (0.0ms)  begin transaction
D, [2015-07-23T10:53:15.435563 #59878] DEBUG -- :   SQL (0.0ms)  INSERT INTO "people" ("name") VALUES (?)  [["name", "test"]]
#<Person id: 1, name: "test">
2842541575130211311
#<Person id: 2, name: "test">
-1273712618394956023
D, [2015-07-23T10:53:15.435954 #59878] DEBUG -- :    (0.0ms)  rollback transaction
#<Person id: 1, name: "test">
2842541575130211311
#<Person id: 1, name: "test">
2842541575130211311

As visible from the output, the copy and original vars are referencing the same object after rollback!

Note: There are few conditions for this bug to appear. First, the model needs to have at least one before_create callback. Second, we need to call #save on the original object inside a transaction (original.save! - which does nothing to the db because object is not dirty). If we remove any of these two conditions, the bug does not appear.

@arthurnn
Copy link
Member

On 4.2, we did some big refactoring on the code of transaction, to fix mainly a bug around callback exceptions been swallowed. With the refactor I assumed that I fixed other problems too. I guess this was one of the issues solved. As the refactor was big, we could not backport it to Rails 4.1 or 4.0 .
So if this is working on 4.2 and master I would say this is not a bug.

thanks for reporting anyways.

@chancancode
Copy link
Member

Technically, the fact that we did a big refactor that happens to fix the bug does not imply the bug can't be fixed with a more targeted patch.

However, I currently can't volunteer to take on this myself, and to be quite honest, with 5.0 due soon, 4.1 is very low on everyone's priority list (4.1 will be EOL'ed when 5.0 comes out, and we already don't ship bug fixes for 4.0 anymore) - it's an unfortunate reality since we don't have unlimited resources so we have to pick our battles.

If you ended up figuring out how to fix this in a targeted way, and you can PR it before 5.0 ships, I think we will be more than happy to take the patch!

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

No branches or pull requests

3 participants