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: fix return of Commit and Rollback #595

Merged
merged 3 commits into from
Feb 3, 2024
Merged

bugfix: fix return of Commit and Rollback #595

merged 3 commits into from
Feb 3, 2024

Conversation

luweiqianyi
Copy link
Contributor

commit or rollback success,but ctx error happens, should not return "failed"

What this PR does: commit or rollback success,but ctx error happens

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?: yes

SendSyncRequest success,  ctx error happens, should not return  "send global commit request failed ..." or "GlobalRollbackRequest rollback failed ..."

commit or rollback success,but ctx error happens, should not return "failed"
@codecov-commenter
Copy link

Codecov Report

Merging #595 (838dc68) into master (5ecf43a) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master     #595   +/-   ##
=======================================
  Coverage   37.08%   37.08%           
=======================================
  Files         172      172           
  Lines       11616    11616           
=======================================
  Hits         4308     4308           
  Misses       6944     6944           
  Partials      364      364           
Files Changed Coverage Δ
pkg/tm/global_transaction.go 100.00% <100.00%> (ø)

@iSuperCoder iSuperCoder requested a review from georgehao July 25, 2023 02:06
@iSuperCoder
Copy link
Contributor

@luweiqianyi 展开描述下这个问题的复现步骤和实际影响?

@Issues-translate-bot
Copy link

RoBot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@luweiqianyi Expand and describe the reproduction steps and actual impact of this problem?

@Issues-translate-bot
Copy link

RoBot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@luweiqianyi expand and describe the reproduction steps and actual impact of this problem?

I used the main method of the file in this link "https://github.com/seata/seata-go-samples/blob/main/tcc/local/cmd/local.go" as the test script, and passed it to tm.WithGlobalTx a ctx, cancel := context.WithTimeout(context.Background(), time.Second*5) ctx, and simulated "Successful submission, but ct x error”, for the Commit function, submitting the global commit request is indeed successful, at this time err=nil, but the original code before modification will return send global commit request failed, is it unreasonable, and Rollback also has the same problem

@luweiqianyi
Copy link
Contributor Author

luweiqianyi commented Jul 25, 2023

@luweiqianyi 展开描述下这个问题的复现步骤和实际影响?

我就以这个链接“https://github.com/seata/seata-go-samples/blob/main/tcc/local/cmd/local.go”的文件的main方法为测试脚本了,传给tm.WithGlobalTx一个ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)的ctx,并且通过打断点调试的方式来模拟 “提交成功、但是ctx error”的情况,对于Commit函数来说,提交全局commit请求确实成功了,此时err=nil,但是修改前的原代码是会日志输出 send global commit request failed, 是不是不太合理, 同理Rollback也有相同的问题

@iSuperCoder

@Issues-translate-bot
Copy link

RoBot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@luweiqianyi Expand and describe the reproduction steps and actual impact of this problem?

I used the main method of the file in this link "[https://github.com/seata/seata-go-samples/blob/main/tcc/local/cmd/local.go" as the test script, and passed a ctx to tm.WithGlobalTx](https://github.com/seata/seata-go-samples/blob/main/tcc/local/cmd/local.go%E2%80%9D%E7%9 A%84%E6%96%87%E4%BB%B6%E7%9A%84main%E6%96%B9%E6%B3%95%E4%B8%BA%E6%B5%8B%E8%AF%95%E8%84%9A%E6%9C%AC%E4%BA%86%EF%BC%8C%E4%BC%A0%E7%BB%99t m.WithGlobalTx%E4%B8%80%E4%B8%AActx), cancel := ctx of context.WithTimeout(context.Background(), time.Second*5), and simulate the situation of "commit successfully, but ctx error" by breaking point debugging. For the Commit function, submitting the global commit request is indeed successful. At this time, err=nil, but the original code before modification will be Return send global commit request failed, is it unreasonable? Similarly, Rollback has the same problem

@iSuperCoder

Copy link
Contributor

@iSuperCoder iSuperCoder left a comment

Choose a reason for hiding this comment

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

这个修改应该不太对,建议业务调用方设置合理的超时时间。
如果是bf.Ongoing()调用时ctx超时,全局事务提交的请求就不会发送出去,此时err也不为nil。按照你修改后的判断逻辑,将会打印success的日志,明显是不对的。

@slievrly slievrly added this to the 1.3.0 milestone Aug 15, 2023
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

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

LGTM

@luky116 luky116 merged commit 3534336 into apache:master Feb 3, 2024
5 checks passed
@luweiqianyi luweiqianyi deleted the patch-2 branch February 5, 2024 14:29
@luweiqianyi luweiqianyi restored the patch-2 branch February 5, 2024 14:30
@slievrly slievrly modified the milestones: 1.3.0, 2.0.0 Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants