Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Fixed a bug #2

Open
wants to merge 1 commit into from

1 participant

Colin Curtin
Colin Curtin

Hey, I found an infinite loop bug that I've fixed and provided tests for. Please let me know if you have any questions.

Colin Curtin perplexes Fix infinite loop bug.
Cause: an on_commit callback creates a transaction, which afterwards loops through all the on_commit records waiting to be called, which includes the one that creates a transaction.

Fix: Have a commit object stack that gets pushed and popped for each commit object, so that it doesn't loop, and (untested) its inner-transaction's on_commit can run.
e687184
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Nov 06, 2010
Colin Curtin perplexes Fix infinite loop bug.
Cause: an on_commit callback creates a transaction, which afterwards loops through all the on_commit records waiting to be called, which includes the one that creates a transaction.

Fix: Have a commit object stack that gets pushed and popped for each commit object, so that it doesn't loop, and (untested) its inner-transaction's on_commit can run.
e687184
This page is out of date. Refresh to see the latest.
75 lib/after_commit.rb
... ... @@ -1,33 +1,92 @@
  1 +require 'rubygems'
  2 +require 'ruby-debug'
  3 +Debugger.start
1 4 module AfterCommit
  5 + @@records = {}
  6 + @@records_queue = []
  7 +
  8 + @@record_methods = {
  9 + :all => :committed_records,
  10 + :create => :committed_records_on_create,
  11 + :update => :committed_records_on_update,
  12 + :destroy => :committed_records_on_destroy
  13 + }
  14 +
  15 + @@callback_methods = {
  16 + :all => :after_commit_callback,
  17 + :create => :after_commit_on_create_callback,
  18 + :update => :after_commit_on_update_callback,
  19 + :destroy => :after_commit_on_destroy_callback
  20 + }
  21 +
  22 + def self.records
  23 + @@records
  24 + end
  25 +
  26 + # Push a new variable holder onto our stack.
  27 + def self.push
  28 + @@records_queue.push @@records
  29 + @@records = {}
  30 + end
  31 +
  32 + def self.pop
  33 + @@records = @@records_queue.pop
  34 + end
  35 +
  36 + # Send callbacks of this type after a commit.
  37 + # We push a new variable holder on each record, then pop it off, which avoids
  38 + # an infinite loop whereby an on_commit callback makes a new transaction
  39 + # (like in creating a BackgrounDRb record)
  40 + def self.callback(crud_method, &block)
  41 + record_method = @@record_methods[crud_method]
  42 + callback_method = @@callback_methods[crud_method]
  43 + committed_records = send(record_method)
  44 +
  45 + unless committed_records.empty?
  46 + committed_records.each do |record|
  47 + push
  48 + record.send(callback_method)
  49 + pop
  50 + end
  51 + end
  52 +
  53 + records[crud_method] = []
  54 + end
  55 +
  56 + def self.clear!
  57 + @@records = {}
  58 + @@records_queue = []
  59 + end
  60 +
2 61 def self.committed_records
3   - @@committed_records ||= []
  62 + records[:all] ||= []
4 63 end
5 64
6 65 def self.committed_records=(committed_records)
7   - @@committed_records = committed_records
  66 + records[:all] = committed_records
8 67 end
9 68
10 69 def self.committed_records_on_create
11   - @@committed_records_on_create ||= []
  70 + records[:create] ||= []
12 71 end
13 72
14 73 def self.committed_records_on_create=(committed_records)
15   - @@committed_records_on_create = committed_records
  74 + records[:create] = committed_records
16 75 end
17 76
18 77 def self.committed_records_on_update
19   - @@committed_records_on_update ||= []
  78 + records[:update] ||= []
20 79 end
21 80
22 81 def self.committed_records_on_update=(committed_records)
23   - @@committed_records_on_update = committed_records
  82 + records[:update] = committed_records
24 83 end
25 84
26 85 def self.committed_records_on_destroy
27   - @@committed_records_on_destroy ||= []
  86 + records[:destroy] ||= []
28 87 end
29 88
30 89 def self.committed_records_on_destroy=(committed_records)
31   - @@committed_records_on_destroy = committed_records
  90 + records[:destroy] = committed_records
32 91 end
33 92 end
68 lib/after_commit/connection_adapters.rb
@@ -21,80 +21,26 @@ def commit_db_transaction_with_callback
21 21 def rollback_db_transaction_with_callback
22 22 rollback_db_transaction_without_callback
23 23
24   - AfterCommit.committed_records = []
25   - AfterCommit.committed_records_on_create = []
26   - AfterCommit.committed_records_on_update = []
27   - AfterCommit.committed_records_on_destroy = []
  24 + AfterCommit.clear!
28 25 end
29 26 alias_method_chain :rollback_db_transaction, :callback
30 27
31   - protected
  28 + protected
  29 +
32 30 def trigger_after_commit_callbacks
33   - # Trigger the after_commit callback for each of the committed
34   - # records.
35   - if AfterCommit.committed_records.any?
36   - AfterCommit.committed_records.each do |record|
37   - begin
38   - record.after_commit_callback
39   - rescue
40   - end
41   - end
42   - end
43   -
44   - # Make sure we clear out our list of committed records now that we've
45   - # triggered the callbacks for each one.
46   - AfterCommit.committed_records = []
  31 + AfterCommit.callback(:all)
