diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2016-11-17 19:23:42 +0530 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2016-11-18 14:27:13 +0530 |
commit | f95fa7145b41dbeb0cbaea1e9677fb7dd65a2f04 (patch) | |
tree | b5defbfcfb4f849eb2ce1acee6f26547a4b55d98 | |
parent | bf9ab0f33d65df1566fcde8100576d23f5c77a4f (diff) | |
download | gitlab-ce-f95fa7145b41dbeb0cbaea1e9677fb7dd65a2f04.tar.gz |
Write a spec covering the race condition during group deletion.
- Use multiple threads / database connections to:
1. Escape the transaction the spec seems to be running
in (`config.use_transactional_fixtures` is off, but
`ActiveRecord::Base.connection.open_transactions` is not empty
at the beginning of the spec.
2. Simulate a Sidekiq worker performing the hard delete outside of the
soft-delete transaction.
- The spec is a little clunky, but it was the smallest thing I could get
working - and even this took a couple of hours. Let me know if you
have any suggestions to improve it!
-rw-r--r-- | spec/services/destroy_group_service_spec.rb | 40 | ||||
-rw-r--r-- | spec/support/database_connection_helpers.rb | 9 |
2 files changed, 49 insertions, 0 deletions
diff --git a/spec/services/destroy_group_service_spec.rb b/spec/services/destroy_group_service_spec.rb index da724643604..538e85cdc89 100644 --- a/spec/services/destroy_group_service_spec.rb +++ b/spec/services/destroy_group_service_spec.rb @@ -1,6 +1,8 @@ require 'spec_helper' describe DestroyGroupService, services: true do + include DatabaseConnectionHelpers + let!(:user) { create(:user) } let!(:group) { create(:group) } let!(:project) { create(:project, namespace: group) } @@ -50,6 +52,44 @@ describe DestroyGroupService, services: true do describe 'asynchronous delete' do it_behaves_like 'group destruction', true + + context 'potential race conditions' do + context "when the `GroupDestroyWorker` task runs immediately" do + it "deletes the group" do + # Commit the contents of this spec's transaction so far + # so subsequent db connections can see it. + # + # DO NOT REMOVE THIS LINE, even if you see a WARNING with "No + # transaction is currently in progress". Without this, this + # spec will always be green, since the group created in setup + # cannot be seen by any other connections / threads in this spec. + Group.connection.commit_db_transaction + + group_record = run_with_new_database_connection do |conn| + conn.execute("SELECT * FROM namespaces WHERE id = #{group.id}").first + end + + expect(group_record).not_to be_nil + + # Execute the contents of `GroupDestroyWorker` in a separate thread, to + # simulate data manipulation by the Sidekiq worker (different database + # connection / transaction). + expect(GroupDestroyWorker).to receive(:perform_async).and_wrap_original do |m, group_id, user_id| + Thread.new { m[group_id, user_id] }.join(5) + end + + # Kick off the initial group destroy in a new thread, so that + # it doesn't share this spec's database transaction. + Thread.new { DestroyGroupService.new(group, user).async_execute }.join(5) + + group_record = run_with_new_database_connection do |conn| + conn.execute("SELECT * FROM namespaces WHERE id = #{group.id}").first + end + + expect(group_record).to be_nil + end + end + end end describe 'synchronous delete' do diff --git a/spec/support/database_connection_helpers.rb b/spec/support/database_connection_helpers.rb new file mode 100644 index 00000000000..763329499f0 --- /dev/null +++ b/spec/support/database_connection_helpers.rb @@ -0,0 +1,9 @@ +module DatabaseConnectionHelpers + def run_with_new_database_connection + pool = ActiveRecord::Base.connection_pool + conn = pool.checkout + yield conn + ensure + pool.checkin(conn) + end +end |