Current transaction records retained by transactional fixtures (master) #3300

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
7 participants
@willbryant
Contributor

willbryant commented Oct 12, 2011

As discussed on the mailing list, this fixes the memory bloat when using transactional fixtures to run large test suites, which is basically a regression since 2.3 due to the side effects of the better record rollback code. This substantially improves test runtime too if you save a lot of objects in tests.

This branch also exposes the option to not retain references to the records saved in transactions, which is useful for applications that have large transactions that save a lot of objects but don't need their state in memory to be rolled back if the transaction fails nor need the transaction callbacks.

willbryant added some commits Oct 11, 2011

when the transactional fixtures keeps a database transaction alive wi…
…thout using the transaction method, don't retain references to all the records saved in the fixtures transaction once their subtransaction completes
expose :remember_record_state => false option on transaction(), so ap…
…plications that don't need state rollback (or after_commit/rollback hooks) can garbage collect the records during long transactions
@seanwalbran

This comment has been minimized.

Show comment
Hide comment
@seanwalbran

seanwalbran Oct 17, 2011

Contributor

Just fought with this leak myself, happy to see this in play. The approach I took was to add the below to the teardown_fixtures method in fixtures.rb, which seems consistent with the original callbacks commit ( b070739 ) and should allow for the actual rollback callbacks to take effect in tests (for, e.g., cleanup of external references if any, etc.). Any thoughts on this spin instead/as well?

def teardown_fixtures
...
# Rollback changes if a transaction is active.
if run_in_transaction? && ActiveRecord::Base.connection.open_transactions != 0
ActiveRecord::Base.connection.rollback_db_transaction

  •  ActiveRecord::Base.connection.send(:rollback_transaction_records,true) if ActiveRecord::Base.connection.instance_variable_get('@_current_transaction_records')
    
    ActiveRecord::Base.connection.decrement_open_transactions
    end
    ActiveRecord::Base.clear_active_connections!
    end
Contributor

seanwalbran commented Oct 17, 2011

Just fought with this leak myself, happy to see this in play. The approach I took was to add the below to the teardown_fixtures method in fixtures.rb, which seems consistent with the original callbacks commit ( b070739 ) and should allow for the actual rollback callbacks to take effect in tests (for, e.g., cleanup of external references if any, etc.). Any thoughts on this spin instead/as well?

def teardown_fixtures
...
# Rollback changes if a transaction is active.
if run_in_transaction? && ActiveRecord::Base.connection.open_transactions != 0
ActiveRecord::Base.connection.rollback_db_transaction

  •  ActiveRecord::Base.connection.send(:rollback_transaction_records,true) if ActiveRecord::Base.connection.instance_variable_get('@_current_transaction_records')
    
    ActiveRecord::Base.connection.decrement_open_transactions
    end
    ActiveRecord::Base.clear_active_connections!
    end
@willbryant

This comment has been minimized.

Show comment
Hide comment
@willbryant

willbryant Feb 18, 2012

Contributor

Sean, firing rollback event handlers seems reasonable to me as an additional thing. Haven't been able to produce much interest in that when I asked about it on the rails-core mailing list (see posts around 12 October) though.

Contributor

willbryant commented Feb 18, 2012

Sean, firing rollback event handlers seems reasonable to me as an additional thing. Haven't been able to produce much interest in that when I asked about it on the rails-core mailing list (see posts around 12 October) though.

@isaacsanders

This comment has been minimized.

Show comment
Hide comment
@isaacsanders

isaacsanders May 5, 2012

Contributor

Is this still an issue?

Contributor

isaacsanders commented May 5, 2012

Is this still an issue?

@willbryant

This comment has been minimized.

Show comment
Hide comment
@willbryant

willbryant May 5, 2012

Contributor

Yes.

Contributor

willbryant commented May 5, 2012

Yes.

@isaacsanders

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 14, 2012

Member

This is gonna need a rebase if it's ever gonna get included.

Member

steveklabnik commented Sep 14, 2012

This is gonna need a rebase if it's ever gonna get included.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Nov 17, 2012

Member

@willbryant ping! are you still interested in keeping up with this pull request?

Member

steveklabnik commented Nov 17, 2012

@willbryant ping! are you still interested in keeping up with this pull request?

+ remember_record_state = options[:remember_record_state]
+ else
+ remember_record_state = true
+ end

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Nov 17, 2012

Member

This can be simplified with options.fetch

@carlosantoniodasilva

carlosantoniodasilva Nov 17, 2012

Member

This can be simplified with options.fetch

@willbryant

This comment has been minimized.

Show comment
Hide comment
@willbryant

willbryant Dec 2, 2012

