Commit 6f88984b authored by Yorick Peterse's avatar Yorick Peterse

Synchronize all project authorization refreshing

Previously a lease would only be obtained to update data. This could
lead to duplicate data being inserted, triggering a UNIQUE constraint
error. To work around this we now acquire a lease before performing
_any_ project authorization work, releasing it at the very end.

Fixes #25987
parent 142be72a
...@@ -26,8 +26,26 @@ module Users ...@@ -26,8 +26,26 @@ module Users
user.reload user.reload
end end
# This method returns the updated User object.
def execute def execute
lease_key = "refresh_authorized_projects:#{user.id}"
lease = Gitlab::ExclusiveLease.new(lease_key, timeout: LEASE_TIMEOUT)
until uuid = lease.try_obtain
# Keep trying until we obtain the lease. If we don't do so we may end up
# not updating the list of authorized projects properly. To prevent
# hammering Redis too much we'll wait for a bit between retries.
sleep(1)
end
begin
execute_without_lease
ensure
Gitlab::ExclusiveLease.cancel(lease_key, uuid)
end
end
# This method returns the updated User object.
def execute_without_lease
current = current_authorizations_per_project current = current_authorizations_per_project
fresh = fresh_access_levels_per_project fresh = fresh_access_levels_per_project
...@@ -47,26 +65,7 @@ module Users ...@@ -47,26 +65,7 @@ module Users
end end
end end
update_with_lease(remove, add) update_authorizations(remove, add)
end
# Updates the list of authorizations using an exclusive lease.
def update_with_lease(remove = [], add = [])
lease_key = "refresh_authorized_projects:#{user.id}"
lease = Gitlab::ExclusiveLease.new(lease_key, timeout: LEASE_TIMEOUT)
until uuid = lease.try_obtain
# Keep trying until we obtain the lease. If we don't do so we may end up
# not updating the list of authorized projects properly. To prevent
# hammering Redis too much we'll wait for a bit between retries.
sleep(1)
end
begin
update_authorizations(remove, add)
ensure
Gitlab::ExclusiveLease.cancel(lease_key, uuid)
end
end end
# Updates the list of authorizations for the current user. # Updates the list of authorizations for the current user.
......
---
title: Synchronize all project authorization refreshing work to prevent race conditions
merge_request:
author:
...@@ -10,7 +10,21 @@ describe Users::RefreshAuthorizedProjectsService do ...@@ -10,7 +10,21 @@ describe Users::RefreshAuthorizedProjectsService do
create!(project: project, user: user, access_level: access_level) create!(project: project, user: user, access_level: access_level)
end end
describe '#execute' do describe '#execute', :redis do
it 'refreshes the authorizations using a lease' do
expect_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).
and_return('foo')
expect(Gitlab::ExclusiveLease).to receive(:cancel).
with(an_instance_of(String), 'foo')
expect(service).to receive(:execute_without_lease)
service.execute
end
end
describe '#execute_without_lease' do
before do before do
user.project_authorizations.delete_all user.project_authorizations.delete_all
end end
...@@ -19,37 +33,23 @@ describe Users::RefreshAuthorizedProjectsService do ...@@ -19,37 +33,23 @@ describe Users::RefreshAuthorizedProjectsService do
project2 = create(:empty_project) project2 = create(:empty_project)
to_remove = create_authorization(project2, user) to_remove = create_authorization(project2, user)
expect(service).to receive(:update_with_lease). expect(service).to receive(:update_authorizations).
with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]]) with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]])
service.execute service.execute_without_lease
end end
it 'sets the access level of a project to the highest available level' do it 'sets the access level of a project to the highest available level' do
to_remove = create_authorization(project, user, Gitlab::Access::DEVELOPER) to_remove = create_authorization(project, user, Gitlab::Access::DEVELOPER)
expect(service).to receive(:update_with_lease). expect(service).to receive(:update_authorizations).
with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]]) with([to_remove.project_id], [[user.id, project.id, Gitlab::Access::MASTER]])
service.execute service.execute_without_lease
end end
it 'returns a User' do it 'returns a User' do
expect(service.execute).to be_an_instance_of(User) expect(service.execute_without_lease).to be_an_instance_of(User)
end
end
describe '#update_with_lease', :redis do
it 'refreshes the authorizations using a lease' do
expect_any_instance_of(Gitlab::ExclusiveLease).to receive(:try_obtain).
and_return('foo')
expect(Gitlab::ExclusiveLease).to receive(:cancel).
with(an_instance_of(String), 'foo')
expect(service).to receive(:update_authorizations).with([1], [])
service.update_with_lease([1])
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment