Don't close eventChannel when panicking
* Don't close eventChannel when panicking
The state of the system is completely unknown when panicking and in
general it's not safe to infer whether or not it is safe to close a
channel when in this tate.
* CR feedback
* Spelling
@@ -75,10 +75,12 @@ func PreviewThenPrompt(ctx context.Context, kind apitype.UpdateKind, stack Stack
op UpdateOperation, apply Applier) (engine.ResourceChanges, error) {
// create a channel to hear about the update events from the engine. this will be used so that
// we can build up the diff display in case the user asks to see the details of the diff
+
+ // Note that eventsChannel is not closed in a `defer`. It is generally unsafe to do so, since defers run during
+ // panics and we can't know whether or not we were in the middle of writing to this channel when the panic occurred.
+ //
+ // Instead of using a `defer`, we manually close `eventsChannel` on every exit of this function.
eventsChannel := make(chan engine.Event)
- defer func() {
- close(eventsChannel)
- }()
var events []engine.Event
go func() {
@@ -98,6 +100,7 @@ func PreviewThenPrompt(ctx context.Context, kind apitype.UpdateKind, stack Stack
if !op.Opts.SkipPreview {
c, err := apply(ctx, kind, stack, op, true /*dryRun*/, false /*persist*/, eventsChannel)
if err != nil {
+ close(eventsChannel)
return c, err
}
changes = c
@@ -105,11 +108,14 @@ func PreviewThenPrompt(ctx context.Context, kind apitype.UpdateKind, stack Stack
// If there are no changes, or we're auto-approving or just previewing, we can skip the confirmation prompt.
if op.Opts.AutoApprove || kind == apitype.PreviewUpdate {
+ close(eventsChannel)
return changes, nil
}
// Otherwise, ensure the user wants to proceed.
- return changes, confirmBeforeUpdating(kind, stack, events, op.Opts)
+ err := confirmBeforeUpdating(kind, stack, events, op.Opts)
+ close(eventsChannel)
+ return changes, err
}
// confirmBeforeUpdating asks the user whether to proceed. A nil error means yes.
