From 32db9761569d2eaaeae383933bafb761ba6a069e Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 18 Jul 2018 22:04:43 +0100 Subject: [PATCH 1/4] FIX: Stop race condition when topic notification jobs are scheduled during a database transaction This was not picked up by tests because scheduled jobs are run immediately and in the current thread (and therefore the current database transaction). This particular case sometimes occurs inside multiple nested transactions, so simply moving the offending line outside of the transaction is not enough. Implemented TransactionHelper, which allows us to use `TransactionHelper.after_commit` to define code to be run after the current transaction has been committed. --- app/helpers/transaction_helper.rb | 38 +++++++++++++++++++++++++ app/models/topic.rb | 4 ++- spec/helpers/transaction_helper_spec.rb | 32 +++++++++++++++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 app/helpers/transaction_helper.rb create mode 100644 spec/helpers/transaction_helper_spec.rb diff --git a/app/helpers/transaction_helper.rb b/app/helpers/transaction_helper.rb new file mode 100644 index 00000000000..648d0fb948b --- /dev/null +++ b/app/helpers/transaction_helper.rb @@ -0,0 +1,38 @@ +## +# Allows running arbitrary code after the current transaction has been committed. +# Works even with nexted transactions. Useful for scheduling sidekiq jobs. +# Slightly simplified version of https://dev.to/evilmartians/rails-aftercommit-everywhere--4j9g +# Usage: +# Topic.transaction do +# puts "Some work before scheduling" +# TransactionHelper.after_commit do +# puts "Running after commit" +# end +# puts "Some work after scheduling" +# end +# +# Produces: +# > Some work before scheduling +# > Some work after scheduling +# > Running after commit + +module TransactionHelper + class AfterCommitWrapper + def initialize + @callback = Proc.new + end + + def committed!(*) + @callback.call + end + + def before_committed!(*); end + def rolledback!(*); end + end + + def self.after_commit(&blk) + ActiveRecord::Base.connection.add_transaction_record( + AfterCommitWrapper.new(&blk) + ) + end +end diff --git a/app/models/topic.rb b/app/models/topic.rb index 975825a0350..2aa41f6bb6b 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -686,7 +686,9 @@ class Topic < ActiveRecord::Base if post = self.ordered_posts.first notified_user_ids = [post.user_id, post.last_editor_id].uniq - Jobs.enqueue(:notify_category_change, post_id: post.id, notified_user_ids: notified_user_ids) + TransactionHelper.after_commit do + Jobs.enqueue(:notify_category_change, post_id: post.id, notified_user_ids: notified_user_ids) + end end end diff --git a/spec/helpers/transaction_helper_spec.rb b/spec/helpers/transaction_helper_spec.rb new file mode 100644 index 00000000000..d1c4a3e4947 --- /dev/null +++ b/spec/helpers/transaction_helper_spec.rb @@ -0,0 +1,32 @@ +require 'rails_helper' + +describe TransactionHelper do + + it "runs callbacks after outermost transaction is committed" do + outputString = "1" + + # Main transaction + ActiveRecord::Base.transaction do + outputString += "2" + + # Nested transaction + ActiveRecord::Base.transaction do + outputString += "3" + + TransactionHelper.after_commit do + outputString += "6" + end + outputString += "4" + end + + TransactionHelper.after_commit do + outputString += "7" + end + + outputString += "5" + end + + expect(outputString).to eq("1234567") + end + +end From 5b4b632358cc8827394eed4e906f57115d77ab45 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Wed, 18 Jul 2018 22:27:55 +0100 Subject: [PATCH 2/4] Correct typo --- app/helpers/transaction_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/transaction_helper.rb b/app/helpers/transaction_helper.rb index 648d0fb948b..f76eb97723b 100644 --- a/app/helpers/transaction_helper.rb +++ b/app/helpers/transaction_helper.rb @@ -1,6 +1,6 @@ ## # Allows running arbitrary code after the current transaction has been committed. -# Works even with nexted transactions. Useful for scheduling sidekiq jobs. +# Works even with nested transactions. Useful for scheduling sidekiq jobs. # Slightly simplified version of https://dev.to/evilmartians/rails-aftercommit-everywhere--4j9g # Usage: # Topic.transaction do From d7b5a374c2e96e52e67c6ca9c0500724016242f1 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Mon, 23 Jul 2018 10:42:15 +0100 Subject: [PATCH 3/4] Fix indentation --- app/helpers/transaction_helper.rb | 2 +- spec/helpers/transaction_helper_spec.rb | 34 ++++++++++++------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/app/helpers/transaction_helper.rb b/app/helpers/transaction_helper.rb index f76eb97723b..efb98d6a53e 100644 --- a/app/helpers/transaction_helper.rb +++ b/app/helpers/transaction_helper.rb @@ -32,7 +32,7 @@ module TransactionHelper def self.after_commit(&blk) ActiveRecord::Base.connection.add_transaction_record( - AfterCommitWrapper.new(&blk) + AfterCommitWrapper.new(&blk) ) end end diff --git a/spec/helpers/transaction_helper_spec.rb b/spec/helpers/transaction_helper_spec.rb index d1c4a3e4947..01908b15aaa 100644 --- a/spec/helpers/transaction_helper_spec.rb +++ b/spec/helpers/transaction_helper_spec.rb @@ -5,28 +5,28 @@ describe TransactionHelper do it "runs callbacks after outermost transaction is committed" do outputString = "1" - # Main transaction - ActiveRecord::Base.transaction do - outputString += "2" + # Main transaction + ActiveRecord::Base.transaction do + outputString += "2" - # Nested transaction - ActiveRecord::Base.transaction do - outputString += "3" + # Nested transaction + ActiveRecord::Base.transaction do + outputString += "3" - TransactionHelper.after_commit do - outputString += "6" - end - outputString += "4" - end + TransactionHelper.after_commit do + outputString += "6" + end + outputString += "4" + end - TransactionHelper.after_commit do - outputString += "7" - end + TransactionHelper.after_commit do + outputString += "7" + end - outputString += "5" - end + outputString += "5" + end - expect(outputString).to eq("1234567") + expect(outputString).to eq("1234567") end end From 20a21b1240a1b28dabee8bf97f1dd194a33053eb Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 24 Jul 2018 09:41:55 +0100 Subject: [PATCH 4/4] Move into MiniSQLMultisiteConnection, and add test for rollback --- app/helpers/transaction_helper.rb | 38 -------------- app/models/topic.rb | 2 +- lib/mini_sql_multisite_connection.rb | 21 ++++++++ spec/helpers/transaction_helper_spec.rb | 32 ------------ .../lib/mini_sql_multisite_connection_spec.rb | 52 +++++++++++++++++++ 5 files changed, 74 insertions(+), 71 deletions(-) delete mode 100644 app/helpers/transaction_helper.rb delete mode 100644 spec/helpers/transaction_helper_spec.rb create mode 100644 spec/lib/mini_sql_multisite_connection_spec.rb diff --git a/app/helpers/transaction_helper.rb b/app/helpers/transaction_helper.rb deleted file mode 100644 index efb98d6a53e..00000000000 --- a/app/helpers/transaction_helper.rb +++ /dev/null @@ -1,38 +0,0 @@ -## -# Allows running arbitrary code after the current transaction has been committed. -# Works even with nested transactions. Useful for scheduling sidekiq jobs. -# Slightly simplified version of https://dev.to/evilmartians/rails-aftercommit-everywhere--4j9g -# Usage: -# Topic.transaction do -# puts "Some work before scheduling" -# TransactionHelper.after_commit do -# puts "Running after commit" -# end -# puts "Some work after scheduling" -# end -# -# Produces: -# > Some work before scheduling -# > Some work after scheduling -# > Running after commit - -module TransactionHelper - class AfterCommitWrapper - def initialize - @callback = Proc.new - end - - def committed!(*) - @callback.call - end - - def before_committed!(*); end - def rolledback!(*); end - end - - def self.after_commit(&blk) - ActiveRecord::Base.connection.add_transaction_record( - AfterCommitWrapper.new(&blk) - ) - end -end diff --git a/app/models/topic.rb b/app/models/topic.rb index 2aa41f6bb6b..94f2a8566fc 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -686,7 +686,7 @@ class Topic < ActiveRecord::Base if post = self.ordered_posts.first notified_user_ids = [post.user_id, post.last_editor_id].uniq - TransactionHelper.after_commit do + DB.after_commit do Jobs.enqueue(:notify_category_change, post_id: post.id, notified_user_ids: notified_user_ids) end end diff --git a/lib/mini_sql_multisite_connection.rb b/lib/mini_sql_multisite_connection.rb index c6842fe19ab..c628ef644b5 100644 --- a/lib/mini_sql_multisite_connection.rb +++ b/lib/mini_sql_multisite_connection.rb @@ -23,6 +23,27 @@ class MiniSqlMultisiteConnection < MiniSql::Connection end end + class AfterCommitWrapper + def initialize + @callback = Proc.new + end + + def committed!(*) + @callback.call + end + + def before_committed!(*); end + def rolledback!(*); end + end + + # Allows running arbitrary code after the current transaction has been committed. + # Works even with nested transactions. Useful for scheduling sidekiq jobs. + def after_commit(&blk) + ActiveRecord::Base.connection.add_transaction_record( + AfterCommitWrapper.new(&blk) + ) + end + def self.instance new(nil, param_encoder: ParamEncoder.new) end diff --git a/spec/helpers/transaction_helper_spec.rb b/spec/helpers/transaction_helper_spec.rb deleted file mode 100644 index 01908b15aaa..00000000000 --- a/spec/helpers/transaction_helper_spec.rb +++ /dev/null @@ -1,32 +0,0 @@ -require 'rails_helper' - -describe TransactionHelper do - - it "runs callbacks after outermost transaction is committed" do - outputString = "1" - - # Main transaction - ActiveRecord::Base.transaction do - outputString += "2" - - # Nested transaction - ActiveRecord::Base.transaction do - outputString += "3" - - TransactionHelper.after_commit do - outputString += "6" - end - outputString += "4" - end - - TransactionHelper.after_commit do - outputString += "7" - end - - outputString += "5" - end - - expect(outputString).to eq("1234567") - end - -end diff --git a/spec/lib/mini_sql_multisite_connection_spec.rb b/spec/lib/mini_sql_multisite_connection_spec.rb new file mode 100644 index 00000000000..c4213d9180d --- /dev/null +++ b/spec/lib/mini_sql_multisite_connection_spec.rb @@ -0,0 +1,52 @@ +require 'rails_helper' + +describe MiniSqlMultisiteConnection do + + describe "after_commit" do + it "runs callbacks after outermost transaction is committed" do + outputString = "1" + + # Main transaction + ActiveRecord::Base.transaction do + outputString += "2" + + # Nested transaction + ActiveRecord::Base.transaction do + outputString += "3" + + DB.after_commit do + outputString += "6" + end + outputString += "4" + end + + DB.after_commit do + outputString += "7" + end + + outputString += "5" + end + + expect(outputString).to eq("1234567") + end + + it "does not run if the transaction is rolled back" do + outputString = "1" + + ActiveRecord::Base.transaction do + outputString += "2" + + DB.after_commit do + outputString += "4" + end + + outputString += "3" + + raise ActiveRecord::Rollback + end + + expect(outputString).to eq("123") + end + end + +end