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

* Fixed deadlock in CreateComment

* Fix possible deadlock in UpdateIssueDeadline from createDeadlineComment

* Ensure that calls to IsTimeTracker enabled are called within session

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Ensure that calls to reactionList are also called within session

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Ensure all calls in NewPullRequest with the session are called within the session

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Deal with potential deadlocks in repo

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Ensure that isStaring is checked within our transaction

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Fix mistake in isOrganizationMember

Sorry.

models/issue.go

@@ -103,7 +103,11 @@ func (issue *Issue) loadRepo(e Engine) (err error) {
 
 // IsTimetrackerEnabled returns true if the repo enables timetracking
 func (issue *Issue) IsTimetrackerEnabled() bool {
-	if err := issue.loadRepo(x); err != nil {
+	return issue.isTimetrackerEnabled(x)
+}
+
+func (issue *Issue) isTimetrackerEnabled(e Engine) bool {
+	if err := issue.loadRepo(e); err != nil {
 		log.Error(4, fmt.Sprintf("loadRepo: %v", err))
 		return false
 	}
@@ -196,7 +200,7 @@ func (issue *Issue) loadReactions(e Engine) (err error) {
 		return err
 	}
 	// Load reaction user data
-	if _, err := ReactionList(reactions).LoadUsers(); err != nil {
+	if _, err := ReactionList(reactions).loadUsers(e); err != nil {
 		return err
 	}
 
@@ -255,7 +259,7 @@ func (issue *Issue) loadAttributes(e Engine) (err error) {
 	if err = issue.loadComments(e); err != nil {
 		return err
 	}
-	if issue.IsTimetrackerEnabled() {
+	if issue.isTimetrackerEnabled(e) {
 		if err = issue.loadTotalTimes(e); err != nil {
 			return err
 		}

models/issue_comment.go

@@ -559,7 +559,7 @@ func sendCreateCommentAction(e *xorm.Session, opts *CreateCommentOptions, commen
 	case CommentTypeCode:
 		if comment.ReviewID != 0 {
 			if comment.Review == nil {
-				if err := comment.LoadReview(); err != nil {
+				if err := comment.loadReview(e); err != nil {
 					return err
 				}
 			}
@@ -709,7 +709,7 @@ func createDeadlineComment(e *xorm.Session, doer *User, issue *Issue, newDeadlin
 		content = newDeadlineUnix.Format("2006-01-02") + "|" + issue.DeadlineUnix.Format("2006-01-02")
 	}
 
-	if err := issue.LoadRepo(); err != nil {
+	if err := issue.loadRepo(e); err != nil {
 		return nil, err
 	}
 

models/org.go

@@ -314,7 +314,11 @@ func IsOrganizationOwner(orgID, uid int64) (bool, error) {
 
 // IsOrganizationMember returns true if given user is member of organization.
 func IsOrganizationMember(orgID, uid int64) (bool, error) {
-	return x.
+	return isOrganizationMember(x, orgID, uid)
+}
+
+func isOrganizationMember(e Engine, orgID, uid int64) (bool, error) {
+	return e.
 		Where("uid=?", uid).
 		And("org_id=?", orgID).
 		Table("org_user").

models/pull.go

@@ -721,15 +721,15 @@ var patchConflicts = []string{
 }
 
 // testPatch checks if patch can be merged to base repository without conflict.
-func (pr *PullRequest) testPatch() (err error) {
+func (pr *PullRequest) testPatch(e Engine) (err error) {
 	if pr.BaseRepo == nil {
-		pr.BaseRepo, err = GetRepositoryByID(pr.BaseRepoID)
+		pr.BaseRepo, err = getRepositoryByID(e, pr.BaseRepoID)
 		if err != nil {
 			return fmt.Errorf("GetRepositoryByID: %v", err)
 		}
 	}
 
-	patchPath, err := pr.BaseRepo.PatchPath(pr.Index)
+	patchPath, err := pr.BaseRepo.patchPath(e, pr.Index)
 	if err != nil {
 		return fmt.Errorf("BaseRepo.PatchPath: %v", err)
 	}
@@ -758,7 +758,7 @@ func (pr *PullRequest) testPatch() (err error) {
 		return fmt.Errorf("git read-tree --index-output=%s %s: %v - %s", indexTmpPath, pr.BaseBranch, err, stderr)
 	}
 
-	prUnit, err := pr.BaseRepo.GetUnit(UnitTypePullRequests)
+	prUnit, err := pr.BaseRepo.getUnit(e, UnitTypePullRequests)
 	if err != nil {
 		return err
 	}
@@ -811,12 +811,12 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str
 	}
 
 	pr.Index = pull.Index
-	if err = repo.SavePatch(pr.Index, patch); err != nil {
+	if err = repo.savePatch(sess, pr.Index, patch); err != nil {
 		return fmt.Errorf("SavePatch: %v", err)
 	}
 
 	pr.BaseRepo = repo
-	if err = pr.testPatch(); err != nil {
+	if err = pr.testPatch(sess); err != nil {
 		return fmt.Errorf("testPatch: %v", err)
 	}
 	// No conflict appears after test means mergeable.
@@ -1363,7 +1363,7 @@ func TestPullRequests() {
 		if pr.manuallyMerged() {
 			continue
 		}
-		if err := pr.testPatch(); err != nil {
+		if err := pr.testPatch(x); err != nil {
 			log.Error(3, "testPatch: %v", err)
 			continue
 		}
@@ -1387,7 +1387,7 @@ func TestPullRequests() {
 			continue
 		} else if pr.manuallyMerged() {
 			continue
-		} else if err = pr.testPatch(); err != nil {
+		} else if err = pr.testPatch(x); err != nil {
 			log.Error(4, "testPatch[%d]: %v", pr.ID, err)
 			continue
 		}

models/repo.go

@@ -776,7 +776,11 @@ func (repo *Repository) UpdateLocalCopyBranch(branch string) error {
 
 // PatchPath returns corresponding patch file path of repository by given issue ID.
 func (repo *Repository) PatchPath(index int64) (string, error) {
-	if err := repo.GetOwner(); err != nil {
+	return repo.patchPath(x, index)
+}
+
+func (repo *Repository) patchPath(e Engine, index int64) (string, error) {
+	if err := repo.getOwner(e); err != nil {
 		return "", err
 	}
 
@@ -785,7 +789,11 @@ func (repo *Repository) PatchPath(index int64) (string, error) {
 
 // SavePatch saves patch data to corresponding location by given issue ID.
 func (repo *Repository) SavePatch(index int64, patch []byte) error {
-	patchPath, err := repo.PatchPath(index)
+	return repo.savePatch(x, index, patch)
+}
+
+func (repo *Repository) savePatch(e Engine, index int64, patch []byte) error {
+	patchPath, err := repo.patchPath(e, index)
 	if err != nil {
 		return fmt.Errorf("PatchPath: %v", err)
 	}
@@ -1479,7 +1487,7 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
 	collaboration := &Collaboration{RepoID: repo.ID}
 	for _, c := range collaborators {
 		if c.ID != newOwner.ID {
-			isMember, err := newOwner.IsOrgMember(c.ID)
+			isMember, err := isOrganizationMember(sess, newOwner.ID, c.ID)
 			if err != nil {
 				return fmt.Errorf("IsOrgMember: %v", err)
 			} else if !isMember {
@@ -1536,12 +1544,12 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
 	if err = os.Rename(RepoPath(owner.Name, repo.Name), RepoPath(newOwner.Name, repo.Name)); err != nil {
 		return fmt.Errorf("rename repository directory: %v", err)
 	}
-	RemoveAllWithNotice("Delete repository local copy", repo.LocalCopyPath())
+	removeAllWithNotice(sess, "Delete repository local copy", repo.LocalCopyPath())
 
 	// Rename remote wiki repository to new path and delete local copy.
 	wikiPath := WikiPath(owner.Name, repo.Name)
 	if com.IsExist(wikiPath) {
-		RemoveAllWithNotice("Delete repository wiki local copy", repo.LocalWikiPath())
+		removeAllWithNotice(sess, "Delete repository wiki local copy", repo.LocalWikiPath())
 		if err = os.Rename(wikiPath, WikiPath(newOwner.Name, repo.Name)); err != nil {
 			return fmt.Errorf("rename repository wiki: %v", err)
 		}

models/star.go

@@ -21,7 +21,7 @@ func StarRepo(userID, repoID int64, star bool) error {
 	}
 
 	if star {
-		if IsStaring(userID, repoID) {
+		if isStaring(sess, userID, repoID) {
 			return nil
 		}
 
@@ -35,7 +35,7 @@ func StarRepo(userID, repoID int64, star bool) error {
 			return err
 		}
 	} else {
-		if !IsStaring(userID, repoID) {
+		if !isStaring(sess, userID, repoID) {
 			return nil
 		}
 
@@ -55,7 +55,11 @@ func StarRepo(userID, repoID int64, star bool) error {
 
 // IsStaring checks if user has starred given repository.
 func IsStaring(userID, repoID int64) bool {
-	has, _ := x.Get(&Star{0, userID, repoID})
+	return isStaring(x, userID, repoID)
+}
+
+func isStaring(e Engine, userID, repoID int64) bool {
+	has, _ := e.Get(&Star{0, userID, repoID})
 	return has
 }