Much refactor; many fix; so wow

Fixed pidfile writing problem where a pidfile would be written even if child failed, also cleaned up restarts a bit and fixed a few bugs, it's more robust now in case of failures and with logging.

caddy/caddy.go

@@ -59,7 +59,7 @@ var (
 
 	// errIncompleteRestart occurs if this process is a fork
 	// of the parent but no Caddyfile was piped in
-	errIncompleteRestart = errors.New("cannot finish restart successfully")
+	errIncompleteRestart = errors.New("incomplete restart")
 
 	// servers is a list of all the currently-listening servers
 	servers []*server.Server
@@ -103,17 +103,18 @@ const (
 // In any case, an error is returned if Caddy could not be
 // started.
 func Start(cdyfile Input) (err error) {
-	// When we return, tell the parent whether we started
-	// successfully, and if so, write the pidfile (if enabled)
+	// If we return with no errors, we must do two things: tell the
+	// parent that we succeeded and write to the pidfile.
 	defer func() {
-		success := err == nil
-		signalParent(success)
-		if success && PidFile != "" {
+		if err == nil {
+			signalSuccessToParent() // TODO: Is doing this more than once per process a bad idea? Start could get called more than once in other apps.
+			if PidFile != "" {
 				err := writePidFile()
 				if err != nil {
 					log.Printf("[ERROR] Could not write pidfile: %v", err)
 				}
 			}
+		}
 	}()
 
 	// Input must never be nil; try to load something

caddy/helpers.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"io/ioutil"
+	"log"
 	"os"
 	"os/exec"
 	"runtime"
@@ -39,16 +40,16 @@ func checkFdlimit() {
 	}
 }
 
-// signalParent tells the parent our status using pipe at index 3.
+// signalSuccessToParent tells the parent our status using pipe at index 3.
 // If this process is not a restart, this function does nothing.
-// Calling this is vital so that the parent process can unblock and
-// either continue running or kill itself.
-func signalParent(success bool) {
+// Calling this function once this process has successfully initialized
+// is vital so that the parent process can unblock and kill itself.
+func signalSuccessToParent() {
 	if IsRestart() {
-		ppipe := os.NewFile(3, "") // parent is listening on pipe at index 3
-		if success {
-			// Tell parent process that we're OK so it can quit now
-			ppipe.Write([]byte("success"))
+		ppipe := os.NewFile(3, "")               // parent is reading from pipe at index 3
+		_, err := ppipe.Write([]byte("success")) // we must send some bytes to the parent
+		if err != nil {
+			log.Printf("[ERROR] Communicating successful init to parent: %v", err)
 		}
 		ppipe.Close()
 	}

caddy/restart.go

@@ -8,7 +8,6 @@ import (
 	"log"
 	"os"
 	"os/exec"
-	"syscall"
 )
 
 func init() {
@@ -54,56 +53,56 @@ func Restart(newCaddyfile Input) error {
 	}
 
 	// Prepare a pipe that the child process will use to communicate
-	// its success or failure with us, the parent
+	// its success with us by sending > 0 bytes
 	sigrpipe, sigwpipe, err := os.Pipe()
 	if err != nil {
 		return err
 	}
 
-	// Pass along current environment and file descriptors to child.
-	// Ordering here is very important: stdin, stdout, stderr, sigpipe,
-	// and then the listener file descriptors (in order).
-	fds := []uintptr{rpipe.Fd(), os.Stdout.Fd(), os.Stderr.Fd(), sigwpipe.Fd()}
+	// Pass along relevant file descriptors to child process; ordering
+	// is very important since we rely on these being in certain positions.
+	extraFiles := []*os.File{sigwpipe}
 
-	// Now add file descriptors of the sockets
+	// Add file descriptors of all the sockets
 	serversMu.Lock()
 	for i, s := range servers {
-		fds = append(fds, s.ListenerFd())
+		extraFiles = append(extraFiles, s.ListenerFd())
 		cdyfileGob.ListenerFds[s.Addr] = uintptr(4 + i) // 4 fds come before any of the listeners
 	}
 	serversMu.Unlock()
 
-	// We're gonna need the proper path to the executable
-	exepath, err := exec.LookPath(os.Args[0])
+	// Set up the command
+	cmd := exec.Command(os.Args[0], os.Args[1:]...)
+	cmd.Stdin = rpipe      // fd 0
+	cmd.Stdout = os.Stdout // fd 1
+	cmd.Stderr = os.Stderr // fd 2
+	cmd.ExtraFiles = extraFiles
+
+	// Spawn the child process
+	err = cmd.Start()
 	if err != nil {
 		return err
 	}
 
-	// Fork the process with the current environment and file descriptors
-	execSpec := &syscall.ProcAttr{
-		Env:   os.Environ(),
-		Files: fds,
-	}
-	_, err = syscall.ForkExec(exepath, os.Args, execSpec)
-	if err != nil {
-		return err
-	}
-
-	// Feed it the Caddyfile
+	// Feed Caddyfile to the child
 	err = gob.NewEncoder(wpipe).Encode(cdyfileGob)
 	if err != nil {
 		return err
 	}
 	wpipe.Close()
 
-	// Wait for child process to signal success or fail
-	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("[ERROR] Restart: child failed to initialize; changes not applied")
+	// Determine whether child startup succeeded
+	sigwpipe.Close() // close our copy of the write end of the pipe -- TODO: why?
+	answer, readErr := ioutil.ReadAll(sigrpipe)
+	if answer == nil || len(answer) == 0 {
+		cmdErr := cmd.Wait() // get exit status
+		log.Printf("[ERROR] Restart: child failed to initialize (%v) - changes not applied", cmdErr)
+		if readErr != nil {
+			log.Printf("[ERROR] Restart: additionally, error communicating with child process: %v", readErr)
+		}
 		return errIncompleteRestart
 	}
 
-	// Child process is listening now; we can stop all our servers here.
+	// Looks like child was successful; we can exit gracefully.
 	return Stop()
 }

caddy/sigtrap_posix.go

@@ -19,6 +19,8 @@ func init() {
 		for {
 			<-reload
 
+			log.Println("[INFO] SIGUSR1: Reloading")
+
 			var updatedCaddyfile Input
 
 			caddyfileMu.Lock()
@@ -42,7 +44,7 @@ func init() {
 
 			err := Restart(updatedCaddyfile)
 			if err != nil {
-				log.Printf("[ERROR] SIGUSR1: Restart returned: %v", err)
+				log.Printf("[ERROR] SIGUSR1: %v", err)
 			}
 		}
 	}()

main.go

@@ -30,7 +30,7 @@ const (
 
 func init() {
 	flag.BoolVar(&letsencrypt.Agreed, "agree", false, "Agree to Let's Encrypt Subscriber Agreement")
-	flag.StringVar(&letsencrypt.CAUrl, "ca", "https://acme-staging.api.letsencrypt.org", "Certificate authority ACME server")
+	flag.StringVar(&letsencrypt.CAUrl, "ca", "https://acme-staging.api.letsencrypt.org/directory", "Certificate authority ACME server")
 	flag.StringVar(&conf, "conf", "", "Configuration file to use (default="+caddy.DefaultConfigFile+")")
 	flag.StringVar(&cpu, "cpu", "100%", "CPU cap")
 	flag.StringVar(&letsencrypt.DefaultEmail, "email", "", "Default Let's Encrypt account email address")
@@ -62,7 +62,7 @@ func main() {
 	default:
 		file, err := os.OpenFile(logfile, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0644)
 		if err != nil {
-			log.Fatalf("Error opening log file: %v", err)
+			log.Fatalf("Error opening process log file: %v", err)
 		}
 		log.SetOutput(file)
 	}
@@ -95,22 +95,23 @@ func main() {
 	// Start your engines
 	err = caddy.Start(caddyfile)
 	if err != nil {
-		if caddy.IsRestart() {
-			log.Printf("[ERROR] Upon starting %s: %v", appName, err)
-		} else {
 		mustLogFatal(err)
 	}
-	}
 
 	// Twiddle your thumbs
 	caddy.Wait()
 }
 
 // mustLogFatal just wraps log.Fatal() in a way that ensures the
-// output is always printed to stderr so the user can see it,
-// even if the process log was not enabled.
+// output is always printed to stderr so the user can see it
+// if the user is still there, even if the process log was not
+// enabled. If this process is a restart, however, and the user
+// might not be there anymore, this just logs to the process log
+// and exits.
 func mustLogFatal(args ...interface{}) {
+	if !caddy.IsRestart() {
 		log.SetOutput(os.Stderr)
+	}
 	log.Fatal(args...)
 }
 

server/server.go

@@ -280,14 +280,11 @@ func (s *Server) WaitUntilStarted() {
 }
 
 // ListenerFd gets the file descriptor of the listener.
-func (s *Server) ListenerFd() uintptr {
+func (s *Server) ListenerFd() *os.File {
 	s.listenerMu.Lock()
 	defer s.listenerMu.Unlock()
-	file, err := s.listener.File()
-	if err != nil {
-		return 0
-	}
-	return file.Fd()
+	file, _ := s.listener.File()
+	return file
 }
 
 // ServeHTTP is the entry point for every request to the address that s