Skip to content

(R4R) Fix infinite looping #2

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.

Sign up for GitHub

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

Open
marlonnelson wants to merge 1 commit into
base: add-transaction-callbacks
Choose a base branch
from
Open

(R4R) Fix infinite looping #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions lib/activerecord/delay_touching.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,18 +86,23 @@ def self.touch_records(attr, klass, records)
column = column.to_s
changes[column] = current_time
records.each do |record|
next if record.destroyed?
next unless record.persisted? # A new record may have been rolled back

@pcstout pcstout Aug 17, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only fix new records, how about old records? Is there a way to check if the record was rolled back so we can NOT do the touching?

@marlonnelson marlonnelson Aug 17, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was one of the deficiencies I wanted to discuss. I don't know how to identify those records. @afiedler, do you?

@pcstout pcstout Aug 17, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be a nice-to-have change, definitely not needed for our immediate hotfix needs.

@pcstout pcstout Sep 1, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...rolled back or destroyed

record.instance_eval do
write_attribute column, current_time
@changed_attributes.except!(*changes.keys)
end
end
end

klass.unscoped.where(klass.primary_key => records).update_all(changes)
updatable_records = records.select{|r| r.persisted?} # No need to touch unpersisted records

if updatable_records.present?
klass.unscoped.where(klass.primary_key => updatable_records).update_all(changes)
end
end
state.updated attr, records
records.each do |record|
next unless record.persisted? # A new record may have been rolled back

@pcstout pcstout Sep 1, 2016 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this now prevent a destroyed object from running touch and commit callbacks?

@marlonnelson marlonnelson Sep 2, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pcstout - Yes. I'm going to rework this fix and reduce it down to just avoiding the infinite loop.

record.run_callbacks(:touch)
if klass.connection.open_transactions > 0
klass.connection.add_transaction_record record
Expand Down
7 changes: 7 additions & 0 deletions lib/activerecord/delay_touching/state.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,13 @@ def initialize
end

def updated(attr, records)
# Records may have been changed since they were added to the set. For instance, if an error
# occurred and a Rollback was generated, Rails might change the record's id from an integer
# to a nil (if it's a new record). Since it was stored in the set originally, using the
# hash of the id of the record (thanks to Rails' magic), it won't be removed because now the
# hash is different and it isn't found in the set.
@records[attr] = Set.new(@records[attr]) # recreate the Set so it's reliable

@pcstout pcstout Aug 17, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this break anything? Would it be better to handle the bad records while subtracting them?

@marlonnelson marlonnelson Aug 17, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say it doesn't break anything. I can't think of anything it would break. I haven't seen any evidence that it breaks anything. The difficulty in handling the bad records while subtracting them is that the code is in ruby's Set class. We could monkey patch it (blech), or rewrite the gem to use something else, but then we'd have to replace the uniqueness quality with something.

The bottom line is, once the object in the Set has been changed, the set becomes unreliable (this is mentioned in the documentation for Set). That unreliable-ness makes it difficult to fix the set. It's possible I could do something like:

@records[attr].delete_if {|e| e.id.nil?}

Maybe that would perform better.

Do you have a specific technique in mind?

@marlonnelson marlonnelson Aug 17, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if @records[attr] contained a mix of persisted and unpersisted objects?


@records[attr].subtract records
@records.delete attr if @records[attr].empty?
@already_updated_records[attr] += records
Expand Down