Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bugfix: modify XA mode pre commit transaction from commit phase to before close phase #7102

Open
wants to merge 17 commits into
base: 2.x
Choose a base branch
from

Conversation

xiaoxiangyeyu0
Copy link
Contributor

@xiaoxiangyeyu0 xiaoxiangyeyu0 commented Jan 11, 2025

  • I have registered the PR changes.

Ⅰ. Describe what this PR did

Modify XA mode pre commit transaction from commit phase to before close phase

Ⅱ. Does this pull request fix one issue?

fixes #7100

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

link #7100

Ⅴ. Special notes for reviews

Copy link

codecov bot commented Jan 12, 2025

Codecov Report

Attention: Patch coverage is 32.65306% with 33 lines in your changes missing coverage. Please review.

Project coverage is 53.14%. Comparing base (4632770) to head (690b73c).
Report is 1 commits behind head on 2.x.

Files with missing lines Patch % Lines
...ache/seata/rm/datasource/xa/ConnectionProxyXA.java 32.65% 27 Missing and 6 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #7102      +/-   ##
============================================
+ Coverage     53.08%   53.14%   +0.05%     
- Complexity     6795     6806      +11     
============================================
  Files          1136     1137       +1     
  Lines         40341    40365      +24     
  Branches       4723     4729       +6     
============================================
+ Hits          21416    21452      +36     
+ Misses        16885    16868      -17     
- Partials       2040     2045       +5     
Files with missing lines Coverage Δ
...ache/seata/rm/datasource/xa/ConnectionProxyXA.java 52.71% <32.65%> (-4.26%) ⬇️

... and 10 files with indirect coverage changes

@xingfudeshi
Copy link
Member

I suggest making the title more specific to better describe the bug that has been fixed.

@xiaoxiangyeyu0
Copy link
Contributor Author

Description added

@xingfudeshi xingfudeshi changed the title Fix bug in https://github.com/apache/incubator-seata/issues/7100 bugfix:modify XA mode pre commit transaction from commit phase to before close phase Jan 13, 2025
@funky-eyes funky-eyes changed the title bugfix:modify XA mode pre commit transaction from commit phase to before close phase bugfix: modify XA mode pre commit transaction from commit phase to before close phase Jan 13, 2025
} finally {
cleanXABranchContext();
xaActive = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个改成false后,如果通过setAutoCommit触发commit时要怎么办?
After changing this to false, what if the commit is triggered by setAutoCommit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里置为false是因为,自动提交事务第二次调用setAutoCommit时,if (xaActive)这里会调用commit方法,currentAutoCommitStatus在第一次被置为false,进入会继续执行end方法就会报错。
现在commit方法的xaActive = false;逻辑已去掉,setAutoCommit新加了一层判断 if (xaActive && !xaEnded)

return;
try {
if (xaEnded) {
termination();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我认为prepare这块的逻辑不需要termination
I don't think the logic of preparing this piece needs termination

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已调整commit从调用xaEnd改为end方法,这里已去掉

long now = System.currentTimeMillis();
checkTimeout(now);
setPrepareTime(now);
int prepare = xaResource.prepare(xaBranchXid);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

如果走了rollback的事务不需要prepare,直接上报一阶段失败即可
If the transaction that has gone rollback does not need to be prepared, it can directly report the failure of the first stage

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rollback方法的finally会调用cleanXABranchContext,这里会把xaEnded置为false

// if kept by a keeper, just hold the connection.
return;
try {
if (xaEnded) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end后的xa事务应该是可以join的,只要没有进入prepare阶段,如果我手动commit或rollback,再接着发sql,这里被标识成了xaended,如何进行prepare?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

接着发sql最终还是会调用commit方法,xaEnded会被置为true,close时依然会进行prepare

@funky-eyes funky-eyes added type: bug Category issues or prs related to bug. mode: XA XA transaction mode module/rm-datasource rm-datasource module labels Jan 13, 2025
@funky-eyes funky-eyes added this to the 2.4.0 milestone Jan 13, 2025
@@ -117,6 +117,10 @@ private void xaEnd(XAXid xaXid, int flags) throws XAException {
if (!xaEnded) {
xaResource.end(xaXid, flags);
xaEnded = true;
} else {
if (flags == XAResource.TMSUCCESS) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

为什么只有TMSUCCESS才需要end?join进来的不再end了吗?
假设事务1 commit,事务2继续用这个connection commit(等于先join)走到这个xaend,就不执行xaend了?还有rollback的时候会走到这个xaend方法,此时rollback不直接end,直接进行rollback吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xaEnded为false的时候会xaResource.end,这里flags == XAResource.TMSUCCESS是为了事务2继续用这个connection commit时能够xaResource.end,因为第一次commit把xaEnded置为true了

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiaoxiangyeyu0 我看到只在close的时候至为false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xaEnded默认是false,第一次调用xaEnd方法会把xaEnded置为true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connection没有close时直接复用,xaended就是true了,如何再次end? @xiaoxiangyeyu0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

传入的flags值为XAResource.TMSUCCESS,走else的分支会执行end

@funky-eyes funky-eyes requested a review from slievrly January 15, 2025 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mode: XA XA transaction mode module/rm-datasource rm-datasource module type: bug Category issues or prs related to bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

同一个PreparedStatement执行多次execute会报ShouldNeverHappenException
3 participants