A little refactoring and better function naming

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)
 	}))
 }
 

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

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))

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.

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)