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

fix: catch server stop error #2125

Merged
merged 1 commit into from
Jun 22, 2022

Conversation

charviki
Copy link
Contributor

Description (what this PR does / why we need it):

问题:服务在收到 os.Signal 后退出,但并未返回服务在关闭过程中出现的错误。
原因:在 errgroup 中返回了 ctx.Err()。按照原有的代码逻辑,如果不考虑极端情况,在收到 os.Signalerrgroup 都会返回 context.Canceled,因为 errgroup 只会返回第一个错误,而 context.Canceled 这个错误在 Run 方法中会被忽略,这就导致了 Run 未捕获到服务关闭过程中的错误。
p.s. 这里把 for 代码也删了,因为考虑到 ctx.Err() 仅会返回 context.Canceled 以及 context.DeadlineExceeded,结合上下文,该 ctx 不会出现超时的情况,所以不需要返回 ctx.Err()

Which issue(s) this PR fixes (resolves / be part of):

Other special notes for the reviewers:

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2022

Codecov Report

Merging #2125 (4121362) into main (a0e624c) will increase coverage by 0.06%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #2125      +/-   ##
==========================================
+ Coverage   78.99%   79.06%   +0.06%     
==========================================
  Files          84       84              
  Lines        3699     3697       -2     
==========================================
+ Hits         2922     2923       +1     
+ Misses        564      561       -3     
  Partials      213      213              
Impacted Files Coverage Δ
app.go 83.83% <60.00%> (+0.67%) ⬆️
internal/context/context.go 100.00% <0.00%> (+2.98%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0e624c...4121362. Read the comment docs.

@tonybase tonybase merged commit a3439c9 into go-kratos:main Jun 22, 2022
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.

None yet

4 participants