go/cmd: Audit and fix context.Background() usage #15928
Conversation
dbussink
requested review from
ajm188,
mattlord,
notfelineit and
deepthi
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
| @@ -189,17 +189,20 @@ func run(cmd *cobra.Command, args []string) (err error) { | |||
| cmd.Flags().Set("log_dir", "$VTDATAROOT/tmp") | |||
| } | |||
|
|
|||
| ctx, cancel := context.WithCancel(cmd.Context()) | |||
| defer cancel() | |||
There was a problem hiding this comment.
This creates the toplevel context here and then uses it for example for creating the memory topo and other operations too.
Once this cancels, it ensures that things like the topo and watchers etc. are closed in the right order.
| } | ||
|
|
||
| servenv.OnRun(func() { | ||
| addStatusParts(vtg) | ||
| }) | ||
|
|
||
| servenv.OnTerm(func() { | ||
| log.Error("Terminating") | ||
| // FIXME(alainjobart): stop vtgate |
There was a problem hiding this comment.
This is actually fixed now that the context is cancelled that is used to setup vtgate, so we can remove the TODO and the whole block here as it does nothing.
| // We will still use the topo server during lameduck period | ||
| // to update our state, so closing it in OnClose() | ||
| ts.Close() | ||
| }) |
There was a problem hiding this comment.
Closing the topo isn't a long running operation or anything, so we can do it with a defer when we create it, removing the need for this callback to be used here at all.
It doesn't change the order of things, since right after RunDefault below we return, and then we'd run the defer now to close ts as well.
| resilientServer = srvtopo.NewResilientServer(context.Background(), ts, srvTopoCounts) | ||
| ctx, cancel := context.WithCancel(cmd.Context()) | ||
| defer cancel() | ||
| resilientServer = srvtopo.NewResilientServer(ctx, ts, srvTopoCounts) |
There was a problem hiding this comment.
This fix ensures that during shutdown we don't hit the panic from #15927, because the context will have been cancelled always before the topo is closed (which is deferred right above here).
| defer ts.Close() | ||
|
|
||
| ctx, cancel := context.WithCancel(cmd.Context()) | ||
| defer cancel() |
There was a problem hiding this comment.
This logic is moved here to match how vtgate is set up, so we open and defer close / cancel in the right order.
There was a problem hiding this comment.
Can you also add a comment in the code to this effect as well? That will help to ensure that this implied behavior is not changed later by someone not fully realizing the important of the order here.
| @@ -170,9 +175,6 @@ func run(cmd *cobra.Command, args []string) error { | |||
| // Close the tm so that our topo entry gets pruned properly and any | |||
| // background goroutines that use the topo connection are stopped. | |||
| tm.Close() | |||
|
|
|||
| // tm uses ts. So, it should be closed after tm. | |||
| ts.Close() | |||
There was a problem hiding this comment.
We handle this with the above defer, so no need to close it here.
dbussink
removed
NeedsDescriptionUpdate
dbussink
force-pushed
the
dbussink/fix-cmd-context-setup
branch
from
fbf3970 to
42b4588
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15928 +/- ##
==========================================
+ Coverage 68.45% 68.46% +0.01%
==========================================
Files 1559 1559
Lines 196872 196871 -1
==========================================
+ Hits 134769 134789 +20
+ Misses 62103 62082 -21 ☔ View full report in Codecov by Sentry. |
| defer ts.Close() | ||
|
|
||
| ctx, cancel := context.WithCancel(cmd.Context()) | ||
| defer cancel() |
There was a problem hiding this comment.
Can you also add a comment in the code to this effect as well? That will help to ensure that this implied behavior is not changed later by someone not fully realizing the important of the order here.
This removes a bunch of the context.Background() usage for the command line entrypoints. In general, we should use the command context (which normally is context.Background(), but it's more semantically accurate). There's a few cases where we need more fixes. Specifically in vtgate where we want to setup a cancellable context and cancel it when we shut down. This is the one that ends up running things like the topo watcher and this ensures things are closed appropriately. Similarly in vtcombo we apply similar fixes so that we always correctly cancel the context on shutdown and the same for vttablet. Signed-off-by: Dirkjan Bussink <[email protected]>
Signed-off-by: Dirkjan Bussink <[email protected]>
dbussink
force-pushed
the
dbussink/fix-cmd-context-setup
branch
from
42b4588 to
cf32fe9
Compare
This removes a bunch of the context.Background() usage for the command line entrypoints. In general, we should use the command context (which normally is context.Background(), but it's more semantically accurate).
There's a few cases where we need more fixes. Specifically in vtgate where we want to setup a cancellable context and cancel it when we shut down. This is the one that ends up running things like the topo watcher and this ensures things are closed appropriately.
Similarly in vtcombo we apply similar fixes so that we always correctly cancel the context on shutdown and the same for vttablet.
Related Issue(s)
Fixes #15927
Checklist