diff options
author | Jason Song <i@wolfogre.com> | 2024-03-25 15:00:16 +0800 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-03-25 15:00:16 +0800 |
commit | c6c4d66004c70b24abc8048b39b660b8361a0395 (patch) | |
tree | da0063343c72492a9ebf0c6a4506e230766806a6 /models | |
parent | 2e31a2800e1112ee0ab5a8d3c66b0fba2e737870 (diff) | |
download | gitea-c6c4d66004c70b24abc8048b39b660b8361a0395.tar.gz gitea-c6c4d66004c70b24abc8048b39b660b8361a0395.zip |
Fix misuse of `TxContext` (#30061)
Help #29999, or its tests cannot pass.
Also, add some comments to clarify the usage of `TxContext`.
I don't check all usages of `TxContext` because there are too many
(almost 140+). It's a better idea to replace them with `WithTx` instead
of checking them one by one. However, that may be another refactoring
PR.
Diffstat (limited to 'models')
-rw-r--r-- | models/db/context.go | 10 | ||||
-rw-r--r-- | models/issues/review.go | 2 |
2 files changed, 11 insertions, 1 deletions
diff --git a/models/db/context.go b/models/db/context.go index cda608af19..43f612518a 100644 --- a/models/db/context.go +++ b/models/db/context.go @@ -120,6 +120,16 @@ func (c *halfCommitter) Close() error { // TxContext represents a transaction Context, // it will reuse the existing transaction in the parent context or create a new one. +// Some tips to use: +// +// 1 It's always recommended to use `WithTx` in new code instead of `TxContext`, since `WithTx` will handle the transaction automatically. +// 2. To maintain the old code which uses `TxContext`: +// a. Always call `Close()` before returning regardless of whether `Commit()` has been called. +// b. Always call `Commit()` before returning if there are no errors, even if the code did not change any data. +// c. Remember the `Committer` will be a halfCommitter when a transaction is being reused. +// So calling `Commit()` will do nothing, but calling `Close()` without calling `Commit()` will rollback the transaction. +// And all operations submitted by the caller stack will be rollbacked as well, not only the operations in the current function. +// d. It doesn't mean rollback is forbidden, but always do it only when there is an error, and you do want to rollback. func TxContext(parentCtx context.Context) (*Context, Committer, error) { if sess, ok := inTransaction(parentCtx); ok { return newContext(parentCtx, sess, true), &halfCommitter{committer: sess}, nil diff --git a/models/issues/review.go b/models/issues/review.go index 70aba0f94d..455bcda50a 100644 --- a/models/issues/review.go +++ b/models/issues/review.go @@ -620,7 +620,7 @@ func AddReviewRequest(ctx context.Context, issue *Issue, reviewer, doer *user_mo // skip it when reviewer hase been request to review if review != nil && review.Type == ReviewTypeRequest { - return nil, nil + return nil, committer.Commit() // still commit the transaction, or committer.Close() will rollback it, even if it's a reused transaction. } // if the reviewer is an official reviewer, |