Clean up leaking goroutines and safer Start()/Stop()

caddy/caddy.go

@@ -26,6 +26,7 @@ import (
 	"strings"
 	"sync"
 
+	"github.com/mholt/caddy/caddy/letsencrypt"
 	"github.com/mholt/caddy/server"
 )
 
@@ -90,6 +91,8 @@ const (
 // In any case, an error is returned if Caddy could not be
 // started.
 func Start(cdyfile Input) error {
+	// TODO: What if already started -- is that an error?
+
 	var err error
 
 	// Input must never be nil; try to load something
@@ -104,7 +107,20 @@ func Start(cdyfile Input) error {
 	caddyfile = cdyfile
 	caddyfileMu.Unlock()
 
-	groupings, err := Load(path.Base(cdyfile.Path()), bytes.NewReader(cdyfile.Body()))
+	// load the server configs
+	configs, err := load(path.Base(cdyfile.Path()), bytes.NewReader(cdyfile.Body()))
+	if err != nil {
+		return err
+	}
+
+	// secure all the things
+	configs, err = letsencrypt.Activate(configs)
+	if err != nil {
+		return err
+	}
+
+	// group virtualhosts by address
+	groupings, err := arrangeBindings(configs)
 	if err != nil {
 		return err
 	}
@@ -217,11 +233,15 @@ func startServers(groupings Group) error {
 
 // Stop stops all servers. It blocks until they are all stopped.
 func Stop() error {
+	letsencrypt.Deactivate()
+
 	serversMu.Lock()
 	for _, s := range servers {
 		s.Stop() // TODO: error checking/reporting?
 	}
+	servers = []*server.Server{} // don't reuse servers
 	serversMu.Unlock()
+
 	return nil
 }
 

caddy/config.go

@@ -7,7 +7,6 @@ import (
 	"net"
 	"sync"
 
-	"github.com/mholt/caddy/caddy/letsencrypt"
 	"github.com/mholt/caddy/caddy/parse"
 	"github.com/mholt/caddy/caddy/setup"
 	"github.com/mholt/caddy/middleware"
@@ -20,9 +19,9 @@ const (
 	DefaultConfigFile = "Caddyfile"
 )
 
-// Load reads input (named filename) and parses it, returning server
-// configurations grouped by listening address.
-func Load(filename string, input io.Reader) (Group, error) {
+// load reads input (named filename) and parses it, returning the
+// server configurations in the order they appeared in the input.
+func load(filename string, input io.Reader) ([]server.Config, error) {
 	var configs []server.Config
 
 	// turn off timestamp for parsing
@@ -34,7 +33,7 @@ func Load(filename string, input io.Reader) (Group, error) {
 		return nil, err
 	}
 	if len(serverBlocks) == 0 {
-		return Default()
+		return []server.Config{NewDefault()}, nil
 	}
 
 	// Each server block represents similar hosts/addresses.
@@ -101,14 +100,7 @@ func Load(filename string, input io.Reader) (Group, error) {
 	// restore logging settings
 	log.SetFlags(flags)
 
-	// secure all the things
-	configs, err = letsencrypt.Activate(configs)
-	if err != nil {
-		return nil, err
-	}
-
-	// group by address/virtualhosts
-	return arrangeBindings(configs)
+	return configs, nil
 }
 
 // makeOnces makes a map of directive name to sync.Once
@@ -271,12 +263,6 @@ func NewDefault() server.Config {
 	}
 }
 
-// Default obtains a default config and arranges
-// bindings so it's ready to use.
-func Default() (Group, error) {
-	return arrangeBindings([]server.Config{NewDefault()})
-}
-
 // These defaults are configurable through the command line
 var (
 	// Site root

caddy/letsencrypt/letsencrypt.go

@@ -36,6 +36,9 @@ var OnRenew func() error
 // argument. If absent, it will use the most recent email
 // address from last time. If there isn't one, the user
 // will be prompted. If the user leaves email blank, <TODO>.
+//
+// Also note that calling this function activates asset
+// management automatically, which <TODO>.
 func Activate(configs []server.Config) ([]server.Config, error) {
 	// First identify and configure any elligible hosts for which
 	// we already have certs and keys in storage from last time.
@@ -47,7 +50,7 @@ func Activate(configs []server.Config) ([]server.Config, error) {
 	}
 
 	// First renew any existing certificates that need it
-	processCertificateRenewal(configs)
+	renewCertificates(configs)
 
 	// Group configs by LE email address; this will help us
 	// reduce round-trips when getting the certs.
@@ -83,11 +86,26 @@ func Activate(configs []server.Config) ([]server.Config, error) {
 		}
 	}
 
-	go keepCertificatesRenewed(configs)
+	stopChan = make(chan struct{})
+	go maintainAssets(configs, stopChan)
 
 	return configs, nil
 }
 
+// Deactivate cleans up long-term, in-memory resources
+// allocated by calling Activate(). Essentially, it stops
+// the asset maintainer from running, meaning that certificates
+// will not be renewed, OCSP staples will not be updated, etc.
+func Deactivate() (err error) {
+	defer func() {
+		if rec := recover(); rec != nil {
+			err = errors.New("already deactivated")
+		}
+	}()
+	close(stopChan)
+	return
+}
+
 // groupConfigsByEmail groups configs by the Let's Encrypt email address
 // associated to them or to the default Let's Encrypt email address. If the
 // default email is not available, the user will be prompted to provide one.
@@ -360,6 +378,9 @@ const (
 
 	// How often to check certificates for renewal
 	renewInterval = 24 * time.Hour
+
+	// How often to update OCSP stapling
+	ocspInterval = 1 * time.Hour
 )
 
 // KeySize represents the length of a key in bits.
@@ -377,3 +398,7 @@ const (
 // This shouldn't need to change except for in tests;
 // the size can be drastically reduced for speed.
 var rsaKeySizeToUse = RSA_2048
+
+// stopChan is used to signal the maintenance goroutine
+// to terminate.
+var stopChan chan struct{}

caddy/letsencrypt/renew.go → caddy/letsencrypt/maintain.go

@@ -10,33 +10,49 @@ import (
 	"github.com/xenolf/lego/acme"
 )
 
-// keepCertificatesRenewed is a permanently-blocking function
+// maintainAssets is a permanently-blocking function
 // that loops indefinitely and, on a regular schedule, checks
 // certificates for expiration and initiates a renewal of certs
-// that are expiring soon.
-func keepCertificatesRenewed(configs []server.Config) {
-	ticker := time.Tick(renewInterval)
-	for range ticker {
-		if n, errs := processCertificateRenewal(configs); len(errs) > 0 {
-			for _, err := range errs {
-				log.Printf("[ERROR] cert renewal: %v\n", err)
-			}
-			if n > 0 && OnRenew != nil {
-				err := OnRenew()
-				if err != nil {
-					log.Printf("[ERROR] onrenew callback: %v\n", err)
+// that are expiring soon. It also updates OCSP stapling and
+// performs other maintenance of assets.
+//
+// You must pass in the server configs to maintain and the channel
+// which you'll close when maintenance should stop, to allow this
+// goroutine to clean up after itself.
+func maintainAssets(configs []server.Config, stopChan chan struct{}) {
+	renewalTicker := time.NewTicker(renewInterval)
+	ocspTicker := time.NewTicker(ocspInterval)
+
+	for {
+		select {
+		case <-renewalTicker.C:
+			if n, errs := renewCertificates(configs); len(errs) > 0 {
+				for _, err := range errs {
+					log.Printf("[ERROR] cert renewal: %v\n", err)
+				}
+				if n > 0 && OnRenew != nil {
+					err := OnRenew()
+					if err != nil {
+						log.Printf("[ERROR] onrenew callback: %v\n", err)
+					}
 				}
 			}
+		case <-ocspTicker.C:
+			// TODO: Update OCSP
+		case <-stopChan:
+			renewalTicker.Stop()
+			ocspTicker.Stop()
+			return
 		}
 	}
 }
 
-// checkCertificateRenewal loops through all configured
-// sites and looks for certificates to renew. Nothing is mutated
+// renewCertificates loops through all configured site and
+// looks for certificates to renew. Nothing is mutated
 // through this function. The changes happen directly on disk.
 // It returns the number of certificates renewed and any errors
-// that occurred.
-func processCertificateRenewal(configs []server.Config) (int, []error) {
+// that occurred. It only performs a renewal if necessary.
+func renewCertificates(configs []server.Config) (int, []error) {
 	log.Print("[INFO] Processing certificate renewals...")
 	var errs []error
 	var n int

caddy/restart.go

@@ -81,7 +81,7 @@ func Restart(newCaddyfile Input) error {
 	wpipe.Close()
 
 	// Wait for child process to signal success or fail
-	sigwpipe.Close() // close our copy of the write end of the pipe
+	sigwpipe.Close() // close our copy of the write end of the pipe or we might be stuck
 	answer, err := ioutil.ReadAll(sigrpipe)
 	if err != nil || len(answer) == 0 {
 		log.Println("restart: child failed to answer; changes not applied")

server/server.go

@@ -66,6 +66,7 @@ func New(addr string, configs []Config) (*Server, error) {
 	// into sync.WaitGroup.Wait() - basically, an add
 	// with a positive delta must be guaranteed to
 	// occur before Wait() is called on the wg.
+	// In a way, this kind of acts as a safety barrier.
 	s.httpWg.Add(1)
 
 	// Set up each virtualhost
@@ -228,12 +229,12 @@ func (s *Server) Stop() error {
 		// Wait for remaining connections to finish or
 		// force them all to close after timeout
 		select {
-		case <-time.After(5 * time.Second): // TODO: configurable?
+		case <-time.After(5 * time.Second): // TODO: make configurable?
 		case <-done:
 		}
 	}
 
-	// Close the listener now; this stops the server and
+	// Close the listener now; this stops the server without delay
 	s.listenerMu.Lock()
 	err := s.listener.Close()
 	s.listenerMu.Unlock()