Commit 4be77d0b authored by Stan Hu's avatar Stan Hu

Improve multiple branch push performance by memoizing permission checking

If you attempt to push thousands of branches at once, the 60-second timeout
will occur because GitAccess checking does a lot of work to check if the
user has permission to push to a branch. This changes does two things:

1. Instead of making 1 DB query per branch push, use a memoized list of protected branches to check
2. Memoize what permissions the user has to perform on this project

On a test of 10,000 branch pushes, this prevents gitlab-shell from hitting the 60-second
timeout.

Closes #17225
parent 4bc4f065
...@@ -4,6 +4,7 @@ v 8.8.0 (unreleased) ...@@ -4,6 +4,7 @@ v 8.8.0 (unreleased)
- Assign labels and milestone to target project when moving issue. !3934 (Long Nguyen) - Assign labels and milestone to target project when moving issue. !3934 (Long Nguyen)
- Project#open_branches has been cleaned up and no longer loads entire records into memory. - Project#open_branches has been cleaned up and no longer loads entire records into memory.
- Escape HTML in commit titles in system note messages - Escape HTML in commit titles in system note messages
- Improve multiple branch push performance by memoizing permission checking
- Log to application.log when an admin starts and stops impersonating a user - Log to application.log when an admin starts and stops impersonating a user
- Updated gitlab_git to 10.1.0 - Updated gitlab_git to 10.1.0
- GitAccess#protected_tag? no longer loads all tags just to check if a single one exists - GitAccess#protected_tag? no longer loads all tags just to check if a single one exists
......
...@@ -767,7 +767,7 @@ class Project < ActiveRecord::Base ...@@ -767,7 +767,7 @@ class Project < ActiveRecord::Base
# Check if current branch name is marked as protected in the system # Check if current branch name is marked as protected in the system
def protected_branch?(branch_name) def protected_branch?(branch_name)
protected_branches.where(name: branch_name).any? protected_branch_names.include?(branch_name)
end end
def developers_can_push_to_protected_branch?(branch_name) def developers_can_push_to_protected_branch?(branch_name)
......
...@@ -122,6 +122,11 @@ module Gitlab ...@@ -122,6 +122,11 @@ module Gitlab
build_status_object(true) build_status_object(true)
end end
def can_user_do_action?(action)
@permission_cache ||= {}
@permission_cache[action] ||= user.can?(action, project)
end
def change_access_check(change) def change_access_check(change)
oldrev, newrev, ref = change.split(' ') oldrev, newrev, ref = change.split(' ')
...@@ -135,7 +140,7 @@ module Gitlab ...@@ -135,7 +140,7 @@ module Gitlab
:push_code :push_code
end end
unless user.can?(action, project) unless can_user_do_action?(action)
status = status =
case action case action
when :force_push_code_to_protected_branches when :force_push_code_to_protected_branches
......
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