47 32 end
48 33
49 34 def trigger_after_commit_on_create_callbacks
50   - # Trigger the after_commit_on_create callback for each of the committed
51   - # records.
52   - if AfterCommit.committed_records_on_create.any?
53   - AfterCommit.committed_records_on_create.each do |record|
54   - begin
55   - record.after_commit_on_create_callback
56   - rescue
57   - end
58   - end
59   - end
60   -
61   - # Make sure we clear out our list of committed records now that we've
62   - # triggered the callbacks for each one.
63   - AfterCommit.committed_records_on_create = []
  35 + AfterCommit.callback(:create)
64 36 end
65 37
66 38 def trigger_after_commit_on_update_callbacks
67   - # Trigger the after_commit_on_update callback for each of the committed
68   - # records.
69   - if AfterCommit.committed_records_on_update.any?
70   - AfterCommit.committed_records_on_update.each do |record|
71   - begin
72   - record.after_commit_on_update_callback
73   - rescue
74   - end
75   - end
76   - end
77   -
78   - # Make sure we clear out our list of committed records now that we've
79   - # triggered the callbacks for each one.
80   - AfterCommit.committed_records_on_update = []
  39 + AfterCommit.callback(:update)
81 40 end
82 41
83 42 def trigger_after_commit_on_destroy_callbacks
84   - # Trigger the after_commit_on_destroy callback for each of the committed
85   - # records.
86   - if AfterCommit.committed_records_on_destroy.any?
87   - AfterCommit.committed_records_on_destroy.each do |record|
88   - begin
89   - record.after_commit_on_destroy_callback
90   - rescue
91   - end
92   - end
93   - end
94   -
95   - # Make sure we clear out our list of committed records now that we've
96   - # triggered the callbacks for each one.
97   - AfterCommit.committed_records_on_destroy = []
  43 + AfterCommit.callback(:destroy)
98 44 end
99 45 #end protected
100 46 end
33 test/after_commit_test.rb
@@ -8,31 +8,53 @@
8 8
9 9 ActiveRecord::Base.establish_connection({"adapter" => "sqlite3", "database" => 'test.sqlite3'})
10 10 begin
11   - ActiveRecord::Base.connection.execute("drop table mock_records");
  11 + ActiveRecord::Base.connection.execute("drop table mock_records")
  12 + ActiveRecord::Base.connection.execute("drop table mock_non_callbacks")
12 13 rescue
13 14 end
14   -ActiveRecord::Base.connection.execute("create table mock_records(id int)");
  15 +ActiveRecord::Base.connection.execute("create table mock_records(id int)")
  16 +ActiveRecord::Base.connection.execute("create table mock_non_callbacks(id int)")
15 17
16 18 require File.dirname(__FILE__) + '/../init.rb'
17 19
  20 +class MockNonCallback < ActiveRecord::Base; end
  21 +
18 22 class MockRecord < ActiveRecord::Base
  23 + attr_accessor :after_commit_called
19 24 attr_accessor :after_commit_on_create_called
20 25 attr_accessor :after_commit_on_update_called
21 26 attr_accessor :after_commit_on_destroy_called
  27 +
  28 + def clear_flags
  29 + @after_commit_called = @after_commit_on_create_called = @after_commit_on_update_called = @after_commit_on_destroy_called = nil
  30 + end
  31 +
  32 + after_commit :do_commit
  33 + def do_commit
  34 + raise "Re-called on commit!" if self.after_commit_called
  35 + self.after_commit_called = true
  36 + MockNonCallback.transaction{ MockNonCallback.create! }
  37 + end
22 38
23 39 after_commit_on_create :do_create
24 40 def do_create
  41 + raise "Re-called on create!" if self.after_commit_on_create_called
25 42 self.after_commit_on_create_called = true
  43 + MockNonCallback.transaction{ MockNonCallback.create! }
26 44 end
27 45
28 46 after_commit_on_update :do_update
29 47 def do_update
  48 + raise "Re-called on update!" if self.after_commit_on_update_called
30 49 self.after_commit_on_update_called = true
  50 + MockNonCallback.transaction{ MockNonCallback.create! }
31 51 end
32 52
33   - after_commit_on_create :do_destroy
  53 + after_commit_on_destroy :do_destroy
34 54 def do_destroy
  55 + raise "Re-called on destroy!" if self.after_commit_on_destroy_called
35 56 self.after_commit_on_destroy_called = true
  57 + MockNonCallback.transaction{ MockNonCallback.create! }
36 58 end
37 59 end
38 60
@@ -43,11 +65,14 @@ def test_after_commit_on_create_is_called
43 65
44 66 def test_after_commit_on_update_is_called
45 67 record = MockRecord.create!
  68 + record.clear_flags
46 69 record.save
47 70 assert_equal true, record.after_commit_on_update_called
48 71 end
49 72
50 73 def test_after_commit_on_destroy_is_called
51   - assert_equal true, MockRecord.create!.destroy.after_commit_on_destroy_called
  74 + record = MockRecord.create!
  75 + record.clear_flags
  76 + assert_equal true, record.destroy.after_commit_on_destroy_called
52 77 end
53 78 end

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.