Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deadlock when deleting team user #13092

Merged

Conversation

zeripath
Copy link
Contributor

models.getUserRepoPermission(...) calls HasOrgVisible which
uses models.x potentially outside of the transaction e provided
as an argument to getUserRepoPermission.

This PR switches to use hasOrgVisible(e, ...).

Fix #12983

Signed-off-by: Andrew Thornton [email protected]

`models.getUserRepoPermission(...)` calls `HasOrgVisible` which
uses `models.x` potentially outside of the transaction `e` provided
as an argument to `getUserRepoPermission`.

This PR switches to use `hasOrgVisible(e, ...)`.

Fix go-gitea#12983

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added type/bug issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP backport/v1.12 labels Oct 10, 2020
@zeripath zeripath added this to the 1.13.0 milestone Oct 10, 2020
@zeripath
Copy link
Contributor Author

We need to destroy models.x

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 10, 2020
zeripath added a commit to zeripath/gitea that referenced this pull request Oct 10, 2020
Backport go-gitea#13092

`models.getUserRepoPermission(...)` calls `HasOrgVisible` which
uses `models.x` potentially outside of the transaction `e` provided
as an argument to `getUserRepoPermission`.

This PR switches to use `hasOrgVisible(e, ...)`.

Fix go-gitea#12983

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath added the backport/done All backports for this PR have been created label Oct 10, 2020
@codecov-io
Copy link

codecov-io commented Oct 10, 2020

Codecov Report

Merging #13092 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #13092      +/-   ##
==========================================
- Coverage   42.61%   42.60%   -0.02%     
==========================================
  Files         673      673              
  Lines       73862    73862              
==========================================
- Hits        31479    31468      -11     
- Misses      37276    37280       +4     
- Partials     5107     5114       +7     
Impacted Files Coverage Δ
models/repo_permission.go 77.77% <100.00%> (ø)
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
modules/indexer/stats/db.go 43.47% <0.00%> (-17.40%) ⬇️
modules/indexer/stats/queue.go 64.70% <0.00%> (-11.77%) ⬇️
modules/git/utils.go 73.77% <0.00%> (-3.28%) ⬇️
modules/git/repo.go 46.19% <0.00%> (-1.53%) ⬇️
models/gpg_key.go 53.33% <0.00%> (-0.58%) ⬇️
models/error.go 34.34% <0.00%> (-0.51%) ⬇️
services/pull/check.go 48.46% <0.00%> (+0.76%) ⬆️
modules/queue/unique_queue_disk_channel.go 55.38% <0.00%> (+1.53%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d65cd56...710ace1. Read the comment docs.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 10, 2020
lafriks pushed a commit that referenced this pull request Oct 10, 2020
Backport #13092

`models.getUserRepoPermission(...)` calls `HasOrgVisible` which
uses `models.x` potentially outside of the transaction `e` provided
as an argument to `getUserRepoPermission`.

This PR switches to use `hasOrgVisible(e, ...)`.

Fix #12983

Signed-off-by: Andrew Thornton <[email protected]>
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 10, 2020
@lafriks lafriks merged commit 6f27849 into go-gitea:master Oct 10, 2020
@zeripath zeripath deleted the fix-12983-deadlock-in-delete-team-user branch October 10, 2020 20:00
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created issue/critical This issue should be fixed ASAP. If it is a PR, the PR should be merged ASAP lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When deleting members from the organization, the server does not respond after clicking the delete button
5 participants