From c786930c12ea83f7706f56c94794f4eed07f143b Mon Sep 17 00:00:00 2001 From: Guillermo Prandi Date: Sat, 28 Sep 2019 04:46:01 -0300 Subject: [PATCH] Transaction-aware retry create issue to cope with duplicate keys --- models/error.go | 15 +++++++++++++++ models/issue.go | 32 ++++++++++++++++++++++++++++---- models/pull.go | 18 +++++++++++++++++- 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/models/error.go b/models/error.go index 9974287a0aef1..995617e83b8ae 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 eea39fb2c5bae..cc81a52b54113 100644 --- a/models/issue.go +++ b/models/issue.go @@ -74,6 +74,7 @@ var ( const issueTasksRegexpStr = `(^\s*[-*]\s\[[\sx]\]\s.)|(\n\s*[-*]\s\[[\sx]\]\s.)` const issueTasksDoneRegexpStr = `(^\s*[-*]\s\[[x]\]\s.)|(\n\s*[-*]\s\[[x]\]\s.)` +const issueMaxDupIndexAttempts = 3 func init() { issueTasksPat = regexp.MustCompile(issueTasksRegexpStr) @@ -1132,7 +1133,7 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) { // Milestone and assignee validation should happen before insert actual object. if _, err = e.Insert(opts.Issue); err != nil { - return err + return ErrNewIssueInsert{err} } if opts.Issue.MilestoneID > 0 { @@ -1207,6 +1208,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 newIssuePostInsert(issue, repo) + } + 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 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 { @@ -1220,7 +1239,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) @@ -1231,7 +1250,12 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in } sess.Close() - if err = NotifyWatchers(&Action{ + return nil +} + +// newIssuePostInsert performs issue creation operations that need to be separate transactions +func newIssuePostInsert(issue *Issue, repo *Repository) error { + if err := NotifyWatchers(&Action{ ActUserID: issue.Poster.ID, ActUser: issue.Poster, OpType: ActionCreateIssue, @@ -1244,7 +1268,7 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, assigneeIDs []in } mode, _ := AccessLevel(issue.Poster, issue.Repo) - if err = PrepareWebhooks(repo, HookEventIssues, &api.IssuePayload{ + if err := PrepareWebhooks(repo, HookEventIssues, &api.IssuePayload{ Action: api.HookIssueOpened, Index: issue.Index, Issue: issue.APIFormat(), diff --git a/models/pull.go b/models/pull.go index 60ccb144f510a..e35509ddf2852 100644 --- a/models/pull.go +++ b/models/pull.go @@ -654,6 +654,22 @@ 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); !IsErrNewIssueInsert(err) { + // Usually err == nil + 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 { @@ -668,7 +684,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)