Contributor

@steveklabnik: Yes I am, we use it in production and it's pretty critical for us.

Both commits apply cleanly to 3-0-stable, 3-1-stable, 3-2-stable; I've updated current_transaction_records_3-2-stable in my fork.

For master, first see @jonleighton's comments on b89ffe7 - note that weakrefs are completely broken on 1.9.

@jonleighton, could you please give me some feedback about what your intention is on master? There's no issue tracker numbers on the conflicting commit that have refactored the transaction internals, so I'm just not sure if you plan to achieve this by other means, or if not, whether you'd consider merging this feature if I update it to match the refactoring master.

Contributor

willbryant commented Dec 2, 2012

@steveklabnik: Yes I am, we use it in production and it's pretty critical for us.

Both commits apply cleanly to 3-0-stable, 3-1-stable, 3-2-stable; I've updated current_transaction_records_3-2-stable in my fork.

For master, first see @jonleighton's comments on b89ffe7 - note that weakrefs are completely broken on 1.9.

@jonleighton, could you please give me some feedback about what your intention is on master? There's no issue tracker numbers on the conflicting commit that have refactored the transaction internals, so I'm just not sure if you plan to achieve this by other means, or if not, whether you'd consider merging this feature if I update it to match the refactoring master.

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Dec 10, 2012

Member

@willbryant:

I discussed more with @tenderlove after that commit. He intends that each record will hold its transaction in an ivar, rather than the other way round. Then, calling e.g. record.new_record? will query the transaction state. It will still be necessary to hold a list of records in the transaction for records with commit/rollback hooks, that's unavoidable. But for the usual case it should work.

Are you up for implementing that?

Member

jonleighton commented Dec 10, 2012

@willbryant:

I discussed more with @tenderlove after that commit. He intends that each record will hold its transaction in an ivar, rather than the other way round. Then, calling e.g. record.new_record? will query the transaction state. It will still be necessary to hold a list of records in the transaction for records with commit/rollback hooks, that's unavoidable. But for the usual case it should work.

Are you up for implementing that?

@willbryant

This comment has been minimized.

Show comment
Hide comment
@willbryant

willbryant Dec 10, 2012

Contributor

@jonleighton: that's an interesting approach. That would mean also that #id, for example, and any other things currently reset by the transaction rollback code would need to query the transaction state?

Contributor

willbryant commented Dec 10, 2012

@jonleighton: that's an interesting approach. That would mean also that #id, for example, and any other things currently reset by the transaction rollback code would need to query the transaction state?

@jonleighton

This comment has been minimized.

Show comment
Hide comment
@jonleighton

jonleighton Dec 11, 2012

Member

Unsure, I haven't really looked into it..

Member

jonleighton commented Dec 11, 2012

Unsure, I haven't really looked into it..

@willbryant

This comment has been minimized.

Show comment
Hide comment
@willbryant

willbryant Dec 11, 2012

Contributor

Looking at the remember_transaction_record_state code, here's the list of things that we'd need to give the special behavior to:

  • id
  • new_record
  • destroyed
  • frozen?
    Not sure about level (transaction nesting level), that would depend on how the reference to the transaction is maintained.

id is the only one that's an attribute and that could make it a little messy to get the special semantics. The others wouldn't be too hard.

Contributor

willbryant commented Dec 11, 2012

Looking at the remember_transaction_record_state code, here's the list of things that we'd need to give the special behavior to:

  • id
  • new_record
  • destroyed
  • frozen?
    Not sure about level (transaction nesting level), that would depend on how the reference to the transaction is maintained.

id is the only one that's an attribute and that could make it a little messy to get the special semantics. The others wouldn't be too hard.

@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Mar 15, 2013

Member

Guys, what's the status?

/cc @jonleighton @willbryant

Member

lukaszx0 commented Mar 15, 2013

Guys, what's the status?

/cc @jonleighton @willbryant

@willbryant

This comment has been minimized.

Show comment
Hide comment
@willbryant

willbryant Mar 15, 2013

Contributor

I think the above alternate solution got committed to master for 4.0. I haven't tested it but I'll close this in the meantime.

Contributor

willbryant commented Mar 15, 2013

I think the above alternate solution got committed to master for 4.0. I haven't tested it but I'll close this in the meantime.

@willbryant willbryant closed this Mar 15, 2013

@lukaszx0

This comment has been minimized.

Show comment
Hide comment
@lukaszx0

lukaszx0 Mar 16, 2013

Member

Thanks @willbryant for an update and taking action 👍

Member

lukaszx0 commented Mar 16, 2013

Thanks @willbryant for an update and taking action 👍

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