Properly reap child processes

This change reaps child plugin processes before exiting.  It also hardens
some of the exit paths to avoid os.Exiting from the middle of a callstack.

cmd/coconut.go

@@ -61,6 +61,18 @@ func sink() diag.Sink {
 	return snk
 }
 
+// runFunc wraps an error-returning run func with standard Coconut error handling.  All Coconut commands should wrap
+// themselves in this to ensure consistent and appropriate error behavior.  In particular, we want to avoid any calls to
+// os.Exit in the middle of a callstack which might prohibit reaping of child processes, resources, etc.  And we wish to
+// avoid the default Cobra unhandled error behavior, because it is formatted incorrectly and needlessly prints usage.
+func runFunc(run func(cmd *cobra.Command, args []string) error) func(*cobra.Command, []string) {
+	return func(cmd *cobra.Command, args []string) {
+		if err := run(cmd, args); err != nil {
+			exitError(err.Error())
+		}
+	}
+}
+
 // exitErrorPrefix is auto-appended to any abrupt command exit.
 const exitErrorPrefix = "fatal: "
 

cmd/describe.go

@@ -29,7 +29,7 @@ func newDescribeCmd() *cobra.Command {
 		Long: "Describe one or more Nuts\n" +
 			"\n" +
 			"This command prints package, symbol, and IL information from one or more Nuts.",
-		Run: func(cmd *cobra.Command, args []string) {
+		Run: runFunc(func(cmd *cobra.Command, args []string) error {
 			// If printAll is true, flip all the flags.
 			if printAll {
 				printIL = true
@@ -42,7 +42,7 @@ func newDescribeCmd() *cobra.Command {
 				pwd, _ := os.Getwd()
 				pkgpath, err := workspace.DetectPackage(pwd, sink())
 				if err != nil {
-					exitError("could not locate a nut to load: %v", err)
+					return fmt.Errorf("could not locate a package to load: %v", err)
 				}
 
 				if pkg := cmdutil.ReadPackage(pkgpath); pkg != nil {
@@ -58,7 +58,9 @@ func newDescribeCmd() *cobra.Command {
 					printPackage(pkg, printSymbols, printExportedSymbols, printIL)
 				}
 			}
-		},
+
+			return nil
+		}),
 	}
 
 	cmd.PersistentFlags().BoolVarP(

cmd/env.go

@@ -54,21 +54,20 @@ func newEnvCmd() *cobra.Command {
 	return cmd
 }
 
-func initEnvCmd(cmd *cobra.Command, args []string) *envCmdInfo {
-	// Create a new context for the plan operations.
-	ctx := resource.NewContext(sink())
-
+func initEnvCmd(cmd *cobra.Command, args []string) (*envCmdInfo, error) {
 	// Read in the name of the environment to use.
 	if len(args) == 0 {
-		exitError("missing required environment name")
+		return nil, fmt.Errorf("missing required environment name")
 	}
 
 	// Read in the deployment information, bailing if an IO error occurs.
+	ctx := resource.NewContext(sink())
 	name := tokens.QName(args[0])
 	envfile, env, old := readEnv(ctx, name)
 	if env == nil {
 		contract.Assert(!ctx.Diag.Success())
-		exitError("could not read envfile required to proceed") // failure reading the env information.
+		ctx.Close()                                                          // close now, since we are exiting.
+		return nil, fmt.Errorf("could not read envfile required to proceed") // failure reading the env information.
 	}
 	return &envCmdInfo{
 		Ctx:     ctx,
@@ -77,7 +76,7 @@ func initEnvCmd(cmd *cobra.Command, args []string) *envCmdInfo {
 		Old:     old,
 		Args:    args[1:],
 		Orig:    args,
-	}
+	}, nil
 }
 
 type envCmdInfo struct {
@@ -89,6 +88,10 @@ type envCmdInfo struct {
 	Orig    []string          // the original args before extracting the environment name
 }
 
+func (eci *envCmdInfo) Close() error {
+	return eci.Ctx.Close()
+}
+
 func confirmPrompt(msg string, args ...interface{}) bool {
 	prompt := fmt.Sprintf(msg, args...)
 	fmt.Printf(

cmd/env_config.go

@@ -16,8 +16,13 @@ func newEnvConfigCmd() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "config <env> [<key> [value]]",
 		Short: "Query, set, replace, or unset configuration values",
-		Run: func(cmd *cobra.Command, args []string) {
-			info := initEnvCmd(cmd, args)
+		Run: runFunc(func(cmd *cobra.Command, args []string) error {
+			info, err := initEnvCmd(cmd, args)
+			if err != nil {
+				return err
+			}
+			defer info.Close() // ensure we clean up resources before exiting.
+
 			config := info.Env.Config
 			if len(info.Args) == 0 {
 				// If no args were supplied, we are just printing out the current configuration.
@@ -49,11 +54,13 @@ func newEnvConfigCmd() *cobra.Command {
 						// TODO: print complex values.
 						fmt.Printf("%v\n", v)
 					} else {
-						exitError("configuration key '%v' not found for environment '%v'", key, info.Env.Name)
+						return fmt.Errorf("configuration key '%v' not found for environment '%v'", key, info.Env.Name)
 					}
 				}
 			}
-		},
+
+			return nil
+		}),
 	}
 	cmd.PersistentFlags().BoolVar(&unset, "unset", false, "Unset a configuration value")
 	return cmd

cmd/env_deploy.go

@@ -28,8 +28,12 @@ func newEnvDeployCmd() *cobra.Command {
 			"\n" +
 			"By default, the Nut to execute is loaded from the current directory.  Optionally, an\n" +
 			"explicit path can be provided using the [nut] argument.",
-		Run: func(cmd *cobra.Command, args []string) {
-			info := initEnvCmd(cmd, args)
+		Run: runFunc(func(cmd *cobra.Command, args []string) error {
+			info, err := initEnvCmd(cmd, args)
+			if err != nil {
+				return err
+			}
+			defer info.Close()
 			apply(cmd, info, applyOptions{
 				Delete:           false,
 				DryRun:           dryRun,
@@ -39,7 +43,8 @@ func newEnvDeployCmd() *cobra.Command {
 				Summary:          summary,
 				Output:           output,
 			})
-		},
+			return nil
+		}),
 	}
 
 	cmd.PersistentFlags().BoolVarP(

cmd/env_destroy.go

@@ -21,8 +21,12 @@ func newEnvDestroyCmd() *cobra.Command {
 			"\n" +
 			"Warning: although old snapshots can be used to recreate an environment, this command\n" +
 			"is generally irreversable and should be used with great care.",
-		Run: func(cmd *cobra.Command, args []string) {
-			info := initEnvCmd(cmd, args)
+		Run: runFunc(func(cmd *cobra.Command, args []string) error {
+			info, err := initEnvCmd(cmd, args)
+			if err != nil {
+				return err
+			}
+			defer info.Close()
 			if dryRun || yes ||
 				confirmPrompt("This will permanently destroy all resources in the '%v' environment!", info.Env.Name) {
 				apply(cmd, info, applyOptions{
@@ -31,7 +35,8 @@ func newEnvDestroyCmd() *cobra.Command {
 					Summary: summary,
 				})
 			}
-		},
+			return nil
+		}),
 	}
 
 	cmd.PersistentFlags().BoolVarP(

cmd/env_init.go

@@ -3,6 +3,8 @@
 package cmd
 
 import (
+	"errors"
+
 	"github.com/spf13/cobra"
 
 	"github.com/pulumi/coconut/pkg/tokens"
@@ -17,14 +19,15 @@ func newEnvInitCmd() *cobra.Command {
 			"\n" +
 			"This command creates an empty environment with the given name.  It has no resources,\n" +
 			"but afterwards it can become the target of a deployment using the `deploy` command.",
-		Run: func(cmd *cobra.Command, args []string) {
+		Run: runFunc(func(cmd *cobra.Command, args []string) error {
 			// Read in the name of the environment to use.
 			if len(args) == 0 {
-				exitError("missing required environment name")
+				return errors.New("missing required environment name")
 			}
 
 			name := tokens.QName(args[0])
 			create(name)
-		},
+			return nil
+		}),
 	}
 }

cmd/env_ls.go

@@ -23,13 +23,18 @@ func newEnvLsCmd() *cobra.Command {
 		Use:     "ls",
 		Aliases: []string{"list"},
 		Short:   "List all known environments",
-		Run: func(cmd *cobra.Command, args []string) {
+		Run: runFunc(func(cmd *cobra.Command, args []string) error {
+			// Read the environment directory.
 			path := workspace.EnvPath("")
 			files, err := ioutil.ReadDir(path)
 			if err != nil && !os.IsNotExist(err) {
-				exitError("could not read environments: %v", err)
+				return fmt.Errorf("could not read environments: %v", err)
 			}
 
+			// Create a new context to share amongst all of the loads.
+			ctx := resource.NewContext(sink())
+			defer ctx.Close()
+
 			fmt.Printf("%-20s %-48s %-12s\n", "NAME", "LAST DEPLOYMENT", "RESOURCE COUNT")
 			for _, file := range files {
 				// Ignore directories.
@@ -44,9 +49,8 @@ func newEnvLsCmd() *cobra.Command {
 					continue
 				}
 
-				// Create a new context and read in the husk information.
+				// Read in this environment's information.
 				name := tokens.QName(envfn[:len(envfn)-len(ext)])
-				ctx := resource.NewContext(sink())
 				envfile, env, old := readEnv(ctx, name)
 				if env == nil {
 					contract.Assert(!ctx.Diag.Success())
@@ -64,6 +68,8 @@ func newEnvLsCmd() *cobra.Command {
 				}
 				fmt.Printf("%-20s %-48s %-12s\n", env.Name, lastDeploy, resourceCount)
 			}
-		},
+
+			return nil
+		}),
 	}
 }

cmd/env_rm.go

@@ -3,6 +3,8 @@
 package cmd
 
 import (
+	"fmt"
+
 	"github.com/spf13/cobra"
 )
 
@@ -18,17 +20,27 @@ func newEnvRmCmd() *cobra.Command {
 			"`destroy` command for removing a resources, as this is a distinct operation.\n" +
 			"\n" +
 			"After this command completes, the environment will no longer be available for deployments.",
-		Run: func(cmd *cobra.Command, args []string) {
-			info := initEnvCmd(cmd, args)
+		Run: runFunc(func(cmd *cobra.Command, args []string) error {
+			info, err := initEnvCmd(cmd, args)
+			if err != nil {
+				return err
+			}
+			defer info.Close()
+
+			// Don't remove environments that still have resources.
 			if !force && info.Old != nil && len(info.Old.Resources()) > 0 {
-				exitError(
+				return fmt.Errorf(
 					"'%v' still has resources; removal rejected; pass --force to override", info.Env.Name)
 			}
+
+			// Ensure the user really wants to do this.
 			if yes ||
 				confirmPrompt("This will permanently remove the '%v' environment!", info.Env.Name) {
 				remove(info.Env)
 			}
-		},
+
+			return nil
+		}),
 	}
 
 	cmd.PersistentFlags().BoolVarP(

cmd/eval.go

@@ -30,7 +30,7 @@ func newEvalCmd() *cobra.Command {
 			"\n" +
 			"By default, a blueprint package is loaded from the current directory.  Optionally,\n" +
 			"a path to a blueprint elsewhere can be provided as the [blueprint] argument.",
-		Run: func(cmd *cobra.Command, args []string) {
+		Run: runFunc(func(cmd *cobra.Command, args []string) error {
 			// Perform the compilation and, if non-nil is returned, output the graph.
 			if result := compile(cmd, args, nil); result != nil {
 				// Serialize that evaluation graph so that it's suitable for printing/serializing.
@@ -38,7 +38,7 @@ func newEvalCmd() *cobra.Command {
 				if dotOutput {
 					// Convert the output to a DOT file.
 					if err := dotconv.Print(g, os.Stdout); err != nil {
-						exitError("failed to write DOT file to output: %v", err)
+						return fmt.Errorf("failed to write DOT file to output: %v", err)
 					}
 				} else {
 					// Just print a very basic, yet (hopefully) aesthetically pleasinge, ascii-ization of the graph.
@@ -48,7 +48,8 @@ func newEvalCmd() *cobra.Command {
 					}
 				}
 			}
-		},
+			return nil
+		}),
 	}
 
 	cmd.PersistentFlags().BoolVar(

cmd/get.go

@@ -17,9 +17,10 @@ func newGetCmd() *cobra.Command {
 		Long: "Get downloads a Nut by name.  If run without arguments, get will attempt\n" +
 			"to download dependencies referenced by the current Nut.  Otherwise, if one\n" +
 			"or more specific dependencies are provided, only those will be downloaded.",
-		Run: func(cmd *cobra.Command, args []string) {
+		Run: runFunc(func(cmd *cobra.Command, args []string) error {
 			contract.Failf("Get command is not yet implemented")
-		},
+			return nil
+		}),
 	}
 
 	cmd.PersistentFlags().BoolVarP(

cmd/verify.go

@@ -3,6 +3,8 @@
 package cmd
 
 import (
+	"errors"
+
 	"github.com/spf13/cobra"
 )
 
@@ -19,12 +21,13 @@ func newVerifyCmd() *cobra.Command {
 			"The verify command thoroughly checks the NutIL against these rules, and issues\n" +
 			"errors anywhere it doesn't obey them.  This is generally useful for tools developers\n" +
 			"and can ensure that Nuts do not fail at runtime, when such invariants are checked.",
-		Run: func(cmd *cobra.Command, args []string) {
+		Run: runFunc(func(cmd *cobra.Command, args []string) error {
 			// Create a compiler object and perform the verification.
 			if !verify(cmd, args) {
-				exitError("Nut verification failed")
+				return errors.New("verification failed")
 			}
-		},
+			return nil
+		}),
 	}
 
 	return cmd

cmd/version.go

@@ -14,8 +14,9 @@ func newVersionCmd() *cobra.Command {
 	return &cobra.Command{
 		Use:   "version",
 		Short: "Print Coconut's version number",
-		Run: func(cmd *cobra.Command, args []string) {
+		Run: runFunc(func(cmd *cobra.Command, args []string) error {
 			fmt.Printf("Coconut version %v\n", version)
-		},
+			return nil
+		}),
 	}
 }

pkg/resource/context.go

@@ -5,6 +5,8 @@ package resource
 import (
 	"context"
 
+	"github.com/golang/glog"
+
 	"github.com/pulumi/coconut/pkg/diag"
 	"github.com/pulumi/coconut/pkg/eval/rt"
 	"github.com/pulumi/coconut/pkg/tokens"
@@ -60,3 +62,14 @@ func (ctx *Context) Request() context.Context {
 	// TODO: support cancellation.
 	return context.TODO()
 }
+
+// Close reclaims all resources associated with this context.
+func (ctx *Context) Close() error {
+	for _, plugin := range ctx.Plugins {
+		if err := plugin.Close(); err != nil {
+			glog.Infof("Error closing '%v' plugin during shutdown; ignoring: %v", plugin.Pkg(), err)
+		}
+	}
+	ctx.Plugins = make(map[tokens.Package]*Plugin) // empty out the plugin map
+	return nil
+}

pkg/resource/plugin.go

@@ -139,6 +139,8 @@ func execPlugin(name string) (*os.Process, io.WriteCloser, io.ReadCloser, io.Rea
 	return cmd.Process, in, out, err, nil
 }
 
+func (p *Plugin) Pkg() tokens.Package { return p.pkg }
+
 // Check validates that the given property bag is valid for a resource of the given type.
 func (p *Plugin) Check(t tokens.Type, props PropertyMap) ([]CheckFailure, error) {
 	glog.V(7).Infof("Plugin[%v].Check(t=%v,#props=%v) executing", p.pkg, t, len(props))