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

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

Merged
merged 25 commits into from
Oct 2, 2019
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
9dbabcd
Merge go-gitea/master into master
guillep2k Sep 14, 2019
889c619
Merge branch 'master' of github.com:go-gitea/gitea
guillep2k Sep 14, 2019
d7c46c8
Merge branch 'master' of github.com:go-gitea/gitea
guillep2k Sep 15, 2019
8eaacbf
Merge remote-tracking branch 'refs/remotes/origin/master'
guillep2k Sep 19, 2019
de5aa64
Merge branch 'master' of github.com:go-gitea/gitea
guillep2k Sep 19, 2019
80c6f2b
Merge branch 'master' of github.com:go-gitea/gitea
guillep2k Sep 20, 2019
ac40f7f
Merge branch 'master' of github.com:go-gitea/gitea
guillep2k Sep 22, 2019
f6ac46b
Merge branch 'master' of github.com:go-gitea/gitea
guillep2k Sep 23, 2019
b563158
Merge branch 'master' of github.com:go-gitea/gitea
guillep2k Sep 25, 2019
6f55d1e
Merge branch 'master' of github.com:go-gitea/gitea
guillep2k Sep 25, 2019
188e164
Merge branch 'master' of github.com:go-gitea/gitea
guillep2k Sep 26, 2019
dcb3ddb
Revert #7898
guillep2k Sep 27, 2019
9a20c6a
Merge branch 'master' into fix-8301
guillep2k Sep 27, 2019
c786930
Transaction-aware retry create issue to cope with duplicate keys
guillep2k Sep 28, 2019
e6e777f
Restore INSERT ... WHERE usage
guillep2k Sep 29, 2019
765924a
Merge branch 'master' into newfix-7887
guillep2k Sep 29, 2019
27efc1a
Merge 'master' into 'newfix-7887'
guillep2k Sep 30, 2019
07f5e8e
Rearrange code for clarity
guillep2k Oct 1, 2019
3f7f691
Merge branch 'master' into newfix-7887
guillep2k Oct 1, 2019
073a981
Fix error return in newIssue()
guillep2k Oct 1, 2019
ceec120
Fix error message
guillep2k Oct 1, 2019
789e780
Merge branch 'master' into newfix-7887
lunny Oct 2, 2019
bce8641
Merge branch 'master' into newfix-7887
sapk Oct 2, 2019
02607eb
Merge branch 'master' into newfix-7887
lunny Oct 2, 2019
ad1e5f2
Merge branch 'master' into newfix-7887
sapk Oct 2, 2019
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
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
48 changes: 30 additions & 18 deletions models/issue.go
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 err
guillep2k marked this conversation as resolved.
Show resolved Hide resolved
}

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 newIssuePostInsert(issue, repo)
}
if !IsErrNewIssueInsert(err) {
return err
}
guillep2k marked this conversation as resolved.
Show resolved Hide resolved
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 {
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 @@ -1225,7 +1232,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 {
guillep2k marked this conversation as resolved.
Show resolved Hide resolved
if err := NotifyWatchers(&Action{
ActUserID: issue.Poster.ID,
ActUser: issue.Poster,
OpType: ActionCreateIssue,
Expand All @@ -1238,7 +1250,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(),
Expand Down
18 changes: 17 additions & 1 deletion models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
guillep2k marked this conversation as resolved.
Show resolved Hide resolved
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 @@ -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)
Expand Down