fix(app): use a new context with timeout for afterStop #3228
Conversation
dosubot
bot
added
the
size:M
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.
This changes the default behavior. Before modification, it defaults to never timeout.
There was a problem hiding this comment.
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.
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.
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.
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.
What should users do if they wish to use ctx passed in through kratos.Context
There was a problem hiding this comment.
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):
Appwill always use a cancelledcontext.ContextforafterStopfunctions. This PR uses a new context with a default timeout (5s) forafterStopfunctions and provides a new option for setting that timeout.Which issue(s) this PR fixes (resolves / be part of):
fix #3226