Skip to content

Transaction-aware retry create issue to cope with duplicate keys (#8307) #12

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 2, 2019
Merged

Transaction-aware retry create issue to cope with duplicate keys (#8307) #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions models/error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1089,6 +1089,21 @@ func (err ErrIssueLabelTemplateLoad) Error() string {
return fmt.Sprintf("Failed to load label template file '%s': %v", err.TemplateFile, err.OriginalError)
}

// ErrNewIssueInsert is used when the INSERT statement in newIssue fails
type ErrNewIssueInsert struct {
OriginalError error
}

// IsErrNewIssueInsert checks if an error is a ErrNewIssueInsert.
func IsErrNewIssueInsert(err error) bool {
_, ok := err.(ErrNewIssueInsert)
return ok
}

func (err ErrNewIssueInsert) Error() string {
return err.OriginalError.Error()
}

// __________ .__ .__ __________ __
// \______ \__ __| | | |\______ \ ____ ________ __ ____ _______/ |_
// | ___/ | \ | | | | _// __ \/ ____/ | \_/ __ \ / ___/\ __\
Expand Down
40 changes: 23 additions & 17 deletions models/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,21 +1104,10 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
}

// Milestone and assignee validation should happen before insert actual object.

// There's no good way to identify a duplicate key error in database/sql; brute force some retries
dupIndexAttempts := issueMaxDupIndexAttempts
for {
_, err := e.SetExpr("`index`", "coalesce(MAX(`index`),0)+1").
Where("repo_id=?", opts.Issue.RepoID).
Insert(opts.Issue)
if err == nil {
break
}

dupIndexAttempts--
if dupIndexAttempts <= 0 {
return err
}
if _, err := e.SetExpr("`index`", "coalesce(MAX(`index`),0)+1").
Where("repo_id=?", opts.Issue.RepoID).
Insert(opts.Issue); err != nil {
return ErrNewIssueInsert{err}
}

inserted, err := getIssueByID(e, opts.Issue.ID)
Expand Down Expand Up @@ -1201,6 +1190,24 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {

// NewIssue creates new issue with labels for repository.
func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []int64, uuids []string) (err error) {
// Retry several times in case INSERT fails due to duplicate key for (repo_id, index); see #7887
i := 0
for {
if err = newIssueAttempt(repo, issue, labelIDs, assigneeIDs, uuids); err == nil {
return nil
}
if !IsErrNewIssueInsert(err) {
return err
}
if i++; i == issueMaxDupIndexAttempts {
break
}
log.Error("NewIssue: error attempting to insert the new issue; will retry. Original error: %v", err)
}
return fmt.Errorf("NewIssue: too many errors attempting to insert the new issue. Last error was: %v", err)
}

func newIssueAttempt(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []int64, uuids []string) (err error) {
sess := x.NewSession()
defer sess.Close()
if err = sess.Begin(); err != nil {
Expand All @@ -1214,7 +1221,7 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in
Attachments: uuids,
AssigneeIDs: assigneeIDs,
}); err != nil {
if IsErrUserDoesNotHaveAccessToRepo(err) {
if IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) {
return err
}
return fmt.Errorf("newIssue: %v", err)
Expand All @@ -1223,7 +1230,6 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in
if err = sess.Commit(); err != nil {
return fmt.Errorf("Commit: %v", err)
}
sess.Close()

return nil
}
Expand Down
20 changes: 19 additions & 1 deletion models/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,24 @@ func (pr *PullRequest) testPatch(e Engine) (err error) {

// NewPullRequest creates new pull request with labels for repository.
func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest, patch []byte, assigneeIDs []int64) (err error) {
// Retry several times in case INSERT fails due to duplicate key for (repo_id, index); see #7887
i := 0
for {
if err = newPullRequestAttempt(repo, pull, labelIDs, uuids, pr, patch, assigneeIDs); err == nil {
return nil
}
if !IsErrNewIssueInsert(err) {
return err
}
if i++; i == issueMaxDupIndexAttempts {
break
}
log.Error("NewPullRequest: error attempting to insert the new issue; will retry. Original error: %v", err)
}
return fmt.Errorf("NewPullRequest: too many errors attempting to insert the new issue. Last error was: %v", err)
}

func newPullRequestAttempt(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest, patch []byte, assigneeIDs []int64) (err error) {
sess := x.NewSession()
defer sess.Close()
if err = sess.Begin(); err != nil {
Expand All @@ -671,7 +689,7 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str
IsPull: true,
AssigneeIDs: assigneeIDs,
}); err != nil {
if IsErrUserDoesNotHaveAccessToRepo(err) {
if IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) {
return err
}
return fmt.Errorf("newIssue: %v", err)
Expand Down