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
10 changes: 7 additions & 3 deletions models/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions models/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}

Expand Down
6 changes: 5 additions & 1 deletion models/org.go
Original file line number Diff line number Diff line change
Expand Up @@ -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").
Expand Down
16 changes: 8 additions & 8 deletions models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
18 changes: 13 additions & 5 deletions models/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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)
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand Down
10 changes: 7 additions & 3 deletions models/star.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
}

Expand All @@ -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
}

Expand Down