From cd13f273d1663ef5f1f506bf3824f79bc73656ff Mon Sep 17 00:00:00 2001
From: guillep2k <18600385+guillep2k@users.noreply.github.com>
Date: Wed, 2 Oct 2019 19:28:30 -0300
Subject: [PATCH] Transaction-aware retry create issue to cope with duplicate
keys (#8307)
* Revert #7898
* Transaction-aware retry create issue to cope with duplicate keys
* Restore INSERT ... WHERE usage
* Rearrange code for clarity
* Fix error return in newIssue()
* Fix error message
---
models/error.go | 15 +++++++++++++++
models/issue.go | 40 +++++++++++++++++++++++-----------------
models/pull.go | 20 +++++++++++++++++++-
3 files changed, 57 insertions(+), 18 deletions(-)
diff --git a/models/error.go b/models/error.go
index 9974287a0a..995617e83b 100644
--- a/models/error.go
+++ b/models/error.go
@@ -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()
+}
+
// __________ .__ .__ __________ __
// \______ \__ __| | | |\______ \ ____ ________ __ ____ _______/ |_
// | ___/ | \ | | | | _// __ \/ ____/ | \_/ __ \ / ___/\ __\
diff --git a/models/issue.go b/models/issue.go
index da98fac7df..09d443384b 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -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)
@@ -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 {
@@ -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)
@@ -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
}
diff --git a/models/pull.go b/models/pull.go
index b41fb004e1..ff1f7b773b 100644
--- a/models/pull.go
+++ b/models/pull.go
@@ -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 {
@@ -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)