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

Ensure that sessions are passed into queries that could use the database to prevent deadlocks #5718

Merged
merged 9 commits into from
Jan 14, 2019

Conversation

zeripath
Copy link
Contributor

I've looked through the gitea code following the use x.NewSession() checking whether every request is made within the session in an attempt to prevent a deadlock.

This PR accounts for most of the potential deadlocks I've found in those requests.

Issues

  • Although https://github.com/go-gitea/gitea/blob/master/models/issue.go#L1349 opens a session its call to IssuesList.loadAttributes cannot be called within the session because of an unusual bug Session not completely cleaned  go-xorm/xorm#1185 . This doesn't need to happen within the session as this session is not a transaction
  • models/repo_mirror.go:SyncMirrors() doesn't pass its session down to its children calls however, it doesn't create a transaction (despite updating the db) - this is likely reasonable as the transaction would lock the db whilst these somewhat long actions were ongoing. This could be refactored to have an ongoing mirror action flag in the db (changed and checked with a transaction of course) that would be cleaned up at the end of the method if data consistency was at risk from this. (I haven't looked deep into this and this is likely a non-issue.)

@zeripath zeripath changed the title More deadlocks Ensure that sessions are passed into queries that could use the database to prevent deadlocks Jan 13, 2019
models/org.go Outdated Show resolved Hide resolved
@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 13, 2019
@codecov-io
Copy link

codecov-io commented Jan 13, 2019

Codecov Report

Merging #5718 into master will increase coverage by <.01%.
The diff coverage is 53.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5718      +/-   ##
==========================================
+ Coverage   37.75%   37.76%   +<.01%     
==========================================
  Files         325      325              
  Lines       47640    47650      +10     
==========================================
+ Hits        17988    17994       +6     
- Misses      27058    27062       +4     
  Partials     2594     2594
Impacted Files Coverage Δ
models/issue_comment.go 46.66% <0%> (ø) ⬆️
models/org.go 69.3% <100%> (+0.12%) ⬆️
models/pull.go 50.75% <37.5%> (ø) ⬆️
models/issue.go 47.64% <40%> (-0.09%) ⬇️
models/repo.go 43.88% <55.55%> (+0.01%) ⬆️
models/star.go 56.09% <80%> (+1.09%) ⬆️

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 6564564...fb6764a. Read the comment docs.

@bkcsoft bkcsoft 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 Jan 14, 2019
@lafriks lafriks added this to the 1.8.0 milestone Jan 14, 2019
@bkcsoft bkcsoft 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 Jan 14, 2019
@techknowlogick techknowlogick merged commit 6868378 into go-gitea:master Jan 14, 2019
@zeripath zeripath deleted the more-deadlocks branch January 23, 2019 20:20
@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
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.

6 participants