From 333aec5d60d006abab52ecdfe1b06fae8078a4df Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Mon, 20 Jan 2020 00:11:53 -0300
Subject: [PATCH 01/17] Passes PSQL, SQLite and MSSQL

---
 integrations/locked_resource_test.go | 128 +++++++++++++++++++++++++++
 models/locked_resource.go            |  88 ++++++++++++++++++
 models/locked_resource_test.go       |  76 ++++++++++++++++
 models/migrations/migrations.go      |   2 +
 models/migrations/v125.go            |  20 +++++
 models/models.go                     |   1 +
 6 files changed, 315 insertions(+)
 create mode 100644 integrations/locked_resource_test.go
 create mode 100644 models/locked_resource.go
 create mode 100644 models/locked_resource_test.go
 create mode 100644 models/migrations/v125.go

diff --git a/integrations/locked_resource_test.go b/integrations/locked_resource_test.go
new file mode 100644
index 0000000000000..c4f9707b08b4c
--- /dev/null
+++ b/integrations/locked_resource_test.go
@@ -0,0 +1,128 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package integrations
+
+import (
+	"fmt"
+	"testing"
+	"time"
+
+	"code.gitea.io/gitea/models"
+
+	"github.com/stretchr/testify/assert"
+)
+
+const (
+	// Note: these values might require tuning
+	before = 500 * time.Millisecond
+	after = 1000 * time.Millisecond
+	tolerance = 200 * time.Millisecond
+)
+
+type waitResult struct {
+	Waited	time.Duration
+	Err		error
+}
+
+func TestLockedResource(t *testing.T) {
+	defer prepareTestEnv(t)()
+
+	// We need to check whether two goroutines block each other
+	// Sadly, there's no way to ensure the second goroutine is
+	// waiting other than using a time delay. The longer the delay,
+	// the more certain we are the second goroutine is waiting.
+
+	// This check **must** fail as we're not blocking anything
+	assert.Error(t, blockTest("no block", func(ctx models.DBContext) (func() error, error){
+		return func() error{
+			return nil
+		}, nil
+	}))
+
+	models.AssertNotExistsBean(t, &models.LockedResource{LockType: "test-1", LockKey: 1})
+
+	// Test with creation (i.e. new lock type)
+	assert.NoError(t, blockTest("block-new", func(ctx models.DBContext) (func() error, error){
+		_, err := models.GetLockedResourceCtx(ctx, "block-test-1", 1)
+		return func() error{
+			return nil
+		}, err
+	}))
+
+	// Test without creation (i.e. lock type already exists)
+	assert.NoError(t, blockTest("block-existing", func(ctx models.DBContext) (func() error, error){
+		_, err := models.GetLockedResourceCtx(ctx, "block-test-1", 1)
+		return func() error{
+			return nil
+		}, err
+	}))
+
+	// Test with temporary record
+	assert.NoError(t, blockTest("block-temp", func(ctx models.DBContext) (func() error, error){
+		return models.TempLockResourceCtx(ctx, "temp-1", 1)
+	}))
+}
+
+func blockTest(name string, f func(ctx models.DBContext) (func() error, error)) error {
+	cb := make(chan waitResult)
+	cw := make(chan waitResult)
+	ref := time.Now()
+
+	go func() {
+		cb <- blockTestFunc(name, true, ref, f)
+	}()
+	go func() {
+		cw <- blockTestFunc(name, false, ref, f)
+	}()
+
+	resb := <- cb
+	resw := <- cw
+	if resb.Err != nil {
+		return resb.Err
+	}
+	if resw.Err != nil {
+		return resw.Err
+	}
+
+	if resw.Waited < after - tolerance {
+		return fmt.Errorf("Waiter not blocked on %s; wait: %d ms, expected > %d ms",
+			name, resw.Waited.Milliseconds(), (after - tolerance).Milliseconds())
+	}
+
+	return nil
+}
+
+func blockTestFunc(name string, blocker bool, ref time.Time, f func(ctx models.DBContext) (func() error, error)) (wr waitResult) {
+	if blocker {
+		name = fmt.Sprintf("blocker [%s]", name)
+	} else {
+		name = fmt.Sprintf("waiter [%s]", name)
+	}
+	err := models.WithTx(func(ctx models.DBContext) error {
+		fmt.Printf("Entering %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
+		if !blocker {
+			fmt.Printf("Waiting on %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
+			time.Sleep(before)
+			fmt.Printf("Wait finished on %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
+		}
+		releaseLock, err := f(ctx)
+		if err != nil {
+			return err
+		}
+		if blocker {
+			fmt.Printf("Waiting on %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
+			time.Sleep(after)
+			fmt.Printf("Wait finished on %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
+		} else {
+			wr.Waited = time.Now().Sub(ref)
+		}
+		fmt.Printf("Finishing %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
+		return releaseLock()
+	})
+	if err != nil {
+		wr.Err = fmt.Errorf("error in %s: %v", name, err)
+	}
+	return
+}
diff --git a/models/locked_resource.go b/models/locked_resource.go
new file mode 100644
index 0000000000000..88cec83f4c3ae
--- /dev/null
+++ b/models/locked_resource.go
@@ -0,0 +1,88 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package models
+
+import (
+	"fmt"
+)
+
+type LockedResource struct {
+	LockType	string      `xorm:"pk"`
+	LockKey		int64		`xorm:"pk"`
+	Counter		int64		`xorm:"NOT NULL DEFAULT 0"`
+}
+
+func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResource, error) {
+	locked := &LockedResource{LockType: lockType, LockKey: lockKey}
+	// SQLite3 has no ForUpdate() clause and an UPSERT strategy has many
+	// problems and fallbacks; we perform a bogus update on the table
+	// which will lock the key in a safe way.
+	// Make sure to leave `counter` out of the update.
+	count, err := e.Table(locked).Cols("lock_type", "lock_key").Update(locked)
+	if err != nil {
+		return nil, fmt.Errorf("get locked resource %s:%d: %v", lockType, lockKey, err)
+	}
+	if count == 0 {
+		// No record was found; since the key is now locked,
+		// it's safe to insert a record.
+		_, err = e.Insert(locked)
+		if err != nil {
+			return nil, fmt.Errorf("get locked resource %s:%d: %v", lockType, lockKey, err)
+		}
+	} else {
+		// Read back the record we've locked
+		has, err := e.Table(locked).Get(locked)
+		if err != nil {
+			return nil, fmt.Errorf("get locked resource %s:%d: %v", lockType, lockKey, err)
+		}
+		if !has {
+			return nil, fmt.Errorf("get locked resource %s:%d: record not found", lockType, lockKey)
+		}
+	}
+	return locked, nil
+}
+
+func UpdateLockedResource(e Engine, resource *LockedResource) error {
+	_, err := e.Table(resource).Cols("counter").Update(resource)
+	return err
+}
+
+func DeleteLockedResource(e Engine, resource *LockedResource) error {
+	_, err := e.Delete(resource)
+	return err
+}
+
+func TempLockResource(e Engine, lockType string, lockKey int64) (func() error, error) {
+	locked := &LockedResource{LockType: lockType, LockKey: lockKey}
+	// Temporary locked resources must not exist in the table
+	_, err := e.Insert(locked)
+	if err != nil {
+		return func() error {
+			return nil
+		},
+		fmt.Errorf("insert locked resource %s:%d: %v", lockType, lockKey, err)
+	}
+	return func() error {
+		_, err := e.Delete(locked)
+		return err
+	}, nil
+}
+
+func GetLockedResourceCtx(ctx DBContext, lockType string, lockKey int64) (*LockedResource, error) {
+	return GetLockedResource(ctx.e, lockType, lockKey)
+}
+
+func UpdateLockedResourceCtx(ctx DBContext, resource *LockedResource) error {
+	return UpdateLockedResource(ctx.e, resource)
+}
+
+func DeleteLockedResourceCtx(ctx DBContext, resource *LockedResource) error {
+	return DeleteLockedResource(ctx.e, resource)
+}
+
+func TempLockResourceCtx(ctx DBContext, lockType string, lockKey int64) (func() error, error) {
+	return TempLockResource(ctx.e, lockType, lockKey)
+}
+
diff --git a/models/locked_resource_test.go b/models/locked_resource_test.go
new file mode 100644
index 0000000000000..0c6e8f25ff8ce
--- /dev/null
+++ b/models/locked_resource_test.go
@@ -0,0 +1,76 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package models
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"xorm.io/xorm"
+)
+
+func TestLockedResource(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())
+
+	withSession := func(t *testing.T, f func(t *testing.T, sess *xorm.Session) bool) {
+		sess := x.NewSession()
+		defer sess.Close()
+		err := sess.Begin()
+		if !assert.NoError(t, err) {
+			return
+		}
+		if success := f(t, sess); !success {
+			return
+		}
+		err = sess.Commit()
+		assert.NoError(t, err)
+	}
+
+	// Get lock, increment counter value
+	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+		lck1, err := GetLockedResource(sess, "test-1",1)
+		if	!assert.NoError(t, err) || !assert.NotEmpty(t, lck1) || !assert.Equal(t, int64(0), lck1.Counter) {
+			return false
+		}
+		lck1.Counter++
+		err = UpdateLockedResource(sess, lck1)
+		return assert.NoError(t, err)
+	})
+
+	// Get lock, check counter value
+	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+		lck1, err := GetLockedResource(sess, "test-1",1)
+		return assert.NoError(t, err) && assert.NotEmpty(t, lck1) && assert.Equal(t, int64(1), lck1.Counter)
+	})
+
+	// Attempt temp lock on an existing key, expect error
+	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+		_, err := TempLockResource(sess, "test-1",1)
+		return assert.Error(t, err)
+	})
+
+	// Delete lock
+	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+		lck1, err := GetLockedResource(sess, "test-1",1)
+		if !assert.NoError(t, err) || !assert.NotEmpty(t, lck1) {
+			return false
+		}
+		return assert.NoError(t, DeleteLockedResource(sess, lck1))
+	})
+
+	// Attempt temp lock on an valid key, expect success
+	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+		// Must give error
+		releaseLock, err := TempLockResource(sess, "test-1",1)
+		if !assert.NoError(t, err) {
+			return false
+		}
+		return assert.NoError(t, releaseLock())
+	})
+
+	// Note: testing the validity of the locking mechanism (i.e. whether it actually locks)
+	// must be done in the integration tests, so all the supported databases are checked.
+}
+
diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index 226368b7f36fa..6244a116bb55c 100644
--- a/models/migrations/migrations.go
+++ b/models/migrations/migrations.go
@@ -304,6 +304,8 @@ var migrations = []Migration{
 	NewMigration("Add original informations for reactions", addReactionOriginals),
 	// v124 -> v125
 	NewMigration("Add columns to user and repository", addUserRepoMissingColumns),
+	// v125 -> v126
+	NewMigration("Add locked_resource table", addLockedResourceTable),
 }
 
 // Migrate database to current version
