thunguo commented on code in PR #1137:
URL:
https://github.com/apache/incubator-seata-go/pull/1137#discussion_r3620230167
##########
pkg/datasource/sql/conn_xa.go:
##########
@@ -237,22 +244,51 @@ func (c *XAConn) createNewTxOnExecIfNeed(ctx
context.Context, f func() (types.Ex
// execute SQL
ret, err := f()
if err != nil {
+ // driver.ErrSkip is not a real failure: it asks database/sql
to retry this
+ // statement through the Prepare+Exec fallback path.
+ if errors.Is(err, driver.ErrSkip) {
+ // If we already opened an XA branch for this statement
(autoCommit mode),
+ // the fallback retry would run OUTSIDE the branch -
autoCommit is now false
+ // and txCtx has been reset - leaving this branch
registered-but-never-
+ // prepared (a leak) and the retried write escaping the
global transaction.
+ // Roll the branch back and surface a real
(non-ErrSkip) error so the caller
+ // fails fast instead of silently corrupting the global
transaction.
+ if tx != nil {
+ if rollbackErr := tx.Rollback(); rollbackErr !=
nil {
+ log.Errorf("failed to rollback xa
branch of :%s after ErrSkip, err:%v", c.txCtx.XID, rollbackErr)
+ }
+ xaRollbacked = true
+ return nil, fmt.Errorf("xa branch %s cannot
fall back to non-XA execution after XA START: %v", c.txCtx.XID, err)
+ }
+ // No branch opened yet - safe to let database/sql use
its fallback path.
+ return nil, err
+ }
+ // On real error, rollback the entire branch. Prefer
XATx.Rollback so the
+ // already-registered branch reports phase-1 failure to the TC;
fall back to
+ // the raw connection rollback for non-autoCommit paths.
if tx != nil {
if rollbackErr := tx.Rollback(); rollbackErr != nil {
log.Errorf("failed to rollback xa branch of
:%s, err:%v", c.txCtx.XID, rollbackErr)
}
- } else {
+ } else if c.xaActive {
if rollbackErr := c.Rollback(ctx); rollbackErr != nil {
log.Errorf("failed to rollback xa branch of
:%s, err:%v", c.txCtx.XID, rollbackErr)
}
}
+ xaRollbacked = true // Mark that rollback was handled
return nil, err
}
+ // For autoCommit mode with global transaction, commit the branch now:
+ // XA END + XA PREPARE + report phase-1 success to TC.
if tx != nil && currentAutoCommit {
- // Commit through XATx so phase-one reporting stays coupled to
driver.Tx lifecycle.
if err = tx.Commit(); err != nil {
- log.Errorf("xa connection proxy commit failure xid:%s,
err:%v", c.txCtx.XID, err)
+ log.Errorf("xa transaction commit failure xid:%s,
err:%v", c.txCtx.XID, err)
+ // XA End & Rollback
+ if rollbackErr := tx.Rollback(); rollbackErr != nil {
Review Comment:
验证一下这里的 Rollback 会不会出现这种情况：
tx.Commit() 进行DB层的XA END +
PREPARE，然后向TC上报phase1失败了，tx.Commit返回失败之后这里的Rollback是不是会对一个已经 PREPARE 的 branch
重复调用 XA END，对照下后面 `end()` 方法里
```go
if c.xaErrorClassifier.IsAlreadyEnded(err) {
```
这部分，确认这个场景行为正确，不然对应分支会一直持有锁没法释放
##########
pkg/datasource/sql/exec/executor.go:
##########
Review Comment:
这里可以顺带修复一下，之前不会走到 `e.ex == nil` 这个路径，现在 `newXAExecutor` 是有可能构造出这种case
的，后面会出现参数数量不匹配的问题，改成下面这样即可。
```go
nvargs := make([]driver.NamedValue, 0, len(execCtx.Values))
```
然后补充一个e.ex == nil 的测试 case 验证下，以及修正 TestBaseExecutor_ExecWithValue
现在只判断非空，长度不对也能pass
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
