Skip to content

Commit 6135359

Browse files
wolfogrewolfogre
authored
Always reuse transaction (#22362)
1 parent d42b52f commit 6135359

File tree

11 files changed

+104
-42
lines changed

11 files changed

+104
-42
lines changed

‎models/activities/notification.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ func CountNotifications(ctx context.Context, opts *FindNotificationOptions) (int
141141

142142
// CreateRepoTransferNotification creates notification for the user a repository was transferred to
143143
func CreateRepoTransferNotification(ctx context.Context, doer, newOwner *user_model.User, repo *repo_model.Repository) error {
144-
return db.AutoTx(ctx, func(ctx context.Context) error {
144+
return db.WithTx(ctx, func(ctx context.Context) error {
145145
var notify []*Notification
146146

147147
if newOwner.IsOrganization() {

‎models/db/context.go

Lines changed: 41 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -71,14 +71,22 @@ type Engined interface {
7171

7272
// GetEngine will get a db Engine from this context or return an Engine restricted to this context
7373
func GetEngine(ctx context.Context) Engine {
74+
if e := getEngine(ctx); e != nil {
75+
return e
76+
}
77+
return x.Context(ctx)
78+
}
79+
80+
// getEngine will get a db Engine from this context or return nil
81+
func getEngine(ctx context.Context) Engine {
7482
if engined, ok := ctx.(Engined); ok {
7583
return engined.Engine()
7684
}
7785
enginedInterface := ctx.Value(enginedContextKey)
7886
if enginedInterface != nil {
7987
return enginedInterface.(Engined).Engine()
8088
}
81-
return x.Context(ctx)
89+
return nil
8290
}
8391

8492
// Committer represents an interface to Commit or Close the Context
@@ -87,10 +95,22 @@ type Committer interface {
8795
Close() error
8896
}
8997

90-
// TxContext represents a transaction Context
98+
// halfCommitter is a wrapper of Committer.
99+
// It can be closed early, but can't be committed early, it is useful for reusing a transaction.
100+
type halfCommitter struct {
101+
Committer
102+
}
103+
104+
func (*halfCommitter) Commit() error {
105+
// do nothing
106+
return nil
107+
}
108+
109+
// TxContext represents a transaction Context,
110+
// it will reuse the existing transaction in the parent context or create a new one.
91111
func TxContext(parentCtx context.Context) (*Context, Committer, error) {
92-
if InTransaction(parentCtx) {
93-
return nil, nil, ErrAlreadyInTransaction
112+
if sess, ok := inTransaction(parentCtx); ok {
113+
return newContext(parentCtx, sess, true), &halfCommitter{Committer: sess}, nil
94114
}
95115

96116
sess := x.NewSession()
@@ -102,20 +122,11 @@ func TxContext(parentCtx context.Context) (*Context, Committer, error) {
102122
return newContext(DefaultContext, sess, true), sess, nil
103123
}
104124

105-
// WithTx represents executing database operations on a transaction
106-
// This function will always open a new transaction, if a transaction exist in parentCtx return an error.
107-
func WithTx(parentCtx context.Context, f func(ctx context.Context) error) error {
108-
if InTransaction(parentCtx) {
109-
return ErrAlreadyInTransaction
110-
}
111-
return txWithNoCheck(parentCtx, f)
112-
}
113-
114-
// AutoTx represents executing database operations on a transaction, if the transaction exist,
125+
// WithTx represents executing database operations on a transaction, if the transaction exist,
115126
// this function will reuse it otherwise will create a new one and close it when finished.
116-
func AutoTx(parentCtx context.Context, f func(ctx context.Context) error) error {
117-
if InTransaction(parentCtx) {
118-
return f(newContext(parentCtx, GetEngine(parentCtx), true))
127+
func WithTx(parentCtx context.Context, f func(ctx context.Context) error) error {
128+
if sess, ok := inTransaction(parentCtx); ok {
129+
return f(newContext(parentCtx, sess, true))
119130
}
120131
return txWithNoCheck(parentCtx, f)
121132
}
@@ -202,25 +213,25 @@ func EstimateCount(ctx context.Context, bean interface{}) (int64, error) {
202213

203214
// InTransaction returns true if the engine is in a transaction otherwise return false
204215
func InTransaction(ctx context.Context) bool {
205-
var e Engine
206-
if engined, ok := ctx.(Engined); ok {
207-
e = engined.Engine()
208-
} else {
209-
enginedInterface := ctx.Value(enginedContextKey)
210-
if enginedInterface != nil {
211-
e = enginedInterface.(Engined).Engine()
212-
}
213-
}
216+
_, ok := inTransaction(ctx)
217+
return ok
218+
}
219+
220+
func inTransaction(ctx context.Context) (*xorm.Session, bool) {
221+
e := getEngine(ctx)
214222
if e == nil {
215-
return false
223+
return nil, false
216224
}
217225

218226
switch t := e.(type) {
219227
case *xorm.Engine:
220-
return false
228+
return nil, false
221229
case *xorm.Session:
222-
return t.IsInTx()
230+
if t.IsInTx() {
231+
return t, true
232+
}
233+
return nil, false
223234
default:
224-
return false
235+
return nil, false
225236
}
226237
}

‎models/db/context_test.go

Lines changed: 55 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,62 @@ func TestInTransaction(t *testing.T) {
2525
assert.NoError(t, err)
2626
defer committer.Close()
2727
assert.True(t, db.InTransaction(ctx))
28-
assert.Error(t, db.WithTx(ctx, func(ctx context.Context) error {
28+
assert.NoError(t, db.WithTx(ctx, func(ctx context.Context) error {
2929
assert.True(t, db.InTransaction(ctx))
3030
return nil
3131
}))
3232
}
33+
34+
func TestTxContext(t *testing.T) {
35+
assert.NoError(t, unittest.PrepareTestDatabase())
36+
37+
{ // create new transaction
38+
ctx, committer, err := db.TxContext(db.DefaultContext)
39+
assert.NoError(t, err)
40+
assert.True(t, db.InTransaction(ctx))
41+
assert.NoError(t, committer.Commit())
42+
}
43+
44+
{ // reuse the transaction created by TxContext and commit it
45+
ctx, committer, err := db.TxContext(db.DefaultContext)
46+
engine := db.GetEngine(ctx)
47+
assert.NoError(t, err)
48+
assert.True(t, db.InTransaction(ctx))
49+
{
50+
ctx, committer, err := db.TxContext(ctx)
51+
assert.NoError(t, err)
52+
assert.True(t, db.InTransaction(ctx))
53+
assert.Equal(t, engine, db.GetEngine(ctx))
54+
assert.NoError(t, committer.Commit())
55+
}
56+
assert.NoError(t, committer.Commit())
57+
}
58+
59+
{ // reuse the transaction created by TxContext and close it
60+
ctx, committer, err := db.TxContext(db.DefaultContext)
61+
engine := db.GetEngine(ctx)
62+
assert.NoError(t, err)
63+
assert.True(t, db.InTransaction(ctx))
64+
{
65+
ctx, committer, err := db.TxContext(ctx)
66+
assert.NoError(t, err)
67+
assert.True(t, db.InTransaction(ctx))
68+
assert.Equal(t, engine, db.GetEngine(ctx))
69+
assert.NoError(t, committer.Close())
70+
}
71+
assert.NoError(t, committer.Close())
72+
}
73+
74+
{ // reuse the transaction created by WithTx
75+
assert.NoError(t, db.WithTx(db.DefaultContext, func(ctx context.Context) error {
76+
assert.True(t, db.InTransaction(ctx))
77+
{
78+
ctx, committer, err := db.TxContext(ctx)
79+
assert.NoError(t, err)
80+
assert.True(t, db.InTransaction(ctx))
81+
assert.NoError(t, committer.Commit())
82+
}
83+
return nil
84+
}))
85+
}
86+
}

‎models/db/error.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,11 @@
44
package db
55

66
import (
7-
"errors"
87
"fmt"
98

109
"code.gitea.io/gitea/modules/util"
1110
)
1211

13-
var ErrAlreadyInTransaction = errors.New("database connection has already been in a transaction")
14-
1512
// ErrCancelled represents an error due to context cancellation
1613
type ErrCancelled struct {
1714
Message string

‎models/issues/issue.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2365,7 +2365,7 @@ func CountOrphanedIssues(ctx context.Context) (int64, error) {
23652365
// DeleteOrphanedIssues delete issues without a repo
23662366
func DeleteOrphanedIssues(ctx context.Context) error {
23672367
var attachmentPaths []string
2368-
err := db.AutoTx(ctx, func(ctx context.Context) error {
2368+
err := db.WithTx(ctx, func(ctx context.Context) error {
23692369
var ids []int64
23702370

23712371
if err := db.GetEngine(ctx).Table("issue").Distinct("issue.repo_id").

‎models/project/project.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,7 +300,7 @@ func changeProjectStatus(ctx context.Context, p *Project, isClosed bool) error {
300300
// DeleteProjectByID deletes a project from a repository. if it's not in a database
301301
// transaction, it will start a new database transaction
302302
func DeleteProjectByID(ctx context.Context, id int64) error {
303-
return db.AutoTx(ctx, func(ctx context.Context) error {
303+
return db.WithTx(ctx, func(ctx context.Context) error {
304304
p, err := GetProjectByID(ctx, id)
305305
if err != nil {
306306
if IsErrProjectNotExist(err) {

‎models/repo/collaboration.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ func ChangeCollaborationAccessMode(ctx context.Context, repo *Repository, uid in
105105
return nil
106106
}
107107

108-
return db.AutoTx(ctx, func(ctx context.Context) error {
108+
return db.WithTx(ctx, func(ctx context.Context) error {
109109
e := db.GetEngine(ctx)
110110

111111
collaboration := &Collaboration{

‎models/repo_transfer.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ func TestRepositoryReadyForTransfer(status repo_model.RepositoryStatus) error {
155155
// CreatePendingRepositoryTransfer transfer a repo from one owner to a new one.
156156
// it marks the repository transfer as "pending"
157157
func CreatePendingRepositoryTransfer(ctx context.Context, doer, newOwner *user_model.User, repoID int64, teams []*organization.Team) error {
158-
return db.AutoTx(ctx, func(ctx context.Context) error {
158+
return db.WithTx(ctx, func(ctx context.Context) error {
159159
repo, err := repo_model.GetRepositoryByID(ctx, repoID)
160160
if err != nil {
161161
return err

‎modules/notification/ui/ui.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ func (ns *notificationService) NotifyPullReviewRequest(ctx context.Context, doer
243243
}
244244

245245
func (ns *notificationService) NotifyRepoPendingTransfer(ctx context.Context, doer, newOwner *user_model.User, repo *repo_model.Repository) {
246-
err := db.AutoTx(ctx, func(ctx context.Context) error {
246+
err := db.WithTx(ctx, func(ctx context.Context) error {
247247
return activities_model.CreateRepoTransferNotification(ctx, doer, newOwner, repo)
248248
})
249249
if err != nil {

‎modules/repository/collaborator.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import (
1414
)
1515

1616
func AddCollaborator(ctx context.Context, repo *repo_model.Repository, u *user_model.User) error {
17-
return db.AutoTx(ctx, func(ctx context.Context) error {
17+
return db.WithTx(ctx, func(ctx context.Context) error {
1818
collaboration := &repo_model.Collaboration{
1919
RepoID: repo.ID,
2020
UserID: u.ID,

‎services/issue/comments.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ func UpdateComment(ctx context.Context, c *issues_model.Comment, doer *user_mode
123123

124124
// DeleteComment deletes the comment
125125
func DeleteComment(ctx context.Context, doer *user_model.User, comment *issues_model.Comment) error {
126-
err := db.AutoTx(ctx, func(ctx context.Context) error {
126+
err := db.WithTx(ctx, func(ctx context.Context) error {
127127
return issues_model.DeleteComment(ctx, comment)
128128
})
129129
if err != nil {

0 commit comments

Comments
 (0)