-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(app): use a new context with timeout for afterStop #3228
base: main
Are you sure you want to change the base?
Conversation
app.go
Outdated
@@ -42,6 +42,7 @@ func New(opts ...Option) *App { | |||
sigs: []os.Signal{syscall.SIGTERM, syscall.SIGQUIT, syscall.SIGINT}, | |||
registrarTimeout: 10 * time.Second, | |||
stopTimeout: 10 * time.Second, | |||
afterStopTimeout: 5 * time.Second, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the default behavior. Before modification, it defaults to never timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current code uses a canceled context whose Done()
will return a closed channel behaving the same as a timed out context. I think it defaults to timeout immediately before modification rather than never timeout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope it can remain the same as before, and if you need to control the time, you can set the timeout yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The introduction of hook has brought many problems, and I don't want it to have more problems. So I need it to be as consistent as possible with the previous logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late reply. I have updated the PR to set default afterstop timeout to zero. Kratos will pass a canceled context to AfterStop functions as the same as before if user don't specify the AfterStop timeout.
app.go
Outdated
@@ -140,8 +141,11 @@ func (a *App) Run() error { | |||
return err | |||
} | |||
err = nil | |||
|
|||
asCtx, cancel := context.WithTimeout(NewContext(context.Background(), a), a.opts.afterStopTimeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should users do if they wish to use ctx passed in through kratos.Context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. we should create new context from App.opts.ctx
.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #3228 +/- ##
==========================================
- Coverage 84.72% 84.64% -0.08%
==========================================
Files 88 88
Lines 3993 3999 +6
==========================================
+ Hits 3383 3385 +2
- Misses 438 440 +2
- Partials 172 174 +2 ☔ View full report in Codecov by Sentry. |
Description (what this PR does / why we need it):
App
will always use a cancelledcontext.Context
forafterStop
functions. This PR uses a new context with a default timeout (5s) forafterStop
functions and provides a new option for setting that timeout.Which issue(s) this PR fixes (resolves / be part of):
fix #3226