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.

4 files changed, 136 insertions, 99 deletions

diff --git a/caddy.go b/caddy.go
index b91ae79fe..fb3e488dd 100644
--- a/caddy.go
+++ b/caddy.go
@@ -54,105 +54,124 @@ func Run(newCfg *Config) error {
 	currentCfgMu.Lock()
 	defer currentCfgMu.Unlock()
 
-	if newCfg != 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
+	// run the new config and start all its apps
+	err := run(newCfg, true)
+	if err != nil {
+		return err
+	}
 
-			return nil
-		}()
+	// swap old config with the new one
+	oldCfg := currentCfg
+	currentCfg = newCfg
+
+	// Stop, Cleanup each old app
+	unsyncedStop(oldCfg)
+
+	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 {
-			return err
+			cancel() // clean up now
 		}
+	}()
+	newCfg.cancelFunc = cancel // clean up later
 
-		// 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)
+	// 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)
 			}
-			return nil
-		}()
-		if err != nil {
-			return 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
 
-		// 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
+	}
+
+	// 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)
 			}
-			return nil
-		}()
-		if err != nil {
-			return err
+			newCfg.apps[modName] = val.(App)
 		}
+		return nil
+	}()
+	if err != nil {
+		return err
 	}
 
-	// swap old config with the new one
-	oldCfg := currentCfg
-	currentCfg = newCfg
-
-	// Stop, Cleanup each old app
-	unsyncedStop(oldCfg)
+	if !start {
+		return nil
+	}
 
-	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.
@@ -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) {
-	if oldCfg == nil {
+// This function assumes all apps in cfg were
+// 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
-	oldCfg.cancelFunc()
+	// clean up all modules
+	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.
diff --git a/context.go b/context.go
index 2fd84d5f4..c29aa1d13 100644
--- a/context.go
+++ b/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 {
diff --git a/modules.go b/modules.go
index 3ca53841b..2d01eb3b5 100644
--- a/modules.go
+++ b/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
 }
diff --git a/sigtrap.go b/sigtrap.go
index 8fae706ad..76bb6664a 100644
--- a/sigtrap.go
+++ b/sigtrap.go
@@ -65,9 +65,9 @@ func gracefulStop(sigName string) {
 	os.Exit(exitCode)
 }
 
-// Exit codes. Generally, you will want to avoid
-// automatically restarting the process if the
-// exit code is 1.
+// Exit codes. Generally, you should NOT
+// automatically restart the process if the
+// exit code is ExitCodeFailedStartup (1).
 const (
 	ExitCodeSuccess = iota
 	ExitCodeFailedStartup