Clean up provisioned modules on error; refactor Run(); add Validate()

Modules that return an error during provisioning should still be cleaned
up so that they don't leak any resources they may have allocated before
the error occurred. Cleanup should be able to run even if Provision does
not complete fully.

caddy.go

@@ -54,95 +54,10 @@ func Run(newCfg *Config) error {
 	currentCfgMu.Lock()
 	defer currentCfgMu.Unlock()
 
-	if newCfg != nil {
+	// run the new config and start all its apps
-		// because we will need to roll back any state
+	err := run(newCfg, true)
-		// modifications if this function errors, we
+	if err != nil {
-		// keep a single error value and scope all
+		return err
-		// sub-operations to their own functions to
-		// ensure this error value does not get
-		// overridden or missed when it should have
-		// been set by a short assignment
-		var err error
-
-		// prepare the new config for use
-		newCfg.apps = make(map[string]App)
-
-		// create a context within which to load
-		// modules - essentially our new config's
-		// execution environment; be sure that
-		// cleanup occurs when we return if there
-		// was an error; if no error, it will get
-		// cleaned up on next config cycle
-		ctx, cancel := NewContext(Context{Context: context.Background(), cfg: newCfg})
-		defer func() {
-			if err != nil {
-				cancel() // clean up now
-			}
-		}()
-		newCfg.cancelFunc = cancel // clean up later
-
-		// set up storage and make it CertMagic's default storage, too
-		err = func() error {
-			if newCfg.StorageRaw != nil {
-				val, err := ctx.LoadModuleInline("module", "caddy.storage", newCfg.StorageRaw)
-				if err != nil {
-					return fmt.Errorf("loading storage module: %v", err)
-				}
-				stor, err := val.(StorageConverter).CertMagicStorage()
-				if err != nil {
-					return fmt.Errorf("creating storage value: %v", err)
-				}
-				newCfg.storage = stor
-				newCfg.StorageRaw = nil // allow GC to deallocate
-			}
-			if newCfg.storage == nil {
-				newCfg.storage = &certmagic.FileStorage{Path: dataDir()}
-			}
-			certmagic.Default.Storage = newCfg.storage
-
-			return nil
-		}()
-		if err != nil {
-			return err
-		}
-
-		// Load, Provision, Validate each app and their submodules
-		err = func() error {
-			for modName, rawMsg := range newCfg.AppsRaw {
-				val, err := ctx.LoadModule(modName, rawMsg)
-				if err != nil {
-					return fmt.Errorf("loading app module '%s': %v", modName, err)
-				}
-				newCfg.apps[modName] = val.(App)
-			}
-			return nil
-		}()
-		if err != nil {
-			return err
-		}
-
-		// Start
-		err = func() error {
-			var started []string
-			for name, a := range newCfg.apps {
-				err := a.Start()
-				if err != nil {
-					for _, otherAppName := range started {
-						err2 := newCfg.apps[otherAppName].Stop()
-						if err2 != nil {
-							err = fmt.Errorf("%v; additionally, aborting app %s: %v",
-								err, otherAppName, err2)
-						}
-					}
-					return fmt.Errorf("%s app module: start: %v", name, err)
-				}
-				started = append(started, name)
-			}
-			return nil
-		}()
-		if err != nil {
-			return err
-		}
 	}
 
 	// swap old config with the new one
@@ -155,6 +70,110 @@ func Run(newCfg *Config) error {
 	return nil
 }
 
+// run runs newCfg and starts all its apps if
+// start is true. If any errors happen, cleanup
+// is performed if any modules were provisioned;
+// apps that were started already will be stopped,
+// so this function should not leak resources if
+// an error is returned.
+func run(newCfg *Config, start bool) error {
+	if newCfg == nil {
+		return nil
+	}
+
+	// because we will need to roll back any state
+	// modifications if this function errors, we
+	// keep a single error value and scope all
+	// sub-operations to their own functions to
+	// ensure this error value does not get
+	// overridden or missed when it should have
+	// been set by a short assignment
+	var err error
+
+	// prepare the new config for use
+	newCfg.apps = make(map[string]App)
+
+	// create a context within which to load
+	// modules - essentially our new config's
+	// execution environment; be sure that
+	// cleanup occurs when we return if there
+	// was an error; if no error, it will get
+	// cleaned up on next config cycle
+	ctx, cancel := NewContext(Context{Context: context.Background(), cfg: newCfg})
+	defer func() {
+		if err != nil {
+			cancel() // clean up now
+		}
+	}()
+	newCfg.cancelFunc = cancel // clean up later
+
+	// set up storage and make it CertMagic's default storage, too
+	err = func() error {
+		if newCfg.StorageRaw != nil {
+			val, err := ctx.LoadModuleInline("module", "caddy.storage", newCfg.StorageRaw)
+			if err != nil {
+				return fmt.Errorf("loading storage module: %v", err)
+			}
+			stor, err := val.(StorageConverter).CertMagicStorage()
+			if err != nil {
+				return fmt.Errorf("creating storage value: %v", err)
+			}
+			newCfg.storage = stor
+			newCfg.StorageRaw = nil // allow GC to deallocate
+		}
+		if newCfg.storage == nil {
+			newCfg.storage = &certmagic.FileStorage{Path: dataDir()}
+		}
+		certmagic.Default.Storage = newCfg.storage
+
+		return nil
+	}()
+	if err != nil {
+		return err
+	}
+
+	// Load, Provision, Validate each app and their submodules
+	err = func() error {
+		for modName, rawMsg := range newCfg.AppsRaw {
+			val, err := ctx.LoadModule(modName, rawMsg)
+			if err != nil {
+				return fmt.Errorf("loading app module '%s': %v", modName, err)
+			}
+			newCfg.apps[modName] = val.(App)
+		}
+		return nil
+	}()
+	if err != nil {
+		return err
+	}
+
+	if !start {
+		return nil
+	}
+
+	// Start
+	return func() error {
+		var started []string
+		for name, a := range newCfg.apps {
+			err := a.Start()
+			if err != nil {
+				// an app failed to start, so we need to stop
+				// all other apps that were already started
+				for _, otherAppName := range started {
+					err2 := newCfg.apps[otherAppName].Stop()
+					if err2 != nil {
+						err = fmt.Errorf("%v; additionally, aborting app %s: %v",
+							err, otherAppName, err2)
+					}
+				}
+				return fmt.Errorf("%s app module: start: %v", name, err)
+			}
+			started = append(started, name)
+		}
+		return nil
+	}()
+}
+
 // Stop stops running the current configuration.
 // It is the antithesis of Run(). This function
 // will log any errors that occur during the
@@ -168,26 +187,34 @@ func Stop() error {
 	return nil
 }
 
-// unsyncedStop stops oldCfg from running, but if
+// unsyncedStop stops cfg from running, but if
 // applicable, you need to acquire locks yourself.
-// It is a no-op if oldCfg is nil. If any app
+// It is a no-op if cfg is nil. If any app
 // returns an error when stopping, it is logged
 // and the function continues with the next app.
-func unsyncedStop(oldCfg *Config) {
+// This function assumes all apps in cfg were
-	if oldCfg == nil {
+// successfully started.
+func unsyncedStop(cfg *Config) {
+	if cfg == nil {
 		return
 	}
 
 	// stop each app
-	for name, a := range oldCfg.apps {
+	for name, a := range cfg.apps {
 		err := a.Stop()
 		if err != nil {
 			log.Printf("[ERROR] stop %s: %v", name, err)
 		}
 	}
 
-	// clean up all old modules
+	// clean up all modules
-	oldCfg.cancelFunc()
+	cfg.cancelFunc()
+}
+
+// Validate loads, provisions, and validates
+// cfg, but does not start running it.
+func Validate(cfg *Config) error {
+	return run(cfg, false)
 }
 
 // Duration is a JSON-string-unmarshable duration type.

context.go

@@ -131,6 +131,14 @@ func (ctx Context) LoadModule(name string, rawMsg json.RawMessage) (interface{},
 	if prov, ok := val.(Provisioner); ok {
 		err := prov.Provision(ctx)
 		if err != nil {
+			// incomplete provisioning could have left state
+			// dangling, so make sure it gets cleaned up
+			if cleanerUpper, ok := val.(CleanerUpper); ok {
+				err2 := cleanerUpper.Cleanup()
+				if err2 != nil {
+					err = fmt.Errorf("%v; additionally, cleanup: %v", err, err2)
+				}
+			}
 			return nil, fmt.Errorf("provision %s: %v", mod.Name, err)
 		}
 	}
@@ -138,6 +146,7 @@ func (ctx Context) LoadModule(name string, rawMsg json.RawMessage) (interface{},
 	if validator, ok := val.(Validator); ok {
 		err := validator.Validate()
 		if err != nil {
+			// since the module was already provisioned, make sure we clean up
 			if cleanerUpper, ok := val.(CleanerUpper); ok {
 				err2 := cleanerUpper.Cleanup()
 				if err2 != nil {

modules.go

@@ -253,9 +253,10 @@ type Validator interface {
 
 // CleanerUpper is implemented by modules which may have side-effects
 // such as opened files, spawned goroutines, or allocated some sort
-// of non-local state when they were provisioned. This method should
+// of non-stack state when they were provisioned. This method should
 // deallocate/cleanup those resources to prevent memory leaks. Cleanup
-// should be fast and efficient.
+// should be fast and efficient. Cleanup should work even if Provision
+// returns an error, to allow cleaning up from partial provisionings.
 type CleanerUpper interface {
 	Cleanup() error
 }

sigtrap.go

@@ -65,9 +65,9 @@ func gracefulStop(sigName string) {
 	os.Exit(exitCode)
 }
 
-// Exit codes. Generally, you will want to avoid
+// Exit codes. Generally, you should NOT
-// automatically restarting the process if the
+// automatically restart the process if the
-// exit code is 1.
+// exit code is ExitCodeFailedStartup (1).
 const (
 	ExitCodeSuccess = iota
 	ExitCodeFailedStartup