core: Run startup/shutdown functions only once

Even if defined for multiple hosts. Startup or shutdown callbacks registered by any directive (startup, shutdown, markdown, git, log, etc.) will only run as many times as it appears in the Caddyfile, not repeated for each host that shares that server block. Fixing this involved refactoring three packages (yeesh) and we need to restore some tests that are no longer valid (that used to verify splitting a multiServerBlock into multiple serverBlocks).

config/config.go

@@ -23,7 +23,9 @@ const (
 	DefaultConfigFile = "Caddyfile"
 )
 
-func Load(filename string, input io.Reader) ([]server.Config, error) {
+// Load reads input (named filename) and parses it, returning server
+// configurations grouped by listening address.
+func Load(filename string, input io.Reader) (Group, error) {
 	var configs []server.Config
 
 	// turn off timestamp for parsing
@@ -32,60 +34,82 @@ func Load(filename string, input io.Reader) ([]server.Config, error) {
 
 	serverBlocks, err := parse.ServerBlocks(filename, input)
 	if err != nil {
-		return configs, err
+		return nil, err
 	}
 
-	// Each server block represents a single server/address.
+	// Each server block represents one or more servers/addresses.
 	// Iterate each server block and make a config for each one,
 	// executing the directives that were parsed.
 	for _, sb := range serverBlocks {
-		config := server.Config{
-			Host:       sb.Host,
-			Port:       sb.Port,
-			Root:       Root,
-			Middleware: make(map[string][]middleware.Middleware),
-			ConfigFile: filename,
-			AppName:    app.Name,
-			AppVersion: app.Version,
+		sharedConfig, err := serverBlockToConfig(filename, sb)
+		if err != nil {
+			return nil, err
 		}
 
-		// It is crucial that directives are executed in the proper order.
-		for _, dir := range directiveOrder {
-			// Execute directive if it is in the server block
-			if tokens, ok := sb.Tokens[dir.name]; ok {
-				// Each setup function gets a controller, which is the
-				// server config and the dispenser containing only
-				// this directive's tokens.
-				controller := &setup.Controller{
-					Config:    &config,
-					Dispenser: parse.NewDispenserTokens(filename, tokens),
-				}
-
-				midware, err := dir.setup(controller)
-				if err != nil {
-					return configs, err
-				}
-				if midware != nil {
-					// TODO: For now, we only support the default path scope /
-					config.Middleware["/"] = append(config.Middleware["/"], midware)
-				}
+		// Now share the config with as many hosts as share the server block
+		for i, addr := range sb.Addresses {
+			config := sharedConfig
+			config.Host = addr.Host
+			config.Port = addr.Port
+			if config.Port == "" {
+				config.Port = Port
 			}
+			if i == 0 {
+				sharedConfig.Startup = []func() error{}
+				sharedConfig.Shutdown = []func() error{}
+			}
+			configs = append(configs, config)
 		}
-
-		if config.Port == "" {
-			config.Port = Port
-		}
-
-		configs = append(configs, config)
 	}
 
 	// restore logging settings
 	log.SetFlags(flags)
 
-	return configs, nil
+	// Group by address/virtualhosts
+	return arrangeBindings(configs)
 }
 
-// ArrangeBindings groups configurations by their bind address. For example,
+// serverBlockToConfig makes a config for the server block
+// by executing the tokens that were parsed. The returned
+// config is shared among all hosts/addresses for the server
+// block, so Host and Port information is not filled out
+// here.
+func serverBlockToConfig(filename string, sb parse.ServerBlock) (server.Config, error) {
+	sharedConfig := server.Config{
+		Root:       Root,
+		Middleware: make(map[string][]middleware.Middleware),
+		ConfigFile: filename,
+		AppName:    app.Name,
+		AppVersion: app.Version,
+	}
+
+	// It is crucial that directives are executed in the proper order.
+	for _, dir := range directiveOrder {
+		// Execute directive if it is in the server block
+		if tokens, ok := sb.Tokens[dir.name]; ok {
+			// Each setup function gets a controller, which is the
+			// server config and the dispenser containing only
+			// this directive's tokens.
+			controller := &setup.Controller{
+				Config:    &sharedConfig,
+				Dispenser: parse.NewDispenserTokens(filename, tokens),
+			}
+
+			midware, err := dir.setup(controller)
+			if err != nil {
+				return sharedConfig, err
+			}
+			if midware != nil {
+				// TODO: For now, we only support the default path scope /
+				sharedConfig.Middleware["/"] = append(sharedConfig.Middleware["/"], midware)
+			}
+		}
+	}
+
+	return sharedConfig, nil
+}
+
+// arrangeBindings groups configurations by their bind address. For example,
 // a server that should listen on localhost and another on 127.0.0.1 will
 // be grouped into the same address: 127.0.0.1. It will return an error
 // if an address is malformed or a TLS listener is configured on the
@@ -93,8 +117,8 @@ func Load(filename string, input io.Reader) ([]server.Config, error) {
 // bind address to list of configs that would become VirtualHosts on that
 // server. Use the keys of the returned map to create listeners, and use
 // the associated values to set up the virtualhosts.
-func ArrangeBindings(allConfigs []server.Config) (map[*net.TCPAddr][]server.Config, error) {
-	addresses := make(map[*net.TCPAddr][]server.Config)
+func arrangeBindings(allConfigs []server.Config) (Group, error) {
+	addresses := make(Group)
 
 	// Group configs by bind address
 	for _, conf := range allConfigs {
@@ -206,10 +230,7 @@ func validDirective(d string) bool {
 	return false
 }
 
-// Default makes a default configuration which
-// is empty except for root, host, and port,
-// which are essentials for serving the cwd.
-func Default() server.Config {
+func NewDefault() server.Config {
 	return server.Config{
 		Root: Root,
 		Host: Host,
@@ -217,9 +238,18 @@ func Default() server.Config {
 	}
 }
 
+// Default makes a default configuration which
+// is empty except for root, host, and port,
+// which are essentials for serving the cwd.
+func Default() (Group, error) {
+	return arrangeBindings([]server.Config{NewDefault()})
+}
+
 // These three defaults are configurable through the command line
 var (
 	Root = DefaultRoot
 	Host = DefaultHost
 	Port = DefaultPort
 )
+
+type Group map[*net.TCPAddr][]server.Config

config/config_test.go

@@ -6,7 +6,7 @@ import (
 	"github.com/mholt/caddy/server"
 )
 
-func TestReolveAddr(t *testing.T) {
+func TestResolveAddr(t *testing.T) {
 	// NOTE: If tests fail due to comparing to string "127.0.0.1",
 	// it's possible that system env resolves with IPv6, or ::1.
 	// If that happens, maybe we should use actualAddr.IP.IsLoopback()

config/parse/parse.go

@@ -6,7 +6,7 @@ import "io"
 // ServerBlocks parses the input just enough to organize tokens,
 // in order, by server block. No further parsing is performed.
 // Server blocks are returned in the order in which they appear.
-func ServerBlocks(filename string, input io.Reader) ([]serverBlock, error) {
+func ServerBlocks(filename string, input io.Reader) ([]ServerBlock, error) {
 	p := parser{Dispenser: NewDispenser(filename, input)}
 	blocks, err := p.parseAll()
 	return blocks, err

config/parse/parsing.go

@@ -9,34 +9,26 @@ import (
 
 type parser struct {
 	Dispenser
-	block multiServerBlock // current server block being parsed
-	eof   bool             // if we encounter a valid EOF in a hard place
+	block ServerBlock // current server block being parsed
+	eof   bool        // if we encounter a valid EOF in a hard place
 }
 
-func (p *parser) parseAll() ([]serverBlock, error) {
-	var blocks []serverBlock
+func (p *parser) parseAll() ([]ServerBlock, error) {
+	var blocks []ServerBlock
 
 	for p.Next() {
 		err := p.parseOne()
 		if err != nil {
 			return blocks, err
 		}
-
-		// explode the multiServerBlock into multiple serverBlocks
-		for _, addr := range p.block.addresses {
-			blocks = append(blocks, serverBlock{
-				Host:   addr.host,
-				Port:   addr.port,
-				Tokens: p.block.tokens,
-			})
-		}
+		blocks = append(blocks, p.block)
 	}
 
 	return blocks, nil
 }
 
 func (p *parser) parseOne() error {
-	p.block = multiServerBlock{tokens: make(map[string][]token)}
+	p.block = ServerBlock{Tokens: make(map[string][]token)}
 
 	err := p.begin()
 	if err != nil {
@@ -107,7 +99,7 @@ func (p *parser) addresses() error {
 		if err != nil {
 			return err
 		}
-		p.block.addresses = append(p.block.addresses, address{host, port})
+		p.block.Addresses = append(p.block.Addresses, Address{host, port})
 
 		// Advance token and possibly break out of loop or return error
 		hasNext := p.Next()
@@ -229,7 +221,7 @@ func (p *parser) directive() error {
 	}
 
 	// The directive itself is appended as a relevant token
-	p.block.tokens[dir] = append(p.block.tokens[dir], p.tokens[p.cursor])
+	p.block.Tokens[dir] = append(p.block.Tokens[dir], p.tokens[p.cursor])
 
 	for p.Next() {
 		if p.Val() == "{" {
@@ -242,7 +234,7 @@ func (p *parser) directive() error {
 		} else if p.Val() == "}" && nesting == 0 {
 			return p.Err("Unexpected '}' because no matching opening brace")
 		}
-		p.block.tokens[dir] = append(p.block.tokens[dir], p.tokens[p.cursor])
+		p.block.Tokens[dir] = append(p.block.Tokens[dir], p.tokens[p.cursor])
 	}
 
 	if nesting > 0 {
@@ -305,21 +297,15 @@ func standardAddress(str string) (host, port string, err error) {
 }
 
 type (
-	// serverBlock stores tokens by directive name for a
-	// single host:port (address)
-	serverBlock struct {
-		Host, Port string
-		Tokens     map[string][]token // directive name to tokens (including directive)
+	// ServerBlock associates tokens with a list of addresses
+	// and groups tokens by directive name.
+	ServerBlock struct {
+		Addresses []Address
+		Tokens    map[string][]token
 	}
 
-	// multiServerBlock is the same as serverBlock but for
-	// multiple addresses that share the same tokens
-	multiServerBlock struct {
-		addresses []address
-		tokens    map[string][]token
-	}
-
-	address struct {
-		host, port string
+	// Address represents a host and port.
+	Address struct {
+		Host, Port string
 	}
 )

config/parse/parsing_test.go

 package parse
 
 import (
-	"reflect"
 	"strings"
 	"testing"
 )
@@ -60,7 +59,7 @@ func TestStandardAddress(t *testing.T) {
 func TestParseOneAndImport(t *testing.T) {
 	setupParseTests()
 
-	testParseOne := func(input string) (multiServerBlock, error) {
+	testParseOne := func(input string) (ServerBlock, error) {
 		p := testParser(input)
 		p.Next()
 		err := p.parseOne()
@@ -70,22 +69,22 @@ func TestParseOneAndImport(t *testing.T) {
 	for i, test := range []struct {
 		input     string
 		shouldErr bool
-		addresses []address
+		addresses []Address
 		tokens    map[string]int // map of directive name to number of tokens expected
 	}{
-		{`localhost`, false, []address{
+		{`localhost`, false, []Address{
 			{"localhost", ""},
 		}, map[string]int{}},
 
 		{`localhost
-		  dir1`, false, []address{
+		  dir1`, false, []Address{
 			{"localhost", ""},
 		}, map[string]int{
 			"dir1": 1,
 		}},
 
 		{`localhost:1234
-		  dir1 foo bar`, false, []address{
+		  dir1 foo bar`, false, []Address{
 			{"localhost", "1234"},
 		}, map[string]int{
 			"dir1": 3,
@@ -93,7 +92,7 @@ func TestParseOneAndImport(t *testing.T) {
 
 		{`localhost {
 		    dir1
-		  }`, false, []address{
+		  }`, false, []Address{
 			{"localhost", ""},
 		}, map[string]int{
 			"dir1": 1,
@@ -102,7 +101,7 @@ func TestParseOneAndImport(t *testing.T) {
 		{`localhost:1234 {
 		    dir1 foo bar
 		    dir2
-		  }`, false, []address{
+		  }`, false, []Address{
 			{"localhost", "1234"},
 		}, map[string]int{
 			"dir1": 3,
@@ -110,7 +109,7 @@ func TestParseOneAndImport(t *testing.T) {
 		}},
 
 		{`http://localhost https://localhost
-		  dir1 foo bar`, false, []address{
+		  dir1 foo bar`, false, []Address{
 			{"localhost", "http"},
 			{"localhost", "https"},
 		}, map[string]int{
@@ -119,7 +118,7 @@ func TestParseOneAndImport(t *testing.T) {
 
 		{`http://localhost https://localhost {
 		    dir1 foo bar
-		  }`, false, []address{
+		  }`, false, []Address{
 			{"localhost", "http"},
 			{"localhost", "https"},
 		}, map[string]int{
@@ -128,7 +127,7 @@ func TestParseOneAndImport(t *testing.T) {
 
 		{`http://localhost, https://localhost {
 		    dir1 foo bar
-		  }`, false, []address{
+		  }`, false, []Address{
 			{"localhost", "http"},
 			{"localhost", "https"},
 		}, map[string]int{
@@ -136,13 +135,13 @@ func TestParseOneAndImport(t *testing.T) {
 		}},
 
 		{`http://localhost, {
-		  }`, true, []address{
+		  }`, true, []Address{
 			{"localhost", "http"},
 		}, map[string]int{}},
 
 		{`host1:80, http://host2.com
 		  dir1 foo bar
-		  dir2 baz`, false, []address{
+		  dir2 baz`, false, []Address{
 			{"host1", "80"},
 			{"host2.com", "http"},
 		}, map[string]int{
@@ -152,7 +151,7 @@ func TestParseOneAndImport(t *testing.T) {
 
 		{`http://host1.com,
 		  http://host2.com,
-		  https://host3.com`, false, []address{
+		  https://host3.com`, false, []Address{
 			{"host1.com", "http"},
 			{"host2.com", "http"},
 			{"host3.com", "https"},
@@ -162,7 +161,7 @@ func TestParseOneAndImport(t *testing.T) {
 		  dir1 foo {
 		    bar baz
 		  }
-		  dir2`, false, []address{
+		  dir2`, false, []Address{
 			{"host1.com", "1234"},
 			{"host2.com", "https"},
 		}, map[string]int{
@@ -176,7 +175,7 @@ func TestParseOneAndImport(t *testing.T) {
 		  }
 		  dir2 {
 		    foo bar
-		  }`, false, []address{
+		  }`, false, []Address{
 			{"127.0.0.1", ""},
 		}, map[string]int{
 			"dir1": 5,
@@ -184,13 +183,13 @@ func TestParseOneAndImport(t *testing.T) {
 		}},
 
 		{`127.0.0.1
-		  unknown_directive`, true, []address{
+		  unknown_directive`, true, []Address{
 			{"127.0.0.1", ""},
 		}, map[string]int{}},
 
 		{`localhost
 		  dir1 {
-		    foo`, true, []address{
+		    foo`, true, []Address{
 			{"localhost", ""},
 		}, map[string]int{
 			"dir1": 3,
@@ -198,7 +197,7 @@ func TestParseOneAndImport(t *testing.T) {
 
 		{`localhost
 		  dir1 {
-		  }`, false, []address{
+		  }`, false, []Address{
 			{"localhost", ""},
 		}, map[string]int{
 			"dir1": 3,
@@ -210,18 +209,18 @@ func TestParseOneAndImport(t *testing.T) {
 		      foo
 		    }
 		  }
-		  dir2 foo bar`, false, []address{
+		  dir2 foo bar`, false, []Address{
 			{"localhost", ""},
 		}, map[string]int{
 			"dir1": 7,
 			"dir2": 3,
 		}},
 
-		{``, false, []address{}, map[string]int{}},
+		{``, false, []Address{}, map[string]int{}},
 
 		{`localhost
 		  dir1 arg1
-		  import import_test1.txt`, false, []address{
+		  import import_test1.txt`, false, []Address{
 			{"localhost", ""},
 		}, map[string]int{
 			"dir1": 2,
@@ -229,7 +228,7 @@ func TestParseOneAndImport(t *testing.T) {
 			"dir3": 1,
 		}},
 
-		{`import import_test2.txt`, false, []address{
+		{`import import_test2.txt`, false, []Address{
 			{"host1", ""},
 		}, map[string]int{
 			"dir1": 1,
@@ -245,28 +244,28 @@ func TestParseOneAndImport(t *testing.T) {
 			t.Errorf("Test %d: Expected no error, but got: %v", i, err)
 		}
 
-		if len(result.addresses) != len(test.addresses) {
+		if len(result.Addresses) != len(test.addresses) {
 			t.Errorf("Test %d: Expected %d addresses, got %d",
-				i, len(test.addresses), len(result.addresses))
+				i, len(test.addresses), len(result.Addresses))
 			continue
 		}
-		for j, addr := range result.addresses {
-			if addr.host != test.addresses[j].host {
+		for j, addr := range result.Addresses {
+			if addr.Host != test.addresses[j].Host {
 				t.Errorf("Test %d, address %d: Expected host to be '%s', but was '%s'",
-					i, j, test.addresses[j].host, addr.host)
+					i, j, test.addresses[j].Host, addr.Host)
 			}
-			if addr.port != test.addresses[j].port {
+			if addr.Port != test.addresses[j].Port {
 				t.Errorf("Test %d, address %d: Expected port to be '%s', but was '%s'",
-					i, j, test.addresses[j].port, addr.port)
+					i, j, test.addresses[j].Port, addr.Port)
 			}
 		}
 
-		if len(result.tokens) != len(test.tokens) {
+		if len(result.Tokens) != len(test.tokens) {
 			t.Errorf("Test %d: Expected %d directives, had %d",
-				i, len(test.tokens), len(result.tokens))
+				i, len(test.tokens), len(result.Tokens))
 			continue
 		}
-		for directive, tokens := range result.tokens {
+		for directive, tokens := range result.Tokens {
 			if len(tokens) != test.tokens[directive] {
 				t.Errorf("Test %d, directive '%s': Expected %d tokens, counted %d",
 					i, directive, test.tokens[directive], len(tokens))
@@ -276,96 +275,6 @@ func TestParseOneAndImport(t *testing.T) {
 	}
 }
 
-func TestParseAll(t *testing.T) {
-	setupParseTests()
-
-	for i, test := range []struct {
-		input     string
-		shouldErr bool
-		addresses []address // one per expected server block, in order
-	}{
-		{`localhost`, false, []address{
-			{"localhost", ""},
-		}},
-
-		{`localhost:1234`, false, []address{
-			{"localhost", "1234"},
-		}},
-
-		{`localhost:1234 {
-		  }
-		  localhost:2015 {
-		  }`, false, []address{
-			{"localhost", "1234"},
-			{"localhost", "2015"},
-		}},
-
-		{`localhost:1234, http://host2`, false, []address{
-			{"localhost", "1234"},
-			{"host2", "http"},
-		}},
-
-		{`localhost:1234, http://host2,`, true, []address{}},
-
-		{`http://host1.com, http://host2.com {
-		  }
-		  https://host3.com, https://host4.com {
-		  }`, false, []address{
-			{"host1.com", "http"},
-			{"host2.com", "http"},
-			{"host3.com", "https"},
-			{"host4.com", "https"},
-		}},
-	} {
-		p := testParser(test.input)
-		blocks, err := p.parseAll()
-
-		if test.shouldErr && err == nil {
-			t.Errorf("Test %d: Expected an error, but didn't get one", i)
-		}
-		if !test.shouldErr && err != nil {
-			t.Errorf("Test %d: Expected no error, but got: %v", i, err)
-		}
-
-		if len(blocks) != len(test.addresses) {
-			t.Errorf("Test %d: Expected %d server blocks, got %d",
-				i, len(test.addresses), len(blocks))
-			continue
-		}
-		for j, block := range blocks {
-			if block.Host != test.addresses[j].host {
-				t.Errorf("Test %d, block %d: Expected host to be '%s', but was '%s'",
-					i, j, test.addresses[j].host, block.Host)
-			}
-			if block.Port != test.addresses[j].port {
-				t.Errorf("Test %d, block %d: Expected port to be '%s', but was '%s'",
-					i, j, test.addresses[j].port, block.Port)
-			}
-		}
-	}
-
-	// Exploding the server blocks that have more than one address should replicate/share tokens
-	p := testParser(`host1 {
-	                   dir1 foo bar
-	                 }
-
-	                 host2, host3 {
-	                   dir2 foo bar
-	                   dir3 foo {
-	                     bar
-	                   }
-	                 }`)
-	blocks, err := p.parseAll()
-	if err != nil {
-		t.Fatalf("Expected there to not be an error, but there was: %v", err)
-	}
-
-	if !reflect.DeepEqual(blocks[1].Tokens, blocks[2].Tokens) {
-		t.Errorf("Expected host2 and host3 to have same tokens, but they didn't.\nhost2 Block: %v\nhost3 Block: %v",
-			blocks[1].Tokens, blocks[2].Tokens)
-	}
-}
-
 func setupParseTests() {
 	// Set up some bogus directives for testing
 	ValidDirectives = map[string]struct{}{

main.go

@@ -49,14 +49,8 @@ func main() {
 		log.Fatal(err)
 	}
 
-	// Load config from file
-	allConfigs, err := loadConfigs()
-	if err != nil {
-		log.Fatal(err)
-	}
-
-	// Group by address (virtual hosts)
-	addresses, err := config.ArrangeBindings(allConfigs)
+	// Load address configurations from highest priority input
+	addresses, err := loadConfigs()
 	if err != nil {
 		log.Fatal(err)
 	}
@@ -129,16 +123,17 @@ func isLocalhost(s string) bool {
 }
 
 // loadConfigs loads configuration from a file or stdin (piped).
+// The configurations are grouped by bind address.
 // Configuration is obtained from one of three sources, tried
 // in this order: 1. -conf flag, 2. stdin, 3. Caddyfile.
 // If none of those are available, a default configuration is
 // loaded.
-func loadConfigs() ([]server.Config, error) {
+func loadConfigs() (config.Group, error) {
 	// -conf flag
 	if conf != "" {
 		file, err := os.Open(conf)
 		if err != nil {
-			return []server.Config{}, err
+			return nil, err
 		}
 		defer file.Close()
 		return config.Load(path.Base(conf), file)
@@ -165,9 +160,9 @@ func loadConfigs() ([]server.Config, error) {
 	file, err := os.Open(config.DefaultConfigFile)
 	if err != nil {
 		if os.IsNotExist(err) {
-			return []server.Config{config.Default()}, nil
+			return config.Default()
 		}
-		return []server.Config{}, err
+		return nil, err
 	}
 	defer file.Close()
 

server/server.go

@@ -83,18 +83,23 @@ func (s *Server) Serve() error {
 
 		// Execute shutdown commands on exit
 		if len(vh.config.Shutdown) > 0 {
-			go func() {
+			go func(vh virtualHost) {
+				// Wait for signal
 				interrupt := make(chan os.Signal, 1)
-				signal.Notify(interrupt, os.Interrupt, os.Kill) // TODO: syscall.SIGQUIT? (Ctrl+\, Unix-only)
+				signal.Notify(interrupt, os.Interrupt, os.Kill)
 				<-interrupt
+
+				// Run callbacks
+				exitCode := 0
 				for _, shutdownFunc := range vh.config.Shutdown {
 					err := shutdownFunc()
 					if err != nil {
-						log.Fatal(err)
+						exitCode = 1
+						log.Println(err)
 					}
 				}
-				os.Exit(0)
-			}()
+				os.Exit(exitCode) // BUG: Other shutdown goroutines might be running; use sync.WaitGroup
+			}(vh)
 		}
 	}