chore: cancel context on signals #3042
Conversation
| // process group, and exits. | ||
| func RunWithSignalHandler(ctx context.Context, run func(ctx context.Context) error) error { | ||
| logger := log.FromContext(ctx) | ||
| ctx, stop := signal.NotifyContext(ctx, syscall.SIGINT, syscall.SIGTERM) |
There was a problem hiding this comment.
Doesn't the work around context cancellation happen asynchronously? Given that we immediately send SIGTERM won't it be non-deterministic as to which stuff actually sees the context cancellation?
There was a problem hiding this comment.
"run" won't return until it and all its child goroutines are complete, at which point there shouldn't be anything still running. This is actually an improvement on the previous behaviour, which listened for a signal and immediately ran kill + exit.
There was a problem hiding this comment.
What makes 'run' not return until child goroutines complete?
There was a problem hiding this comment.
The design of the function... using errgroups to track sub-tasks, etc...
There was a problem hiding this comment.
Ah, so this needs follow up work to make sure this happens, because this is definitely not true at the moment.
If we do this properly we probably also want a timeout with some kind of dump threads functionality to diagnose shutdown hangs.
There was a problem hiding this comment.
Correct, this is just the introduction of the basic cancellation.
Actually I may not be able to merge this. Turns out the only way the BuildEngine will terminate currently is by being literally killed, so yep.
alecthomas
force-pushed
the
aat/signal-handlers
branch
2 times, most recently
from
d65ce2d
to
a734fe4
Compare
alecthomas
requested review from
a team,
stuartwdouglas,
worstell and
deniseli
and removed request for
a team
alecthomas
force-pushed
the
aat/signal-handlers
branch
from
ab3f628
to
09d4f30
Compare
No description provided.