diff --git a/models/migrations/v125.go b/models/migrations/v125.go
new file mode 100644
index 0000000000000..7fad92e2f2130
--- /dev/null
+++ b/models/migrations/v125.go
@@ -0,0 +1,20 @@
+// Copyright 2020 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"xorm.io/xorm"
+)
+
+func addLockedResourceTable(x *xorm.Engine) error {
+
+	type LockedResource struct {
+		LockType	string      `xorm:"pk"`
+		LockKey		int64		`xorm:"pk"`
+		Counter		int64		`xorm:"NOT NULL DEFAULT 0"`
+	}
+	
+	return x.Sync2(new(LockedResource))
+}
diff --git a/models/models.go b/models/models.go
index 9eb174e200d4a..63f2866b1defa 100644
--- a/models/models.go
+++ b/models/models.go
@@ -114,6 +114,7 @@ func init() {
 		new(OAuth2AuthorizationCode),
 		new(OAuth2Grant),
 		new(Task),
+		new(LockedResource),
 	)
 
 	gonicNames := []string{"SSL", "UID"}

From 70a2a8df2d6930b66c1da562296a129bf478e6e5 Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Mon, 20 Jan 2020 22:16:25 -0300
Subject: [PATCH 02/17] Move to upsert strategy; all tests work

---
 integrations/locked_resource_test.go | 61 ++++++++++-----------
 models/locked_resource.go            | 82 ++++++++++++++++------------
 models/locked_resource_test.go       | 12 ++--
 models/migrations/v125.go            |  2 +-
 4 files changed, 80 insertions(+), 77 deletions(-)

