Epic revert of 0ac8bf58 and adding OncePerServerBlock

Turns out having each server block share a single server.Config during initialization when the Setup functions are being called was a bad idea. Sure, startup and shutdown functions were only executed once, but they had no idea what their hostname or port was. So here we revert to the old way of doing things where Setup may be called multiple times per server block (once per host associated with the block, to be precise), but the Setup functions now know their host and port since the config belongs to exactly one virtualHost. To have something happen just once per server block, use OncePerServerBlock, a new function available on each Controller.

config/config.go

@@ -5,6 +5,7 @@ import (
 	"io"
 	"log"
 	"net"
+	"sync"
 
 	"github.com/mholt/caddy/app"
 	"github.com/mholt/caddy/config/parse"
@@ -40,50 +41,16 @@ func Load(filename string, input io.Reader) (Group, error) {
 		return Default()
 	}
 
-	// Each server block represents one or more servers/addresses.
+	// Each server block represents similar hosts/addresses.
 	// Iterate each server block and make a config for each one,
 	// executing the directives that were parsed.
 	for _, sb := range serverBlocks {
-		sharedConfig, err := serverBlockToConfig(filename, sb)
-		if err != nil {
-			return nil, err
-		}
-
-		// 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 config.Port == "http" {
-				config.TLS.Enabled = false
-				log.Printf("Warning: TLS disabled for %s://%s. To force TLS over the plaintext HTTP port, "+
-					"specify port 80 explicitly (https://%s:80).", config.Port, config.Host, config.Host)
-			}
-			if i == 0 {
-				sharedConfig.Startup = []func() error{}
-				sharedConfig.Shutdown = []func() error{}
-			}
-			configs = append(configs, config)
-		}
-	}
-
-	// restore logging settings
-	log.SetFlags(flags)
-
-	// Group by address/virtualhosts
-	return arrangeBindings(configs)
-}
+		var once sync.Once
 
-// 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{
+		for _, addr := range sb.Addresses {
+			config := server.Config{
+				Host:       addr.Host,
+				Port:       addr.Port,
 				Root:       Root,
 				Middleware: make(map[string][]middleware.Middleware),
 				ConfigFile: filename,
@@ -99,22 +66,34 @@ func serverBlockToConfig(filename string, sb parse.ServerBlock) (server.Config,
 					// server config and the dispenser containing only
 					// this directive's tokens.
 					controller := &setup.Controller{
-				Config:    &sharedConfig,
+						Config:             &config,
 						Dispenser:          parse.NewDispenserTokens(filename, tokens),
+						OncePerServerBlock: func(f func()) { once.Do(f) },
 					}
 
 					midware, err := dir.setup(controller)
 					if err != nil {
-				return sharedConfig, err
+						return nil, err
 					}
 					if midware != nil {
 						// TODO: For now, we only support the default path scope /
-				sharedConfig.Middleware["/"] = append(sharedConfig.Middleware["/"], midware)
+						config.Middleware["/"] = append(config.Middleware["/"], midware)
+					}
+				}
 			}
+
+			if config.Port == "" {
+				config.Port = Port
+			}
+
+			configs = append(configs, config)
 		}
 	}
 
-	return sharedConfig, nil
+	// restore logging settings
+	log.SetFlags(flags)
+
+	return arrangeBindings(configs)
 }
 
 // arrangeBindings groups configurations by their bind address. For example,
@@ -125,8 +104,8 @@ func serverBlockToConfig(filename string, sb parse.ServerBlock) (server.Config,
 // 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) (Group, error) {
-	addresses := make(Group)
+func arrangeBindings(allConfigs []server.Config) (map[*net.TCPAddr][]server.Config, error) {
+	addresses := make(map[*net.TCPAddr][]server.Config)
 
 	// Group configs by bind address
 	for _, conf := range allConfigs {
@@ -234,8 +213,9 @@ func validDirective(d string) bool {
 	return false
 }
 
-// NewDefault creates a default configuration using the default
-// root, host, and port.
+// NewDefault makes a default configuration, which
+// is empty except for root, host, and port,
+// which are essentials for serving the cwd.
 func NewDefault() server.Config {
 	return server.Config{
 		Root: Root,
@@ -244,9 +224,8 @@ func NewDefault() server.Config {
 	}
 }
 
-// Default makes a default configuration which
-// is empty except for root, host, and port,
-// which are essentials for serving the cwd.
+// Default obtains a default config and arranges
+// bindings so it's ready to use.
 func Default() (Group, error) {
 	return arrangeBindings([]server.Config{NewDefault()})
 }

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,12 +9,12 @@ import (
 
 type parser struct {
 	Dispenser
-	block ServerBlock // current server block being parsed
+	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()
@@ -30,7 +30,7 @@ func (p *parser) parseAll() ([]ServerBlock, error) {
 }
 
 func (p *parser) parseOne() error {
-	p.block = ServerBlock{Tokens: make(map[string][]token)}
+	p.block = serverBlock{Tokens: make(map[string][]token)}
 
 	err := p.begin()
 	if err != nil {
@@ -87,7 +87,7 @@ func (p *parser) addresses() error {
 			break
 		}
 
-		if tkn != "" {
+		if tkn != "" { // empty token possible if user typed "" in Caddyfile
 			// Trailing comma indicates another address will follow, which
 			// may possibly be on the next line
 			if tkn[len(tkn)-1] == ',' {
@@ -102,7 +102,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
@@ -301,15 +301,14 @@ func standardAddress(str string) (host, port string, err error) {
 }
 
 type (
-	// ServerBlock associates tokens with a list of addresses
+	// serverBlock associates tokens with a list of addresses
 	// and groups tokens by directive name.
-	ServerBlock struct {
-		Addresses []Address
+	serverBlock struct {
+		Addresses []address
 		Tokens    map[string][]token
 	}
 
-	// Address represents a host and port.
-	Address struct {
+	address struct {
 		Host, Port string
 	}
 )

config/parse/parsing_test.go

@@ -59,7 +59,7 @@ func TestStandardAddress(t *testing.T) {
 func TestParseOneAndImport(t *testing.T) {
 	setupParseTests()
 
-	testParseOne := func(input string) (ServerBlock, error) {
+	testParseOne := func(input string) (serverBlock, error) {
 		p := testParser(input)
 		p.Next() // parseOne doesn't call Next() to start, so we must
 		err := p.parseOne()
@@ -69,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,
@@ -92,7 +92,7 @@ func TestParseOneAndImport(t *testing.T) {
 
 		{`localhost {
 		    dir1
-		  }`, false, []Address{
+		  }`, false, []address{
 			{"localhost", ""},
 		}, map[string]int{
 			"dir1": 1,
@@ -101,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,
@@ -109,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{
@@ -118,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{
@@ -127,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{
@@ -135,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{
@@ -151,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"},
@@ -161,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{
@@ -175,7 +175,7 @@ func TestParseOneAndImport(t *testing.T) {
 		  }
 		  dir2 {
 		    foo bar
-		  }`, false, []Address{
+		  }`, false, []address{
 			{"127.0.0.1", ""},
 		}, map[string]int{
 			"dir1": 5,
@@ -183,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,
@@ -197,7 +197,15 @@ func TestParseOneAndImport(t *testing.T) {
 
 		{`localhost
 		  dir1 {
-		  }`, false, []Address{
+		  }`, false, []address{
+			{"localhost", ""},
+		}, map[string]int{
+			"dir1": 3,
+		}},
+
+		{`localhost
+		  dir1 {
+		  } }`, true, []address{
 			{"localhost", ""},
 		}, map[string]int{
 			"dir1": 3,
@@ -209,18 +217,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,
@@ -228,16 +236,20 @@ 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,
 			"dir2": 2,
 		}},
 
-		{``, false, []Address{}, map[string]int{}},
+		{`import import_test1.txt import_test2.txt`, true, []address{}, map[string]int{}},
+
+		{`import not_found.txt`, true, []address{}, map[string]int{}},
+
+		{`""`, false, []address{}, map[string]int{}},
 
-		{`""`, false, []Address{}, map[string]int{}},
+		{``, false, []address{}, map[string]int{}},
 	} {
 		result, err := testParseOne(test.input)
 
@@ -282,43 +294,43 @@ func TestParseOneAndImport(t *testing.T) {
 func TestParseAll(t *testing.T) {
 	setupParseTests()
 
-	testParseAll := func(input string) ([]ServerBlock, error) {
-		p := testParser(input)
-		return p.parseAll()
-	}
-
 	for i, test := range []struct {
 		input     string
 		shouldErr bool
-		numBlocks int
+		addresses [][]address // addresses per server block, in order
 	}{
-		{`localhost`, false, 1},
+		{`localhost`, false, [][]address{
+			{{"localhost", ""}},
+		}},
 
-		{`localhost {
-			    dir1
-			  }`, false, 1},
+		{`localhost:1234`, false, [][]address{
+			[]address{{"localhost", "1234"}},
+		}},
 
-		{`http://localhost https://localhost
-			  dir1 foo bar`, false, 1},
+		{`localhost:1234 {
+		  }
+		  localhost:2015 {
+		  }`, false, [][]address{
+			[]address{{"localhost", "1234"}},
+			[]address{{"localhost", "2015"}},
+		}},
 
-		{`http://localhost, https://localhost {
-			    dir1 foo bar
-			  }`, false, 1},
+		{`localhost:1234, http://host2`, false, [][]address{
+			[]address{{"localhost", "1234"}, {"host2", "http"}},
+		}},
 
-		{`http://host1.com,
-			  http://host2.com,
-			  https://host3.com`, false, 1},
+		{`localhost:1234, http://host2,`, true, [][]address{}},
 
-		{`host1 {
+		{`http://host1.com, http://host2.com {
 		  }
-			host2 {
-			}`, false, 2},
-
-		{`""`, false, 0},
-
-		{``, false, 0},
+		  https://host3.com, https://host4.com {
+		  }`, false, [][]address{
+			[]address{{"host1.com", "http"}, {"host2.com", "http"}},
+			[]address{{"host3.com", "https"}, {"host4.com", "https"}},
+		}},
 	} {
-		results, err := testParseAll(test.input)
+		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)
@@ -327,11 +339,28 @@ func TestParseAll(t *testing.T) {
 			t.Errorf("Test %d: Expected no error, but got: %v", i, err)
 		}
 
-		if len(results) != test.numBlocks {
+		if len(blocks) != len(test.addresses) {
 			t.Errorf("Test %d: Expected %d server blocks, got %d",
-				i, test.numBlocks, len(results))
+				i, len(test.addresses), len(blocks))
+			continue
+		}
+		for j, block := range blocks {
+			if len(block.Addresses) != len(test.addresses[j]) {
+				t.Errorf("Test %d: Expected %d addresses in block %d, got %d",
+					i, len(test.addresses[j]), j, len(block.Addresses))
 				continue
 			}
+			for k, addr := range block.Addresses {
+				if addr.Host != test.addresses[j][k].Host {
+					t.Errorf("Test %d, block %d, address %d: Expected host to be '%s', but was '%s'",
+						i, j, k, test.addresses[j][k].Host, addr.Host)
+				}
+				if addr.Port != test.addresses[j][k].Port {
+					t.Errorf("Test %d, block %d, address %d: Expected port to be '%s', but was '%s'",
+						i, j, k, test.addresses[j][k].Port, addr.Port)
+				}
+			}
+		}
 	}
 }
 

config/setup/controller.go

@@ -15,6 +15,7 @@ import (
 type Controller struct {
 	*server.Config
 	parse.Dispenser
+	OncePerServerBlock func(f func())
 }
 
 // NewTestController creates a new *Controller for

config/setup/fastcgi.go

@@ -31,7 +31,7 @@ func FastCGI(c *Controller) (middleware.Middleware, error) {
 			SoftwareName:    c.AppName,
 			SoftwareVersion: c.AppVersion,
 			ServerName:      c.Host,
-			ServerPort:      c.Port, // BUG: This is not known until the server blocks are split up...
+			ServerPort:      c.Port,
 		}
 	}, nil
 }

main.go

@@ -49,7 +49,7 @@ func main() {
 		log.Fatal(err)
 	}
 
-	// Load address configurations from highest priority input
+	// Load config from file
 	addresses, err := loadConfigs()
 	if err != nil {
 		log.Fatal(err)
@@ -123,10 +123,9 @@ 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
+// Configuration is obtained from one of four sources, tried
 // in this order: 1. -conf flag, 2. stdin, 3. command line argument 4. Caddyfile.
-// If none of those are available, a default configuration is
-// loaded.
+// If none of those are available, a default configuration is loaded.
 func loadConfigs() (config.Group, error) {
 	// -conf flag
 	if conf != "" {

server/server.go

@@ -86,7 +86,7 @@ func (s *Server) Serve() error {
 			go func(vh virtualHost) {
 				// Wait for signal
 				interrupt := make(chan os.Signal, 1)
-				signal.Notify(interrupt, os.Interrupt, os.Kill)
+				signal.Notify(interrupt, os.Interrupt, os.Kill) // TODO: syscall.SIGQUIT? (Ctrl+\, Unix-only)
 				<-interrupt
 
 				// Run callbacks