diff --git a/integrations/locked_resource_test.go b/integrations/locked_resource_test.go
index c4f9707b08b4c..169e0a450c927 100644
--- a/integrations/locked_resource_test.go
+++ b/integrations/locked_resource_test.go
@@ -10,15 +10,19 @@ import (
 	"time"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/modules/log"
 
 	"github.com/stretchr/testify/assert"
 )
 
 const (
-	// Note: these values might require tuning
-	before = 500 * time.Millisecond
-	after = 1000 * time.Millisecond
-	tolerance = 200 * time.Millisecond
+	// The tests will fail if the waiter function takes less than
+	// blockerDelay minus tolerance to complete.
+	// Note: these values might require tuning in order to avoid
+	// false negatives.
+	waiterDelay = 100 * time.Millisecond
+	blockerDelay = 200 * time.Millisecond
+	tolerance = 50 * time.Millisecond	// Should be <= (blockerDelay-waiterDelay)/2
 )
 
 type waitResult struct {
@@ -35,37 +39,31 @@ func TestLockedResource(t *testing.T) {
 	// the more certain we are the second goroutine is waiting.
 
 	// This check **must** fail as we're not blocking anything
-	assert.Error(t, blockTest("no block", func(ctx models.DBContext) (func() error, error){
-		return func() error{
-			return nil
-		}, nil
+	assert.Error(t, blockTest("no block", func(ctx models.DBContext) error {
+		return nil
 	}))
 
 	models.AssertNotExistsBean(t, &models.LockedResource{LockType: "test-1", LockKey: 1})
 
 	// Test with creation (i.e. new lock type)
-	assert.NoError(t, blockTest("block-new", func(ctx models.DBContext) (func() error, error){
+	assert.NoError(t, blockTest("block-new", func(ctx models.DBContext) error {
 		_, err := models.GetLockedResourceCtx(ctx, "block-test-1", 1)
-		return func() error{
-			return nil
-		}, err
+		return err
 	}))
 
 	// Test without creation (i.e. lock type already exists)
-	assert.NoError(t, blockTest("block-existing", func(ctx models.DBContext) (func() error, error){
+	assert.NoError(t, blockTest("block-existing", func(ctx models.DBContext) error {
 		_, err := models.GetLockedResourceCtx(ctx, "block-test-1", 1)
-		return func() error{
-			return nil
-		}, err
+		return err
 	}))
 
 	// Test with temporary record
-	assert.NoError(t, blockTest("block-temp", func(ctx models.DBContext) (func() error, error){
+	assert.NoError(t, blockTest("block-temp", func(ctx models.DBContext) error {
 		return models.TempLockResourceCtx(ctx, "temp-1", 1)
 	}))
 }
 
-func blockTest(name string, f func(ctx models.DBContext) (func() error, error)) error {
+func blockTest(name string, f func(ctx models.DBContext) error) error {
 	cb := make(chan waitResult)
 	cw := make(chan waitResult)
 	ref := time.Now()
@@ -86,40 +84,39 @@ func blockTest(name string, f func(ctx models.DBContext) (func() error, error))
 		return resw.Err
 	}
 
-	if resw.Waited < after - tolerance {
+	if resw.Waited < blockerDelay - tolerance {
 		return fmt.Errorf("Waiter not blocked on %s; wait: %d ms, expected > %d ms",
-			name, resw.Waited.Milliseconds(), (after - tolerance).Milliseconds())
+			name, resw.Waited.Milliseconds(), (blockerDelay - tolerance).Milliseconds())
 	}
 
 	return nil
 }
 
-func blockTestFunc(name string, blocker bool, ref time.Time, f func(ctx models.DBContext) (func() error, error)) (wr waitResult) {
+func blockTestFunc(name string, blocker bool, ref time.Time, f func(ctx models.DBContext) error) (wr waitResult) {
 	if blocker {
 		name = fmt.Sprintf("blocker [%s]", name)
 	} else {
 		name = fmt.Sprintf("waiter [%s]", name)
 	}
 	err := models.WithTx(func(ctx models.DBContext) error {
-		fmt.Printf("Entering %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
+		log.Trace("Entering %s @%d", name, time.Now().Sub(ref).Milliseconds())
 		if !blocker {
-			fmt.Printf("Waiting on %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
-			time.Sleep(before)
-			fmt.Printf("Wait finished on %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
+			log.Trace("Waiting on %s @%d", name, time.Now().Sub(ref).Milliseconds())
+			time.Sleep(waiterDelay)
+			log.Trace("Wait finished on %s @%d", name, time.Now().Sub(ref).Milliseconds())
 		}
-		releaseLock, err := f(ctx)
-		if err != nil {
+		if err := f(ctx); err != nil {
 			return err
 		}
 		if blocker {
-			fmt.Printf("Waiting on %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
-			time.Sleep(after)
-			fmt.Printf("Wait finished on %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
+			log.Trace("Waiting on %s @%d", name, time.Now().Sub(ref).Milliseconds())
+			time.Sleep(blockerDelay)
+			log.Trace("Wait finished on %s @%d", name, time.Now().Sub(ref).Milliseconds())
 		} else {
 			wr.Waited = time.Now().Sub(ref)
 		}
-		fmt.Printf("Finishing %s @%d\n", name, time.Now().Sub(ref).Milliseconds())
-		return releaseLock()
+		log.Trace("Finishing %s @%d", name, time.Now().Sub(ref).Milliseconds())
+		return nil
 	})
 	if err != nil {
 		wr.Err = fmt.Errorf("error in %s: %v", name, err)
diff --git a/models/locked_resource.go b/models/locked_resource.go
index 88cec83f4c3ae..0e83835c8acfe 100644
--- a/models/locked_resource.go
+++ b/models/locked_resource.go
@@ -6,41 +6,30 @@ package models
 
 import (
 	"fmt"
+
+	"code.gitea.io/gitea/modules/setting"
 )
 
 type LockedResource struct {
-	LockType	string      `xorm:"pk"`
+	LockType	string      `xorm:"pk VARCHAR(30)"`
 	LockKey		int64		`xorm:"pk"`
 	Counter		int64		`xorm:"NOT NULL DEFAULT 0"`
 }
 
 func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResource, error) {
 	locked := &LockedResource{LockType: lockType, LockKey: lockKey}
-	// SQLite3 has no ForUpdate() clause and an UPSERT strategy has many
-	// problems and fallbacks; we perform a bogus update on the table
-	// which will lock the key in a safe way.
-	// Make sure to leave `counter` out of the update.
-	count, err := e.Table(locked).Cols("lock_type", "lock_key").Update(locked)
-	if err != nil {
-		return nil, fmt.Errorf("get locked resource %s:%d: %v", lockType, lockKey, err)
+
+	if err := upsertLockedResource(e, locked); err != nil {
+		return nil, fmt.Errorf("upsertLockedResource: %v", err)
 	}
-	if count == 0 {
-		// No record was found; since the key is now locked,
-		// it's safe to insert a record.
-		_, err = e.Insert(locked)
-		if err != nil {
-			return nil, fmt.Errorf("get locked resource %s:%d: %v", lockType, lockKey, err)
-		}
-	} else {
-		// Read back the record we've locked
-		has, err := e.Table(locked).Get(locked)
-		if err != nil {
-			return nil, fmt.Errorf("get locked resource %s:%d: %v", lockType, lockKey, err)
-		}
-		if !has {
-			return nil, fmt.Errorf("get locked resource %s:%d: record not found", lockType, lockKey)
-		}
+
+	// Read back the record we've created or locked to get the current Counter value
+	if has, err := e.Table(locked).Get(locked); err != nil {
+		return nil, fmt.Errorf("get locked resource %s:%d: %v", lockType, lockKey, err)
+	} else if !has {
+		return nil, fmt.Errorf("unexpected upsert fail  %s:%d", lockType, lockKey)
 	}
+	
 	return locked, nil
 }
 
@@ -54,20 +43,15 @@ func DeleteLockedResource(e Engine, resource *LockedResource) error {
 	return err
 }
 
-func TempLockResource(e Engine, lockType string, lockKey int64) (func() error, error) {
+func TempLockResource(e Engine, lockType string, lockKey int64) error {
 	locked := &LockedResource{LockType: lockType, LockKey: lockKey}
-	// Temporary locked resources must not exist in the table
+	// Temporary locked resources must not exist in the table.
+	// This allows us to use a simple INSERT to lock the key.
 	_, err := e.Insert(locked)
-	if err != nil {
-		return func() error {
-			return nil
-		},
-		fmt.Errorf("insert locked resource %s:%d: %v", lockType, lockKey, err)
+	if err == nil {
+		_, err = e.Delete(locked)
 	}
-	return func() error {
-		_, err := e.Delete(locked)
-		return err
-	}, nil
+	return err
 }
 
 func GetLockedResourceCtx(ctx DBContext, lockType string, lockKey int64) (*LockedResource, error) {
@@ -82,7 +66,33 @@ func DeleteLockedResourceCtx(ctx DBContext, resource *LockedResource) error {
 	return DeleteLockedResource(ctx.e, resource)
 }
 
-func TempLockResourceCtx(ctx DBContext, lockType string, lockKey int64) (func() error, error) {
+func TempLockResourceCtx(ctx DBContext, lockType string, lockKey int64) error {
 	return TempLockResource(ctx.e, lockType, lockKey)
 }
 
+func upsertLockedResource(e Engine, resource *LockedResource) (err error) {
+	// An atomic UPSERT operation (INSERT/UPDATE) is the only operation
+	// that ensures that the key is actually locked. 
+	switch {
+	case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL:
+		_, err = e.Exec("INSERT INTO locked_resource (lock_type, lock_key) "+
+			"VALUES (?,?) ON CONFLICT(lock_type, lock_key) DO UPDATE SET lock_key = ?",
+			resource.LockType, resource.LockKey, resource.LockKey);
+	case setting.Database.UseMySQL:
+		_, err = e.Exec("INSERT INTO locked_resource (lock_type, lock_key) "+
+			"VALUES (?,?) ON DUPLICATE KEY UPDATE lock_key = lock_key",
+			resource.LockType, resource.LockKey);
+	case setting.Database.UseMSSQL:
+		// https://weblogs.sqlteam.com/dang/2009/01/31/upsert-race-condition-with-merge/
+		_, err = e.Exec("MERGE locked_resource WITH (HOLDLOCK) as target "+
+			"USING (SELECT ? AS lock_type, ? AS lock_key) AS src "+
+			"ON src.lock_type = target.lock_type AND src.lock_key = target.lock_key "+
+			"WHEN MATCHED THEN UPDATE SET target.lock_key = target.lock_key "+
+			"WHEN NOT MATCHED THEN INSERT (lock_type, lock_key) "+
+			"VALUES (src.lock_type, src.lock_key);",
+			resource.LockType, resource.LockKey);
+	default:
+		return fmt.Errorf("database type not supported")
+	}
+	return
+}
\ No newline at end of file
diff --git a/models/locked_resource_test.go b/models/locked_resource_test.go
index 0c6e8f25ff8ce..e5b31c45d8992 100644
--- a/models/locked_resource_test.go
+++ b/models/locked_resource_test.go
@@ -47,7 +47,8 @@ func TestLockedResource(t *testing.T) {
 
 	// Attempt temp lock on an existing key, expect error
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		_, err := TempLockResource(sess, "test-1",1)
+		err := TempLockResource(sess, "test-1",1)
+		// Must give error
 		return assert.Error(t, err)
 	})
 
@@ -62,15 +63,10 @@ func TestLockedResource(t *testing.T) {
 
 	// Attempt temp lock on an valid key, expect success
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		// Must give error
-		releaseLock, err := TempLockResource(sess, "test-1",1)
-		if !assert.NoError(t, err) {
-			return false
-		}
-		return assert.NoError(t, releaseLock())
+		return assert.NoError(t, TempLockResource(sess, "test-1",1))
 	})
 
 	// Note: testing the validity of the locking mechanism (i.e. whether it actually locks)
-	// must be done in the integration tests, so all the supported databases are checked.
+	// is be done at the integration tests to ensure that all the supported databases are checked.
 }
 
diff --git a/models/migrations/v125.go b/models/migrations/v125.go
index 7fad92e2f2130..5968b33749df0 100644
--- a/models/migrations/v125.go
+++ b/models/migrations/v125.go
@@ -11,7 +11,7 @@ import (
 func addLockedResourceTable(x *xorm.Engine) error {
 
 	type LockedResource struct {
-		LockType	string      `xorm:"pk"`
+		LockType	string      `xorm:"pk VARCHAR(30)"`
 		LockKey		int64		`xorm:"pk"`
 		Counter		int64		`xorm:"NOT NULL DEFAULT 0"`
 	}

From d834fb1451dc8cb56f4c37f8b883adb08da1bb49 Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Tue, 21 Jan 2020 14:17:09 -0300
Subject: [PATCH 03/17] Use a LockedResource to numerate issues and prs

---
 integrations/locked_resource_test.go | 14 ++++-----
 models/issue.go                      | 45 +++++++++++-----------------
 models/locked_resource.go            | 18 +++++------
 models/locked_resource_test.go       | 13 ++++----
 models/migrations/v125.go            | 37 +++++++++++++++++++----
 models/pull.go                       | 18 -----------
 6 files changed, 71 insertions(+), 74 deletions(-)

diff --git a/integrations/locked_resource_test.go b/integrations/locked_resource_test.go
index 169e0a450c927..fd975b5915654 100644
--- a/integrations/locked_resource_test.go
+++ b/integrations/locked_resource_test.go
@@ -20,14 +20,14 @@ const (
 	// blockerDelay minus tolerance to complete.
 	// Note: these values might require tuning in order to avoid
 	// false negatives.
-	waiterDelay = 100 * time.Millisecond
+	waiterDelay  = 100 * time.Millisecond
 	blockerDelay = 200 * time.Millisecond
-	tolerance = 50 * time.Millisecond	// Should be <= (blockerDelay-waiterDelay)/2
+	tolerance    = 50 * time.Millisecond // Should be <= (blockerDelay-waiterDelay)/2
 )
 
 type waitResult struct {
-	Waited	time.Duration
-	Err		error
+	Waited time.Duration
+	Err    error
 }
 
 func TestLockedResource(t *testing.T) {
@@ -75,8 +75,8 @@ func blockTest(name string, f func(ctx models.DBContext) error) error {
 		cw <- blockTestFunc(name, false, ref, f)
 	}()
 
-	resb := <- cb
-	resw := <- cw
+	resb := <-cb
+	resw := <-cw
 	if resb.Err != nil {
 		return resb.Err
 	}
@@ -84,7 +84,7 @@ func blockTest(name string, f func(ctx models.DBContext) error) error {
 		return resw.Err
 	}
 
-	if resw.Waited < blockerDelay - tolerance {
+	if resw.Waited < blockerDelay-tolerance {
 		return fmt.Errorf("Waiter not blocked on %s; wait: %d ms, expected > %d ms",
 			name, resw.Waited.Milliseconds(), (blockerDelay - tolerance).Milliseconds())
 	}
diff --git a/models/issue.go b/models/issue.go
index 7976366058c5d..e85920de3bdc0 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -75,7 +75,10 @@ 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
+
+// IssueLockedEnumerator is the name of the locked_resource used to
+// numerate issues in a repository.
+const IssueLockedEnumerator = "repository-index"
 
 func init() {
 	issueTasksPat = regexp.MustCompile(issueTasksRegexpStr)
@@ -898,19 +901,23 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 	}
 
 	// Milestone validation should happen before insert actual object.
-	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)
+	// Obtain the next issue number for this repository, which will be locked
+	// and reserved for the remaining of the transaction. Should the transaction
+	// be rolled back, the previous value will be restored.
+	locked, err := GetLockedResource(e, IssueLockedEnumerator, opts.Issue.RepoID)
 	if err != nil {
-		return err
+		return fmt.Errorf("GetLockedResource(%s)", IssueLockedEnumerator)
+	}
+	locked.Counter++
+	if err := UpdateLockedResource(e, locked); err != nil {
+		return fmt.Errorf("UpdateLockedResource(%s)", IssueLockedEnumerator)
 	}
+	opts.Issue.Index = locked.Counter
 
-	// Patch Index with the value calculated by the database
-	opts.Issue.Index = inserted.Index
+	if _, err = e.Insert(opts.Issue); err != nil {
+		return err
+	}
 
 	if opts.Issue.MilestoneID > 0 {
 		if _, err = e.Exec("UPDATE `milestone` SET num_issues=num_issues+1 WHERE id=?", opts.Issue.MilestoneID); err != nil {
@@ -988,24 +995,6 @@ 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, 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, 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, uuids []string) (err error) {
 	sess := x.NewSession()
 	defer sess.Close()
 	if err = sess.Begin(); err != nil {
diff --git a/models/locked_resource.go b/models/locked_resource.go
index 0e83835c8acfe..1b5705d510950 100644
--- a/models/locked_resource.go
+++ b/models/locked_resource.go
@@ -11,9 +11,9 @@ import (
 )
 
 type LockedResource struct {
-	LockType	string      `xorm:"pk VARCHAR(30)"`
-	LockKey		int64		`xorm:"pk"`
-	Counter		int64		`xorm:"NOT NULL DEFAULT 0"`
+	LockType string `xorm:"pk VARCHAR(30)"`
+	LockKey  int64  `xorm:"pk"`
+	Counter  int64  `xorm:"NOT NULL DEFAULT 0"`
 }
 
 func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResource, error) {
@@ -29,7 +29,7 @@ func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResourc
 	} else if !has {
 		return nil, fmt.Errorf("unexpected upsert fail  %s:%d", lockType, lockKey)
 	}
-	
+
 	return locked, nil
 }
 
@@ -72,16 +72,16 @@ func TempLockResourceCtx(ctx DBContext, lockType string, lockKey int64) error {
 
 func upsertLockedResource(e Engine, resource *LockedResource) (err error) {
 	// An atomic UPSERT operation (INSERT/UPDATE) is the only operation
-	// that ensures that the key is actually locked. 
+	// that ensures that the key is actually locked.
 	switch {
 	case setting.Database.UseSQLite3 || setting.Database.UsePostgreSQL:
 		_, err = e.Exec("INSERT INTO locked_resource (lock_type, lock_key) "+
 			"VALUES (?,?) ON CONFLICT(lock_type, lock_key) DO UPDATE SET lock_key = ?",
-			resource.LockType, resource.LockKey, resource.LockKey);
+			resource.LockType, resource.LockKey, resource.LockKey)
 	case setting.Database.UseMySQL:
 		_, err = e.Exec("INSERT INTO locked_resource (lock_type, lock_key) "+
 			"VALUES (?,?) ON DUPLICATE KEY UPDATE lock_key = lock_key",
-			resource.LockType, resource.LockKey);
+			resource.LockType, resource.LockKey)
 	case setting.Database.UseMSSQL:
 		// https://weblogs.sqlteam.com/dang/2009/01/31/upsert-race-condition-with-merge/
 		_, err = e.Exec("MERGE locked_resource WITH (HOLDLOCK) as target "+
@@ -90,9 +90,9 @@ func upsertLockedResource(e Engine, resource *LockedResource) (err error) {
 			"WHEN MATCHED THEN UPDATE SET target.lock_key = target.lock_key "+
 			"WHEN NOT MATCHED THEN INSERT (lock_type, lock_key) "+
 			"VALUES (src.lock_type, src.lock_key);",
-			resource.LockType, resource.LockKey);
+			resource.LockType, resource.LockKey)
 	default:
 		return fmt.Errorf("database type not supported")
 	}
 	return
-}
\ No newline at end of file
+}
diff --git a/models/locked_resource_test.go b/models/locked_resource_test.go
index e5b31c45d8992..bfc0f443008b7 100644
--- a/models/locked_resource_test.go
+++ b/models/locked_resource_test.go
@@ -30,8 +30,8 @@ func TestLockedResource(t *testing.T) {
 
 	// Get lock, increment counter value
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		lck1, err := GetLockedResource(sess, "test-1",1)
-		if	!assert.NoError(t, err) || !assert.NotEmpty(t, lck1) || !assert.Equal(t, int64(0), lck1.Counter) {
+		lck1, err := GetLockedResource(sess, "test-1", 1)
+		if !assert.NoError(t, err) || !assert.NotEmpty(t, lck1) || !assert.Equal(t, int64(0), lck1.Counter) {
 			return false
 		}
 		lck1.Counter++
@@ -41,20 +41,20 @@ func TestLockedResource(t *testing.T) {
 
 	// Get lock, check counter value
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		lck1, err := GetLockedResource(sess, "test-1",1)
+		lck1, err := GetLockedResource(sess, "test-1", 1)
 		return assert.NoError(t, err) && assert.NotEmpty(t, lck1) && assert.Equal(t, int64(1), lck1.Counter)
 	})
 
 	// Attempt temp lock on an existing key, expect error
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		err := TempLockResource(sess, "test-1",1)
+		err := TempLockResource(sess, "test-1", 1)
 		// Must give error
 		return assert.Error(t, err)
 	})
 
 	// Delete lock
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		lck1, err := GetLockedResource(sess, "test-1",1)
+		lck1, err := GetLockedResource(sess, "test-1", 1)
 		if !assert.NoError(t, err) || !assert.NotEmpty(t, lck1) {
 			return false
 		}
@@ -63,10 +63,9 @@ func TestLockedResource(t *testing.T) {
 
 	// Attempt temp lock on an valid key, expect success
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		return assert.NoError(t, TempLockResource(sess, "test-1",1))
+		return assert.NoError(t, TempLockResource(sess, "test-1", 1))
 	})
 
 	// Note: testing the validity of the locking mechanism (i.e. whether it actually locks)
 	// is be done at the integration tests to ensure that all the supported databases are checked.
 }
-
diff --git a/models/migrations/v125.go b/models/migrations/v125.go
index 5968b33749df0..4e0706f161537 100644
--- a/models/migrations/v125.go
+++ b/models/migrations/v125.go
@@ -5,16 +5,43 @@
 package migrations
 
 import (
+	"code.gitea.io/gitea/models"
+
 	"xorm.io/xorm"
 )
 
 func addLockedResourceTable(x *xorm.Engine) error {
 
 	type LockedResource struct {
-		LockType	string      `xorm:"pk VARCHAR(30)"`
-		LockKey		int64		`xorm:"pk"`
-		Counter		int64		`xorm:"NOT NULL DEFAULT 0"`
+		LockType string `xorm:"pk VARCHAR(30)"`
+		LockKey  int64  `xorm:"pk"`
+		Counter  int64  `xorm:"NOT NULL DEFAULT 0"`
+	}
+
+	sess := x.NewSession()
+	defer sess.Close()
+
+	if err := sess.Begin(); err != nil {
+		return err
+	}
+
+	if err := sess.Sync2(new(LockedResource)); err != nil {
+		return err
 	}
-	
-	return x.Sync2(new(LockedResource))
+
+	// Remove data we're goint to rebuild
+	if _, err := sess.Delete(&LockedResource{LockType: models.IssueLockedEnumerator}); err != nil {
+		return err
+	}
+
+	// Create current data for all repositories with issues and PRs
+	if _, err := sess.Exec("INSERT INTO locked_resource (lock_type, lock_key, counter) "+
+		"SELECT ?, max_data.repo_id, max_data.max_index "+
+		"FROM ( SELECT issue.repo_id AS repo_id, max(issue.index) AS max_index "+
+		"FROM issue GROUP BY issue.repo_id) AS max_data",
+		models.IssueLockedEnumerator); err != nil {
+		return err
+	}
+
+	return sess.Commit()
 }
diff --git a/models/pull.go b/models/pull.go
index 3ef631852ea98..2fd9741107270 100644
--- a/models/pull.go
+++ b/models/pull.go
@@ -531,24 +531,6 @@ func (pr *PullRequest) SetMerged() (err error) {
 
 // NewPullRequest creates new pull request with labels for repository.
 func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []string, pr *PullRequest) (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); 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) (err error) {
 	sess := x.NewSession()
 	defer sess.Close()
 	if err = sess.Begin(); err != nil {

From 58ac90199e561206c472c6721c248346a0033e31 Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Tue, 21 Jan 2020 14:47:47 -0300
Subject: [PATCH 04/17] Fix tests and reserved keyword

---
 models/issue_xref_test.go | 7 +++++--
 models/migrations/v125.go | 2 +-
 models/test_fixtures.go   | 7 +++++++
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go
index 936d1124be475..ac4ba855fbf90 100644
--- a/models/issue_xref_test.go
+++ b/models/issue_xref_test.go
@@ -130,9 +130,12 @@ func testCreateIssue(t *testing.T, repo, doer int64, title, content string, ispu
 	sess := x.NewSession()
 	defer sess.Close()
 	assert.NoError(t, sess.Begin())
-	_, err := sess.SetExpr("`index`", "coalesce(MAX(`index`),0)+1").Where("repo_id=?", repo).Insert(i)
+	lock, err := GetLockedResource(sess, IssueLockedEnumerator, repo)
 	assert.NoError(t, err)
-	i, err = getIssueByID(sess, i.ID)
+	lock.Counter++
+	assert.NoError(t, UpdateLockedResource(sess, lock))
+	i.Index = lock.Counter
+	_, err = sess.Insert(i)
 	assert.NoError(t, err)
 	assert.NoError(t, i.addCrossReferences(sess, d, false))
 	assert.NoError(t, sess.Commit())
diff --git a/models/migrations/v125.go b/models/migrations/v125.go
index 4e0706f161537..70ecb5f05fbe8 100644
--- a/models/migrations/v125.go
+++ b/models/migrations/v125.go
@@ -37,7 +37,7 @@ func addLockedResourceTable(x *xorm.Engine) error {
 	// Create current data for all repositories with issues and PRs
 	if _, err := sess.Exec("INSERT INTO locked_resource (lock_type, lock_key, counter) "+
 		"SELECT ?, max_data.repo_id, max_data.max_index "+
-		"FROM ( SELECT issue.repo_id AS repo_id, max(issue.index) AS max_index "+
+		"FROM ( SELECT issue.repo_id AS repo_id, max(issue.`index`) AS max_index "+
 		"FROM issue GROUP BY issue.repo_id) AS max_data",
 		models.IssueLockedEnumerator); err != nil {
 		return err
diff --git a/models/test_fixtures.go b/models/test_fixtures.go
index fe6a790b0d48f..d6318f75ed985 100644
--- a/models/test_fixtures.go
+++ b/models/test_fixtures.go
@@ -67,5 +67,12 @@ func LoadFixtures() error {
 			}
 		}
 	}
+	// Finally, we must build the last issue index used for each repositories
+	_, err = x.Exec("INSERT INTO locked_resource (lock_type, lock_key, counter) "+
+		"SELECT ?, max_data.repo_id, max_data.max_index "+
+		"FROM ( SELECT issue.repo_id AS repo_id, max(issue.`index`) AS max_index "+
+		"FROM issue GROUP BY issue.repo_id) AS max_data",
+		IssueLockedEnumerator)
+
 	return err
 }

From aa3797c262d32b8b0793f7ae29d5b3f18eb66a9f Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Tue, 21 Jan 2020 15:24:25 -0300
Subject: [PATCH 05/17] Fix unit tests

---
 models/issue_test.go    | 11 ++++++-----
 models/test_fixtures.go |  5 ++++-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/models/issue_test.go b/models/issue_test.go
index d65345a508a1a..0c80d4646e454 100644
--- a/models/issue_test.go
+++ b/models/issue_test.go
@@ -286,7 +286,7 @@ func TestIssue_SearchIssueIDsByKeyword(t *testing.T) {
 	assert.EqualValues(t, []int64{1}, ids)
 }
 
-func testInsertIssue(t *testing.T, title, content string) {
+func testInsertIssue(t *testing.T, title, content string, idx int64) {
 	repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
 	user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
 
@@ -305,8 +305,7 @@ func testInsertIssue(t *testing.T, title, content string) {
 	assert.True(t, has)
 	assert.EqualValues(t, issue.Title, newIssue.Title)
 	assert.EqualValues(t, issue.Content, newIssue.Content)
-	// there are 5 issues and max index is 5 on repository 1, so this one should 6
-	assert.EqualValues(t, 6, newIssue.Index)
+	assert.EqualValues(t, idx, newIssue.Index)
 
 	_, err = x.ID(issue.ID).Delete(new(Issue))
 	assert.NoError(t, err)
@@ -315,8 +314,10 @@ func testInsertIssue(t *testing.T, title, content string) {
 func TestIssue_InsertIssue(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 
-	testInsertIssue(t, "my issue1", "special issue's comments?")
-	testInsertIssue(t, `my issue2, this is my son's love \n \r \ `, "special issue's '' comments?")
+	// there are 5 issues and max index is 5 on repository 1, so this one should be 6
+	testInsertIssue(t, "my issue1", "special issue's comments?", 6)
+	// deleting an issue should not let a new issue reuse its index number; this one should be 7
+	testInsertIssue(t, `my issue2, this is my son's love \n \r \ `, "special issue's '' comments?", 7)
 }
 
 func TestIssue_ResolveMentions(t *testing.T) {
diff --git a/models/test_fixtures.go b/models/test_fixtures.go
index d6318f75ed985..a844a4798d6a2 100644
--- a/models/test_fixtures.go
+++ b/models/test_fixtures.go
@@ -67,7 +67,10 @@ func LoadFixtures() error {
 			}
 		}
 	}
-	// Finally, we must build the last issue index used for each repositories
+	// Finally, we must rebuild the last issue index used for each repositories
+	if _, err := x.Delete(&LockedResource{LockType: IssueLockedEnumerator}); err != nil {
+		return err
+	}
 	_, err = x.Exec("INSERT INTO locked_resource (lock_type, lock_key, counter) "+
 		"SELECT ?, max_data.repo_id, max_data.max_index "+
 		"FROM ( SELECT issue.repo_id AS repo_id, max(issue.`index`) AS max_index "+

From 95db48a8c2b6babb4e29c991726cee5ad818444f Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Wed, 22 Jan 2020 03:47:57 -0300
Subject: [PATCH 06/17] Fix export comments

---
 models/locked_resource.go | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/models/locked_resource.go b/models/locked_resource.go
index 1b5705d510950..4d19ae7190dd3 100644
--- a/models/locked_resource.go
+++ b/models/locked_resource.go
@@ -10,12 +10,15 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 )
 
+// LockedResource represents the locking key for a pessimistic
+// lock that can hold a counter
 type LockedResource struct {
 	LockType string `xorm:"pk VARCHAR(30)"`
 	LockKey  int64  `xorm:"pk"`
 	Counter  int64  `xorm:"NOT NULL DEFAULT 0"`
 }
 
+// GetLockedResource gets or creates a pessimistic lock on the given type and key
 func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResource, error) {
 	locked := &LockedResource{LockType: lockType, LockKey: lockKey}
 
@@ -33,16 +36,19 @@ func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResourc
 	return locked, nil
 }
 
+// UpdateLockedResource updates the value of the counter of a locked resource
 func UpdateLockedResource(e Engine, resource *LockedResource) error {
 	_, err := e.Table(resource).Cols("counter").Update(resource)
 	return err
 }
 
+// DeleteLockedResource deletes a locked resource
 func DeleteLockedResource(e Engine, resource *LockedResource) error {
 	_, err := e.Delete(resource)
 	return err
 }
 
+// TempLockResource locks the given key but does not leave a permanent record
 func TempLockResource(e Engine, lockType string, lockKey int64) error {
 	locked := &LockedResource{LockType: lockType, LockKey: lockKey}
 	// Temporary locked resources must not exist in the table.
@@ -54,22 +60,28 @@ func TempLockResource(e Engine, lockType string, lockKey int64) error {
 	return err
 }
 
+// GetLockedResourceCtx gets or creates a pessimistic lock on the given type and key
 func GetLockedResourceCtx(ctx DBContext, lockType string, lockKey int64) (*LockedResource, error) {
 	return GetLockedResource(ctx.e, lockType, lockKey)
 }
 
+// UpdateLockedResourceCtx updates the value of the counter of a locked resource
 func UpdateLockedResourceCtx(ctx DBContext, resource *LockedResource) error {
 	return UpdateLockedResource(ctx.e, resource)
 }
 
+// DeleteLockedResourceCtx deletes a locked resource
 func DeleteLockedResourceCtx(ctx DBContext, resource *LockedResource) error {
 	return DeleteLockedResource(ctx.e, resource)
 }
 
+// TempLockResourceCtx locks the given key but does not leave a permanent record
 func TempLockResourceCtx(ctx DBContext, lockType string, lockKey int64) error {
 	return TempLockResource(ctx.e, lockType, lockKey)
 }
 
+// upsertLockedResource will create or lock the given key in the database.
+// the function will not return until it acquires the lock or receives an error.
 func upsertLockedResource(e Engine, resource *LockedResource) (err error) {
 	// An atomic UPSERT operation (INSERT/UPDATE) is the only operation
 	// that ensures that the key is actually locked.

From d8ad17445ee3a1cc1f16646a7881ffedf1d50e5b Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Wed, 22 Jan 2020 11:07:50 -0300
Subject: [PATCH 07/17] A little refactoring and better function naming

---
 integrations/locked_resource_test.go |  2 +-
 models/issue.go                      | 10 ++---
 models/issue_xref_test.go            |  8 ++--
 models/locked_resource.go            | 61 ++++++++++++++++------------
 models/locked_resource_test.go       | 34 +++++++++++-----
 5 files changed, 67 insertions(+), 48 deletions(-)

diff --git a/integrations/locked_resource_test.go b/integrations/locked_resource_test.go
index fd975b5915654..d95ce16038706 100644
--- a/integrations/locked_resource_test.go
+++ b/integrations/locked_resource_test.go
@@ -59,7 +59,7 @@ func TestLockedResource(t *testing.T) {
 
 	// Test with temporary record
 	assert.NoError(t, blockTest("block-temp", func(ctx models.DBContext) error {
-		return models.TempLockResourceCtx(ctx, "temp-1", 1)
+		return models.TemporarilyLockResourceKeyCtx(ctx, "temp-1", 1)
 	}))
 }
 
diff --git a/models/issue.go b/models/issue.go
index e85920de3bdc0..75cb26c3e7711 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -905,15 +905,15 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 	// Obtain the next issue number for this repository, which will be locked
 	// and reserved for the remaining of the transaction. Should the transaction
 	// be rolled back, the previous value will be restored.
-	locked, err := GetLockedResource(e, IssueLockedEnumerator, opts.Issue.RepoID)
+	idxresource, err := GetLockedResource(e, IssueLockedEnumerator, opts.Issue.RepoID)
 	if err != nil {
 		return fmt.Errorf("GetLockedResource(%s)", IssueLockedEnumerator)
 	}
-	locked.Counter++
-	if err := UpdateLockedResource(e, locked); err != nil {
-		return fmt.Errorf("UpdateLockedResource(%s)", IssueLockedEnumerator)
+	idxresource.Counter++
+	if err := idxresource.UpdateValue(); err != nil {
+		return fmt.Errorf("locked.UpdateValue(%s)", IssueLockedEnumerator)
 	}
-	opts.Issue.Index = locked.Counter
+	opts.Issue.Index = idxresource.Counter
 
 	if _, err = e.Insert(opts.Issue); err != nil {
 		return err
diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go
index ac4ba855fbf90..1b6a80586bb0c 100644
--- a/models/issue_xref_test.go
+++ b/models/issue_xref_test.go
@@ -130,11 +130,11 @@ func testCreateIssue(t *testing.T, repo, doer int64, title, content string, ispu
 	sess := x.NewSession()
 	defer sess.Close()
 	assert.NoError(t, sess.Begin())
-	lock, err := GetLockedResource(sess, IssueLockedEnumerator, repo)
+	idxresource, err := GetLockedResource(sess, IssueLockedEnumerator, repo)
 	assert.NoError(t, err)
-	lock.Counter++
-	assert.NoError(t, UpdateLockedResource(sess, lock))
-	i.Index = lock.Counter
+	idxresource.Counter++
+	assert.NoError(t, idxresource.UpdateValue())
+	i.Index = idxresource.Counter
 	_, err = sess.Insert(i)
 	assert.NoError(t, err)
 	assert.NoError(t, i.addCrossReferences(sess, d, false))
diff --git a/models/locked_resource.go b/models/locked_resource.go
index 4d19ae7190dd3..4e4ded21a44a2 100644
--- a/models/locked_resource.go
+++ b/models/locked_resource.go
@@ -16,46 +16,58 @@ type LockedResource struct {
 	LockType string `xorm:"pk VARCHAR(30)"`
 	LockKey  int64  `xorm:"pk"`
 	Counter  int64  `xorm:"NOT NULL DEFAULT 0"`
+
+	engine Engine `xorm:"-"`
 }
 
 // GetLockedResource gets or creates a pessimistic lock on the given type and key
 func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResource, error) {
-	locked := &LockedResource{LockType: lockType, LockKey: lockKey}
+	resource := &LockedResource{LockType: lockType, LockKey: lockKey}
 
-	if err := upsertLockedResource(e, locked); err != nil {
+	if err := upsertLockedResource(e, resource); err != nil {
 		return nil, fmt.Errorf("upsertLockedResource: %v", err)
 	}
 
 	// Read back the record we've created or locked to get the current Counter value
-	if has, err := e.Table(locked).Get(locked); err != nil {
+	if has, err := e.Table(resource).Get(resource); err != nil {
 		return nil, fmt.Errorf("get locked resource %s:%d: %v", lockType, lockKey, err)
 	} else if !has {
 		return nil, fmt.Errorf("unexpected upsert fail  %s:%d", lockType, lockKey)
 	}
 
-	return locked, nil
+	// Once active, the locked resource is tied to a specific session
+	resource.engine = e
+
+	return resource, nil
 }
 
-// UpdateLockedResource updates the value of the counter of a locked resource
-func UpdateLockedResource(e Engine, resource *LockedResource) error {
-	_, err := e.Table(resource).Cols("counter").Update(resource)
+// UpdateValue updates the value of the counter of a locked resource
+func (r *LockedResource) UpdateValue() error {
+	_, err := r.engine.Table(r).Cols("counter").Update(r)
 	return err
 }
 
-// DeleteLockedResource deletes a locked resource
-func DeleteLockedResource(e Engine, resource *LockedResource) error {
-	_, err := e.Delete(resource)
+// Delete deletes the locked resource from the database,
+// but the key remains locked until the end of the transaction
+func (r *LockedResource) Delete() error {
+	_, err := r.engine.Delete(r)
 	return err
 }
 
-// TempLockResource locks the given key but does not leave a permanent record
-func TempLockResource(e Engine, lockType string, lockKey int64) error {
-	locked := &LockedResource{LockType: lockType, LockKey: lockKey}
-	// Temporary locked resources must not exist in the table.
+// DeleteLockedResourceKey deletes a locked resource by key
+func DeleteLockedResourceKey(e Engine, lockType string, lockKey int64) error {
+	_, err := e.Delete(&LockedResource{LockType: lockType, LockKey: lockKey})
+	return err
+}
+
+// TemporarilyLockResourceKey locks the given key but does not leave a permanent record
+func TemporarilyLockResourceKey(e Engine, lockType string, lockKey int64) error {
+	resource := &LockedResource{LockType: lockType, LockKey: lockKey}
+	// Temporary locked resources should not exist in the table.
 	// This allows us to use a simple INSERT to lock the key.
-	_, err := e.Insert(locked)
+	_, err := e.Insert(resource)
 	if err == nil {
-		_, err = e.Delete(locked)
+		_, err = e.Delete(resource)
 	}
 	return err
 }
@@ -65,19 +77,14 @@ func GetLockedResourceCtx(ctx DBContext, lockType string, lockKey int64) (*Locke
 	return GetLockedResource(ctx.e, lockType, lockKey)
 }
 
-// UpdateLockedResourceCtx updates the value of the counter of a locked resource
-func UpdateLockedResourceCtx(ctx DBContext, resource *LockedResource) error {
-	return UpdateLockedResource(ctx.e, resource)
-}
-
-// DeleteLockedResourceCtx deletes a locked resource
-func DeleteLockedResourceCtx(ctx DBContext, resource *LockedResource) error {
-	return DeleteLockedResource(ctx.e, resource)
+// DeleteLockedResourceKeyCtx deletes a locked resource by key
+func DeleteLockedResourceKeyCtx(ctx DBContext, lockType string, lockKey int64) error {
+	return DeleteLockedResourceKey(ctx.e, lockType, lockKey)
 }
 
-// TempLockResourceCtx locks the given key but does not leave a permanent record
-func TempLockResourceCtx(ctx DBContext, lockType string, lockKey int64) error {
-	return TempLockResource(ctx.e, lockType, lockKey)
+// TemporarilyLockResourceKeyCtx locks the given key but does not leave a permanent record
+func TemporarilyLockResourceKeyCtx(ctx DBContext, lockType string, lockKey int64) error {
+	return TemporarilyLockResourceKey(ctx.e, lockType, lockKey)
 }
 
 // upsertLockedResource will create or lock the given key in the database.
diff --git a/models/locked_resource_test.go b/models/locked_resource_test.go
index bfc0f443008b7..b74bc4e99f875 100644
--- a/models/locked_resource_test.go
+++ b/models/locked_resource_test.go
@@ -30,40 +30,52 @@ func TestLockedResource(t *testing.T) {
 
 	// Get lock, increment counter value
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		lck1, err := GetLockedResource(sess, "test-1", 1)
-		if !assert.NoError(t, err) || !assert.NotEmpty(t, lck1) || !assert.Equal(t, int64(0), lck1.Counter) {
+		resource, err := GetLockedResource(sess, "test-1", 1)
+		if !assert.NoError(t, err) || !assert.NotEmpty(t, resource) || !assert.Equal(t, int64(0), resource.Counter) {
 			return false
 		}
-		lck1.Counter++
-		err = UpdateLockedResource(sess, lck1)
+		resource.Counter++
+		err = resource.UpdateValue()
 		return assert.NoError(t, err)
 	})
 
 	// Get lock, check counter value
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		lck1, err := GetLockedResource(sess, "test-1", 1)
-		return assert.NoError(t, err) && assert.NotEmpty(t, lck1) && assert.Equal(t, int64(1), lck1.Counter)
+		resource, err := GetLockedResource(sess, "test-1", 1)
+		return assert.NoError(t, err) && assert.NotEmpty(t, resource) && assert.Equal(t, int64(1), resource.Counter)
 	})
 
 	// Attempt temp lock on an existing key, expect error
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		err := TempLockResource(sess, "test-1", 1)
+		err := TemporarilyLockResourceKey(sess, "test-1", 1)
 		// Must give error
 		return assert.Error(t, err)
 	})
 
 	// Delete lock
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		lck1, err := GetLockedResource(sess, "test-1", 1)
-		if !assert.NoError(t, err) || !assert.NotEmpty(t, lck1) {
+		resource, err := GetLockedResource(sess, "test-1", 1)
+		if !assert.NoError(t, err) || !assert.NotEmpty(t, resource) {
 			return false
 		}
-		return assert.NoError(t, DeleteLockedResource(sess, lck1))
+		return assert.NoError(t, resource.Delete())
 	})
+	AssertNotExistsBean(t, &LockedResource{LockType: "test-1", LockKey: 1})
+
+	// Get lock, then delete by key
+	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+		resource, err := GetLockedResource(sess, "test-1", 2)
+		return assert.NoError(t, err) && assert.NotEmpty(t, resource)
+	})
+	AssertExistsAndLoadBean(t, &LockedResource{LockType: "test-1", LockKey: 2})
+	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+		return assert.NoError(t, DeleteLockedResourceKey(sess, "test-1", 2))
+	})
+	AssertNotExistsBean(t, &LockedResource{LockType: "test-1", LockKey: 2})
 
 	// Attempt temp lock on an valid key, expect success
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
-		return assert.NoError(t, TempLockResource(sess, "test-1", 1))
+		return assert.NoError(t, TemporarilyLockResourceKey(sess, "test-1", 1))
 	})
 
 	// Note: testing the validity of the locking mechanism (i.e. whether it actually locks)

From e747a2c5b0a148948e72738ab0154a06d9d2f445 Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Wed, 22 Jan 2020 12:27:02 -0300
Subject: [PATCH 08/17] Support LockType == "" and LockKey == 0

---
 models/locked_resource.go      | 18 +++++++++++-------
 models/locked_resource_test.go | 18 ++++++++++++++++++
 2 files changed, 29 insertions(+), 7 deletions(-)

diff --git a/models/locked_resource.go b/models/locked_resource.go
index 4e4ded21a44a2..68592b5a5a7fa 100644
--- a/models/locked_resource.go
+++ b/models/locked_resource.go
@@ -29,7 +29,8 @@ func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResourc
 	}
 
 	// Read back the record we've created or locked to get the current Counter value
-	if has, err := e.Table(resource).Get(resource); err != nil {
+	if has, err := e.Table(resource).NoCache().NoAutoCondition().AllCols().
+		Where("lock_type = ? AND lock_key = ?", lockType, lockKey).Get(resource); err != nil {
 		return nil, fmt.Errorf("get locked resource %s:%d: %v", lockType, lockKey, err)
 	} else if !has {
 		return nil, fmt.Errorf("unexpected upsert fail  %s:%d", lockType, lockKey)
@@ -43,31 +44,34 @@ func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResourc
 
 // UpdateValue updates the value of the counter of a locked resource
 func (r *LockedResource) UpdateValue() error {
-	_, err := r.engine.Table(r).Cols("counter").Update(r)
+	// Bypass ORM to support lock_type == "" and lock_key == 0
+	_, err := r.engine.Exec("UPDATE locked_resource SET counter = ? WHERE lock_type = ? AND lock_key = ?",
+		r.Counter, r.LockType, r.LockKey)
 	return err
 }
 
 // Delete deletes the locked resource from the database,
 // but the key remains locked until the end of the transaction
 func (r *LockedResource) Delete() error {
-	_, err := r.engine.Delete(r)
+	// Bypass ORM to support lock_type == "" and lock_key == 0
+	_, err := r.engine.Exec("DELETE FROM locked_resource WHERE lock_type = ? AND lock_key = ?", r.LockType, r.LockKey)
 	return err
 }
 
 // DeleteLockedResourceKey deletes a locked resource by key
 func DeleteLockedResourceKey(e Engine, lockType string, lockKey int64) error {
-	_, err := e.Delete(&LockedResource{LockType: lockType, LockKey: lockKey})
+	// Bypass ORM to support lock_type == "" and lock_key == 0
+	_, err := e.Exec("DELETE FROM locked_resource WHERE lock_type = ? AND lock_key = ?", lockType, lockKey)
 	return err
 }
 
 // TemporarilyLockResourceKey locks the given key but does not leave a permanent record
 func TemporarilyLockResourceKey(e Engine, lockType string, lockKey int64) error {
-	resource := &LockedResource{LockType: lockType, LockKey: lockKey}
 	// Temporary locked resources should not exist in the table.
 	// This allows us to use a simple INSERT to lock the key.
-	_, err := e.Insert(resource)
+	_, err := e.Exec("INSERT INTO locked_resource (lock_type, lock_key) VALUES (?, ?)", lockType, lockKey)
 	if err == nil {
-		_, err = e.Delete(resource)
+		_, err = e.Exec("DELETE FROM locked_resource WHERE lock_type = ? AND lock_key = ?", lockType, lockKey)
 	}
 	return err
 }
diff --git a/models/locked_resource_test.go b/models/locked_resource_test.go
index b74bc4e99f875..9a4bb40c3367d 100644
--- a/models/locked_resource_test.go
+++ b/models/locked_resource_test.go
@@ -45,6 +45,24 @@ func TestLockedResource(t *testing.T) {
 		return assert.NoError(t, err) && assert.NotEmpty(t, resource) && assert.Equal(t, int64(1), resource.Counter)
 	})
 
+	// Make sure LockKey == 0 is supported and we're not
+	// affecting other records
+	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+		resource, err := GetLockedResource(sess, "test-1", 0)
+		if !assert.NoError(t, err) || !assert.NotEmpty(t, resource) || !assert.Equal(t, int64(0), resource.Counter) {
+			return false
+		}
+		resource.Counter = 77
+		return assert.NoError(t, resource.UpdateValue())
+	})
+	resource, err := GetLockedResource(x, "test-1", 0)
+	assert.NoError(t, err)
+	assert.NotEmpty(t, resource)
+	assert.Equal(t, int64(77), resource.Counter)
+
+	assert.NoError(t, DeleteLockedResourceKey(x, "test-1", 0))
+	AssertExistsAndLoadBean(t, &LockedResource{LockType: "test-1", LockKey: 1})
+
 	// Attempt temp lock on an existing key, expect error
 	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
 		err := TemporarilyLockResourceKey(sess, "test-1", 1)

From c503f0d61bae77480e68603a756181ba0cdabfcb Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Tue, 28 Jan 2020 19:39:54 -0300
Subject: [PATCH 09/17] Prepare for merge

---
 models/migrations/migrations.go        | 2 +-
 models/migrations/{v125.go => v126.go} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename models/migrations/{v125.go => v126.go} (100%)

diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index 6244a116bb55c..e684776fc6f30 100644
--- a/models/migrations/migrations.go
+++ b/models/migrations/migrations.go
@@ -304,7 +304,7 @@ var migrations = []Migration{
 	NewMigration("Add original informations for reactions", addReactionOriginals),
 	// v124 -> v125
 	NewMigration("Add columns to user and repository", addUserRepoMissingColumns),
-	// v125 -> v126
+	// v126 -> v127
 	NewMigration("Add locked_resource table", addLockedResourceTable),
 }
 
diff --git a/models/migrations/v125.go b/models/migrations/v126.go
similarity index 100%
rename from models/migrations/v125.go
rename to models/migrations/v126.go

From 17926646a501d5a09ab1411dcf79d14994c0ad16 Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Tue, 28 Jan 2020 19:51:29 -0300
Subject: [PATCH 10/17] Go simple

---
 integrations/locked_resource_test.go | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/integrations/locked_resource_test.go b/integrations/locked_resource_test.go
index d95ce16038706..2d801d6c21e14 100644
--- a/integrations/locked_resource_test.go
+++ b/integrations/locked_resource_test.go
@@ -99,23 +99,23 @@ func blockTestFunc(name string, blocker bool, ref time.Time, f func(ctx models.D
 		name = fmt.Sprintf("waiter [%s]", name)
 	}
 	err := models.WithTx(func(ctx models.DBContext) error {
-		log.Trace("Entering %s @%d", name, time.Now().Sub(ref).Milliseconds())
+		log.Trace("Entering %s @%d", name, time.Since(ref).Milliseconds())
 		if !blocker {
-			log.Trace("Waiting on %s @%d", name, time.Now().Sub(ref).Milliseconds())
+			log.Trace("Waiting on %s @%d", name, time.Since(ref).Milliseconds())
 			time.Sleep(waiterDelay)
-			log.Trace("Wait finished on %s @%d", name, time.Now().Sub(ref).Milliseconds())
+			log.Trace("Wait finished on %s @%d", name, time.Since(ref).Milliseconds())
 		}
 		if err := f(ctx); err != nil {
 			return err
 		}
 		if blocker {
-			log.Trace("Waiting on %s @%d", name, time.Now().Sub(ref).Milliseconds())
+			log.Trace("Waiting on %s @%d", name, time.Since(ref).Milliseconds())
 			time.Sleep(blockerDelay)
-			log.Trace("Wait finished on %s @%d", name, time.Now().Sub(ref).Milliseconds())
+			log.Trace("Wait finished on %s @%d", name, time.Since(ref).Milliseconds())
 		} else {
-			wr.Waited = time.Now().Sub(ref)
+			wr.Waited = time.Since(ref)
 		}
-		log.Trace("Finishing %s @%d", name, time.Now().Sub(ref).Milliseconds())
+		log.Trace("Finishing %s @%d", name, time.Since(ref).Milliseconds())
 		return nil
 	})
 	if err != nil {

From ce6c24f574f62ae33aaa6e4091d62db956158ed1 Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Wed, 29 Jan 2020 21:05:48 -0300
Subject: [PATCH 11/17] Improve test legibility

---
 models/issue_test.go | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/models/issue_test.go b/models/issue_test.go
index cc867b78c51a8..033ea50c5affb 100644
--- a/models/issue_test.go
+++ b/models/issue_test.go
@@ -292,7 +292,7 @@ func TestIssue_SearchIssueIDsByKeyword(t *testing.T) {
 	assert.EqualValues(t, []int64{1}, ids)
 }
 
-func testInsertIssue(t *testing.T, title, content string, idx int64) {
+func testInsertIssue(t *testing.T, title, content string, idx int64) int64 {
 	repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
 	user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
 
@@ -313,17 +313,20 @@ func testInsertIssue(t *testing.T, title, content string, idx int64) {
 	assert.EqualValues(t, issue.Content, newIssue.Content)
 	assert.EqualValues(t, idx, newIssue.Index)
 
-	_, err = x.ID(issue.ID).Delete(new(Issue))
-	assert.NoError(t, err)
+	return issue.ID
 }
 
 func TestIssue_InsertIssue(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 
 	// there are 5 issues and max index is 5 on repository 1, so this one should be 6
-	testInsertIssue(t, "my issue1", "special issue's comments?", 6)
+	created := testInsertIssue(t, "my issue1", "special issue's comments?", 6)
+
+	_, err := x.ID(created).Delete(new(Issue))
+	assert.NoError(t, err)
+
 	// deleting an issue should not let a new issue reuse its index number; this one should be 7
-	testInsertIssue(t, `my issue2, this is my son's love \n \r \ `, "special issue's '' comments?", 7)
+	_ = testInsertIssue(t, `my issue2, this is my son's love \n \r \ `, "special issue's '' comments?", 7)
 }
 
 func TestIssue_ResolveMentions(t *testing.T) {

From 15ffbb434be611dbe5b534743169c966879b2e5f Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Wed, 29 Jan 2020 21:05:57 -0300
Subject: [PATCH 12/17] Fix typo

---
 models/locked_resource_test.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/models/locked_resource_test.go b/models/locked_resource_test.go
index 9a4bb40c3367d..d7e45fcced23d 100644
--- a/models/locked_resource_test.go
+++ b/models/locked_resource_test.go
@@ -97,5 +97,5 @@ func TestLockedResource(t *testing.T) {
 	})
 
 	// Note: testing the validity of the locking mechanism (i.e. whether it actually locks)
-	// is be done at the integration tests to ensure that all the supported databases are checked.
+	// is performed at the integration tests to ensure that all the supported databases are checked.
 }

From ea9c875202fbda747f3e318068d89580e8e8b95f Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Wed, 29 Jan 2020 21:06:06 -0300
Subject: [PATCH 13/17] Remove dead code

---
 models/error.go | 15 ---------------
 models/issue.go |  2 +-
 models/pull.go  |  2 +-
 3 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/models/error.go b/models/error.go
index fe9af70f3a49e..97faa55eb83ab 100644
--- a/models/error.go
+++ b/models/error.go
@@ -1155,21 +1155,6 @@ 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()
-}
-
 // ErrIssueWasClosed is used when close a closed issue
 type ErrIssueWasClosed struct {
 	ID    int64
diff --git a/models/issue.go b/models/issue.go
index a6f1fd1877cca..bb3c640ee97f0 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -1007,7 +1007,7 @@ func NewIssue(repo *Repository, issue *Issue, labelIDs []int64, uuids []string)
 		LabelIDs:    labelIDs,
 		Attachments: uuids,
 	}); err != nil {
-		if IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) {
+		if IsErrUserDoesNotHaveAccessToRepo(err) {
 			return err
 		}
 		return fmt.Errorf("newIssue: %v", err)
diff --git a/models/pull.go b/models/pull.go
index 8a533fac67b4c..6c47b249ea0e0 100644
--- a/models/pull.go
+++ b/models/pull.go
@@ -553,7 +553,7 @@ func NewPullRequest(repo *Repository, pull *Issue, labelIDs []int64, uuids []str
 		Attachments: uuids,
 		IsPull:      true,
 	}); err != nil {
-		if IsErrUserDoesNotHaveAccessToRepo(err) || IsErrNewIssueInsert(err) {
+		if IsErrUserDoesNotHaveAccessToRepo(err) {
 			return err
 		}
 		return fmt.Errorf("newIssue: %v", err)

From d185a4ff1496b455b8b8fa68db4f4a9555ad879a Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Sat, 1 Feb 2020 16:06:14 -0300
Subject: [PATCH 14/17] Prepare for merge

---
 models/migrations/migrations.go        | 2 +-
 models/migrations/{v126.go => v127.go} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename models/migrations/{v126.go => v127.go} (100%)

diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index 552aa7d571ce5..71028b963f827 100644
--- a/models/migrations/migrations.go
+++ b/models/migrations/migrations.go
@@ -306,7 +306,7 @@ var migrations = []Migration{
 	NewMigration("Add columns to user and repository", addUserRepoMissingColumns),
 	// v125 -> v126
 	NewMigration("Add some columns on review for migration", addReviewMigrateInfo),
-	// v126 -> v127
+	// v127 -> v128
 	NewMigration("Add locked_resource table", addLockedResourceTable),
 }
 
diff --git a/models/migrations/v126.go b/models/migrations/v127.go
similarity index 100%
rename from models/migrations/v126.go
rename to models/migrations/v127.go

From 621c9d6b2384a33168753c482f018a73e935d872 Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Wed, 12 Feb 2020 09:35:44 -0300
Subject: [PATCH 15/17] Prepare to merge

---
 models/migrations/migrations.go        | 2 +-
 models/migrations/{v127.go => v128.go} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename models/migrations/{v127.go => v128.go} (100%)

diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go
index ec1bab6fb152d..fad19b7badde8 100644
--- a/models/migrations/migrations.go
+++ b/models/migrations/migrations.go
@@ -186,7 +186,7 @@ var migrations = []Migration{
 	NewMigration("Add some columns on review for migration", addReviewMigrateInfo),
 	// v126 -> v127
 	NewMigration("Fix topic repository count", fixTopicRepositoryCount),
-	// v127 -> v128
+	// v128 -> v129
 	NewMigration("Add locked_resource table", addLockedResourceTable),
 }
 
diff --git a/models/migrations/v127.go b/models/migrations/v128.go
similarity index 100%
rename from models/migrations/v127.go
rename to models/migrations/v128.go

From 15e407b06e48daefb905de9d9f05e03f9c094cd9 Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Sat, 2 May 2020 13:09:07 -0300
Subject: [PATCH 16/17] Code review suggestions by @lunny

---
 models/issue.go                |  2 +-
 models/issue_xref_test.go      |  2 +-
 models/locked_resource.go      | 13 ++++---------
 models/locked_resource_test.go | 24 ++++++++++++------------
 4 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/models/issue.go b/models/issue.go
index 70ceb8dff33d2..8df6b54903e03 100644
--- a/models/issue.go
+++ b/models/issue.go
@@ -860,7 +860,7 @@ func newIssue(e *xorm.Session, doer *User, opts NewIssueOptions) (err error) {
 		return fmt.Errorf("GetLockedResource(%s)", IssueLockedEnumerator)
 	}
 	idxresource.Counter++
-	if err := idxresource.UpdateValue(); err != nil {
+	if err := idxresource.UpdateValue(e); err != nil {
 		return fmt.Errorf("locked.UpdateValue(%s)", IssueLockedEnumerator)
 	}
 	opts.Issue.Index = idxresource.Counter
diff --git a/models/issue_xref_test.go b/models/issue_xref_test.go
index 1b6a80586bb0c..160cb8d93af1a 100644
--- a/models/issue_xref_test.go
+++ b/models/issue_xref_test.go
@@ -133,7 +133,7 @@ func testCreateIssue(t *testing.T, repo, doer int64, title, content string, ispu
 	idxresource, err := GetLockedResource(sess, IssueLockedEnumerator, repo)
 	assert.NoError(t, err)
 	idxresource.Counter++
-	assert.NoError(t, idxresource.UpdateValue())
+	assert.NoError(t, idxresource.UpdateValue(sess))
 	i.Index = idxresource.Counter
 	_, err = sess.Insert(i)
 	assert.NoError(t, err)
diff --git a/models/locked_resource.go b/models/locked_resource.go
index 68592b5a5a7fa..8b60bc0efc24c 100644
--- a/models/locked_resource.go
+++ b/models/locked_resource.go
@@ -16,8 +16,6 @@ type LockedResource struct {
 	LockType string `xorm:"pk VARCHAR(30)"`
 	LockKey  int64  `xorm:"pk"`
 	Counter  int64  `xorm:"NOT NULL DEFAULT 0"`
-
-	engine Engine `xorm:"-"`
 }
 
 // GetLockedResource gets or creates a pessimistic lock on the given type and key
@@ -36,25 +34,22 @@ func GetLockedResource(e Engine, lockType string, lockKey int64) (*LockedResourc
 		return nil, fmt.Errorf("unexpected upsert fail  %s:%d", lockType, lockKey)
 	}
 
-	// Once active, the locked resource is tied to a specific session
-	resource.engine = e
-
 	return resource, nil
 }
 
 // UpdateValue updates the value of the counter of a locked resource
-func (r *LockedResource) UpdateValue() error {
+func (r *LockedResource) UpdateValue(e Engine) error {
 	// Bypass ORM to support lock_type == "" and lock_key == 0
-	_, err := r.engine.Exec("UPDATE locked_resource SET counter = ? WHERE lock_type = ? AND lock_key = ?",
+	_, err := e.Exec("UPDATE locked_resource SET counter = ? WHERE lock_type = ? AND lock_key = ?",
 		r.Counter, r.LockType, r.LockKey)
 	return err
 }
 
 // Delete deletes the locked resource from the database,
 // but the key remains locked until the end of the transaction
-func (r *LockedResource) Delete() error {
+func (r *LockedResource) Delete(e Engine) error {
 	// Bypass ORM to support lock_type == "" and lock_key == 0
-	_, err := r.engine.Exec("DELETE FROM locked_resource WHERE lock_type = ? AND lock_key = ?", r.LockType, r.LockKey)
+	_, err := e.Exec("DELETE FROM locked_resource WHERE lock_type = ? AND lock_key = ?", r.LockType, r.LockKey)
 	return err
 }
 
diff --git a/models/locked_resource_test.go b/models/locked_resource_test.go
index d7e45fcced23d..48959ae3f4de1 100644
--- a/models/locked_resource_test.go
+++ b/models/locked_resource_test.go
@@ -14,7 +14,7 @@ import (
 func TestLockedResource(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 
-	withSession := func(t *testing.T, f func(t *testing.T, sess *xorm.Session) bool) {
+	withTransaction := func(t *testing.T, f func(t *testing.T, sess *xorm.Session) bool) {
 		sess := x.NewSession()
 		defer sess.Close()
 		err := sess.Begin()
@@ -29,31 +29,31 @@ func TestLockedResource(t *testing.T) {
 	}
 
 	// Get lock, increment counter value
-	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+	withTransaction(t, func(t *testing.T, sess *xorm.Session) bool {
 		resource, err := GetLockedResource(sess, "test-1", 1)
 		if !assert.NoError(t, err) || !assert.NotEmpty(t, resource) || !assert.Equal(t, int64(0), resource.Counter) {
 			return false
 		}
 		resource.Counter++
-		err = resource.UpdateValue()
+		err = resource.UpdateValue(sess)
 		return assert.NoError(t, err)
 	})
 
 	// Get lock, check counter value
-	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+	withTransaction(t, func(t *testing.T, sess *xorm.Session) bool {
 		resource, err := GetLockedResource(sess, "test-1", 1)
 		return assert.NoError(t, err) && assert.NotEmpty(t, resource) && assert.Equal(t, int64(1), resource.Counter)
 	})
 
 	// Make sure LockKey == 0 is supported and we're not
 	// affecting other records
-	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+	withTransaction(t, func(t *testing.T, sess *xorm.Session) bool {
 		resource, err := GetLockedResource(sess, "test-1", 0)
 		if !assert.NoError(t, err) || !assert.NotEmpty(t, resource) || !assert.Equal(t, int64(0), resource.Counter) {
 			return false
 		}
 		resource.Counter = 77
-		return assert.NoError(t, resource.UpdateValue())
+		return assert.NoError(t, resource.UpdateValue(sess))
 	})
 	resource, err := GetLockedResource(x, "test-1", 0)
 	assert.NoError(t, err)
@@ -64,35 +64,35 @@ func TestLockedResource(t *testing.T) {
 	AssertExistsAndLoadBean(t, &LockedResource{LockType: "test-1", LockKey: 1})
 
 	// Attempt temp lock on an existing key, expect error
-	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+	withTransaction(t, func(t *testing.T, sess *xorm.Session) bool {
 		err := TemporarilyLockResourceKey(sess, "test-1", 1)
 		// Must give error
 		return assert.Error(t, err)
 	})
 
 	// Delete lock
-	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+	withTransaction(t, func(t *testing.T, sess *xorm.Session) bool {
 		resource, err := GetLockedResource(sess, "test-1", 1)
 		if !assert.NoError(t, err) || !assert.NotEmpty(t, resource) {
 			return false
 		}
-		return assert.NoError(t, resource.Delete())
+		return assert.NoError(t, resource.Delete(sess))
 	})
 	AssertNotExistsBean(t, &LockedResource{LockType: "test-1", LockKey: 1})
 
 	// Get lock, then delete by key
-	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+	withTransaction(t, func(t *testing.T, sess *xorm.Session) bool {
 		resource, err := GetLockedResource(sess, "test-1", 2)
 		return assert.NoError(t, err) && assert.NotEmpty(t, resource)
 	})
 	AssertExistsAndLoadBean(t, &LockedResource{LockType: "test-1", LockKey: 2})
-	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+	withTransaction(t, func(t *testing.T, sess *xorm.Session) bool {
 		return assert.NoError(t, DeleteLockedResourceKey(sess, "test-1", 2))
 	})
 	AssertNotExistsBean(t, &LockedResource{LockType: "test-1", LockKey: 2})
 
 	// Attempt temp lock on an valid key, expect success
-	withSession(t, func(t *testing.T, sess *xorm.Session) bool {
+	withTransaction(t, func(t *testing.T, sess *xorm.Session) bool {
 		return assert.NoError(t, TemporarilyLockResourceKey(sess, "test-1", 1))
 	})
 

From dd858737e08fb713210ee2ab744f2938700ed3e6 Mon Sep 17 00:00:00 2001
From: Guillermo Prandi <guillep2k@users.noreply.github.com>
Date: Sat, 2 May 2020 14:20:22 -0300
Subject: [PATCH 17/17] Ignore SQLite3 integration when _txlock=immediate

---
 integrations/locked_resource_test.go | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/integrations/locked_resource_test.go b/integrations/locked_resource_test.go
index 2d801d6c21e14..c82c2cf14abd3 100644
--- a/integrations/locked_resource_test.go
+++ b/integrations/locked_resource_test.go
@@ -6,11 +6,13 @@ package integrations
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 	"time"
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/setting"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -33,6 +35,16 @@ type waitResult struct {
 func TestLockedResource(t *testing.T) {
 	defer prepareTestEnv(t)()
 
+	// Skip SQLite3 test if we're using _txlock=immediate because we won't be able
+	// to create multiple update sessions with that setting in place which invalidates
+	// the test (although the locking mechanism is still useful).
+	connstr, err := setting.DBConnStr()
+	assert.NoError(t, err)
+	if strings.Contains(strings.ToLower(connstr), "_txlock=immediate") {
+		log.Info("TestLockedResource: test skipped for SQLite because _txlock=immediate is set.")
+		return
+	}
+
 	// We need to check whether two goroutines block each other
 	// Sadly, there's no way to ensure the second goroutine is
 	// waiting other than using a time delay. The longer the delay,