Epic revert of 0ac8bf5 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.
This commit is contained in:
Matthew Holt 2015-10-14 23:45:28 -06:00
parent 7121e2c770
commit 0c07f7adcc
8 changed files with 145 additions and 138 deletions

View File

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

View File

@ -6,7 +6,7 @@ import "io"
// ServerBlocks parses the input just enough to organize tokens, // ServerBlocks parses the input just enough to organize tokens,
// in order, by server block. No further parsing is performed. // in order, by server block. No further parsing is performed.
// Server blocks are returned in the order in which they appear. // 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)} p := parser{Dispenser: NewDispenser(filename, input)}
blocks, err := p.parseAll() blocks, err := p.parseAll()
return blocks, err return blocks, err

View File

@ -9,12 +9,12 @@ import (
type parser struct { type parser struct {
Dispenser 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 eof bool // if we encounter a valid EOF in a hard place
} }
func (p *parser) parseAll() ([]ServerBlock, error) { func (p *parser) parseAll() ([]serverBlock, error) {
var blocks []ServerBlock var blocks []serverBlock
for p.Next() { for p.Next() {
err := p.parseOne() err := p.parseOne()
@ -30,7 +30,7 @@ func (p *parser) parseAll() ([]ServerBlock, error) {
} }
func (p *parser) parseOne() 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() err := p.begin()
if err != nil { if err != nil {
@ -87,7 +87,7 @@ func (p *parser) addresses() error {
break break
} }
if tkn != "" { if tkn != "" { // empty token possible if user typed "" in Caddyfile
// Trailing comma indicates another address will follow, which // Trailing comma indicates another address will follow, which
// may possibly be on the next line // may possibly be on the next line
if tkn[len(tkn)-1] == ',' { if tkn[len(tkn)-1] == ',' {
@ -102,7 +102,7 @@ func (p *parser) addresses() error {
if err != nil { if err != nil {
return err 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 // 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 ( type (
// ServerBlock associates tokens with a list of addresses // serverBlock associates tokens with a list of addresses
// and groups tokens by directive name. // and groups tokens by directive name.
ServerBlock struct { serverBlock struct {
Addresses []Address Addresses []address
Tokens map[string][]token Tokens map[string][]token
} }
// Address represents a host and port. address struct {
Address struct {
Host, Port string Host, Port string
} }
) )

View File

@ -59,7 +59,7 @@ func TestStandardAddress(t *testing.T) {
func TestParseOneAndImport(t *testing.T) { func TestParseOneAndImport(t *testing.T) {
setupParseTests() setupParseTests()
testParseOne := func(input string) (ServerBlock, error) { testParseOne := func(input string) (serverBlock, error) {
p := testParser(input) p := testParser(input)
p.Next() // parseOne doesn't call Next() to start, so we must p.Next() // parseOne doesn't call Next() to start, so we must
err := p.parseOne() err := p.parseOne()
@ -69,22 +69,22 @@ func TestParseOneAndImport(t *testing.T) {
for i, test := range []struct { for i, test := range []struct {
input string input string
shouldErr bool shouldErr bool
addresses []Address addresses []address
tokens map[string]int // map of directive name to number of tokens expected tokens map[string]int // map of directive name to number of tokens expected
}{ }{
{`localhost`, false, []Address{ {`localhost`, false, []address{
{"localhost", ""}, {"localhost", ""},
}, map[string]int{}}, }, map[string]int{}},
{`localhost {`localhost
dir1`, false, []Address{ dir1`, false, []address{
{"localhost", ""}, {"localhost", ""},
}, map[string]int{ }, map[string]int{
"dir1": 1, "dir1": 1,
}}, }},
{`localhost:1234 {`localhost:1234
dir1 foo bar`, false, []Address{ dir1 foo bar`, false, []address{
{"localhost", "1234"}, {"localhost", "1234"},
}, map[string]int{ }, map[string]int{
"dir1": 3, "dir1": 3,
@ -92,7 +92,7 @@ func TestParseOneAndImport(t *testing.T) {
{`localhost { {`localhost {
dir1 dir1
}`, false, []Address{ }`, false, []address{
{"localhost", ""}, {"localhost", ""},
}, map[string]int{ }, map[string]int{
"dir1": 1, "dir1": 1,
@ -101,7 +101,7 @@ func TestParseOneAndImport(t *testing.T) {
{`localhost:1234 { {`localhost:1234 {
dir1 foo bar dir1 foo bar
dir2 dir2
}`, false, []Address{ }`, false, []address{
{"localhost", "1234"}, {"localhost", "1234"},
}, map[string]int{ }, map[string]int{
"dir1": 3, "dir1": 3,
@ -109,7 +109,7 @@ func TestParseOneAndImport(t *testing.T) {
}}, }},
{`http://localhost https://localhost {`http://localhost https://localhost
dir1 foo bar`, false, []Address{ dir1 foo bar`, false, []address{
{"localhost", "http"}, {"localhost", "http"},
{"localhost", "https"}, {"localhost", "https"},
}, map[string]int{ }, map[string]int{
@ -118,7 +118,7 @@ func TestParseOneAndImport(t *testing.T) {
{`http://localhost https://localhost { {`http://localhost https://localhost {
dir1 foo bar dir1 foo bar
}`, false, []Address{ }`, false, []address{
{"localhost", "http"}, {"localhost", "http"},
{"localhost", "https"}, {"localhost", "https"},
}, map[string]int{ }, map[string]int{
@ -127,7 +127,7 @@ func TestParseOneAndImport(t *testing.T) {
{`http://localhost, https://localhost { {`http://localhost, https://localhost {
dir1 foo bar dir1 foo bar
}`, false, []Address{ }`, false, []address{
{"localhost", "http"}, {"localhost", "http"},
{"localhost", "https"}, {"localhost", "https"},
}, map[string]int{ }, map[string]int{
@ -135,13 +135,13 @@ func TestParseOneAndImport(t *testing.T) {
}}, }},
{`http://localhost, { {`http://localhost, {
}`, true, []Address{ }`, true, []address{
{"localhost", "http"}, {"localhost", "http"},
}, map[string]int{}}, }, map[string]int{}},
{`host1:80, http://host2.com {`host1:80, http://host2.com
dir1 foo bar dir1 foo bar
dir2 baz`, false, []Address{ dir2 baz`, false, []address{
{"host1", "80"}, {"host1", "80"},
{"host2.com", "http"}, {"host2.com", "http"},
}, map[string]int{ }, map[string]int{
@ -151,7 +151,7 @@ func TestParseOneAndImport(t *testing.T) {
{`http://host1.com, {`http://host1.com,
http://host2.com, http://host2.com,
https://host3.com`, false, []Address{ https://host3.com`, false, []address{
{"host1.com", "http"}, {"host1.com", "http"},
{"host2.com", "http"}, {"host2.com", "http"},
{"host3.com", "https"}, {"host3.com", "https"},
@ -161,7 +161,7 @@ func TestParseOneAndImport(t *testing.T) {
dir1 foo { dir1 foo {
bar baz bar baz
} }
dir2`, false, []Address{ dir2`, false, []address{
{"host1.com", "1234"}, {"host1.com", "1234"},
{"host2.com", "https"}, {"host2.com", "https"},
}, map[string]int{ }, map[string]int{
@ -175,7 +175,7 @@ func TestParseOneAndImport(t *testing.T) {
} }
dir2 { dir2 {
foo bar foo bar
}`, false, []Address{ }`, false, []address{
{"127.0.0.1", ""}, {"127.0.0.1", ""},
}, map[string]int{ }, map[string]int{
"dir1": 5, "dir1": 5,
@ -183,13 +183,13 @@ func TestParseOneAndImport(t *testing.T) {
}}, }},
{`127.0.0.1 {`127.0.0.1
unknown_directive`, true, []Address{ unknown_directive`, true, []address{
{"127.0.0.1", ""}, {"127.0.0.1", ""},
}, map[string]int{}}, }, map[string]int{}},
{`localhost {`localhost
dir1 { dir1 {
foo`, true, []Address{ foo`, true, []address{
{"localhost", ""}, {"localhost", ""},
}, map[string]int{ }, map[string]int{
"dir1": 3, "dir1": 3,
@ -197,7 +197,15 @@ func TestParseOneAndImport(t *testing.T) {
{`localhost {`localhost
dir1 { dir1 {
}`, false, []Address{ }`, false, []address{
{"localhost", ""},
}, map[string]int{
"dir1": 3,
}},
{`localhost
dir1 {
} }`, true, []address{
{"localhost", ""}, {"localhost", ""},
}, map[string]int{ }, map[string]int{
"dir1": 3, "dir1": 3,
@ -209,18 +217,18 @@ func TestParseOneAndImport(t *testing.T) {
foo foo
} }
} }
dir2 foo bar`, false, []Address{ dir2 foo bar`, false, []address{
{"localhost", ""}, {"localhost", ""},
}, map[string]int{ }, map[string]int{
"dir1": 7, "dir1": 7,
"dir2": 3, "dir2": 3,
}}, }},
{``, false, []Address{}, map[string]int{}}, {``, false, []address{}, map[string]int{}},
{`localhost {`localhost
dir1 arg1 dir1 arg1
import import_test1.txt`, false, []Address{ import import_test1.txt`, false, []address{
{"localhost", ""}, {"localhost", ""},
}, map[string]int{ }, map[string]int{
"dir1": 2, "dir1": 2,
@ -228,16 +236,20 @@ func TestParseOneAndImport(t *testing.T) {
"dir3": 1, "dir3": 1,
}}, }},
{`import import_test2.txt`, false, []Address{ {`import import_test2.txt`, false, []address{
{"host1", ""}, {"host1", ""},
}, map[string]int{ }, map[string]int{
"dir1": 1, "dir1": 1,
"dir2": 2, "dir2": 2,
}}, }},
{``, false, []Address{}, map[string]int{}}, {`import import_test1.txt import_test2.txt`, true, []address{}, map[string]int{}},
{`""`, false, []Address{}, map[string]int{}}, {`import not_found.txt`, true, []address{}, map[string]int{}},
{`""`, false, []address{}, map[string]int{}},
{``, false, []address{}, map[string]int{}},
} { } {
result, err := testParseOne(test.input) result, err := testParseOne(test.input)
@ -282,43 +294,43 @@ func TestParseOneAndImport(t *testing.T) {
func TestParseAll(t *testing.T) { func TestParseAll(t *testing.T) {
setupParseTests() setupParseTests()
testParseAll := func(input string) ([]ServerBlock, error) {
p := testParser(input)
return p.parseAll()
}
for i, test := range []struct { for i, test := range []struct {
input string input string
shouldErr bool shouldErr bool
numBlocks int addresses [][]address // addresses per server block, in order
}{ }{
{`localhost`, false, 1}, {`localhost`, false, [][]address{
{{"localhost", ""}},
}},
{`localhost { {`localhost:1234`, false, [][]address{
dir1 []address{{"localhost", "1234"}},
}`, false, 1}, }},
{`http://localhost https://localhost {`localhost:1234 {
dir1 foo bar`, false, 1},
{`http://localhost, https://localhost {
dir1 foo bar
}`, false, 1},
{`http://host1.com,
http://host2.com,
https://host3.com`, false, 1},
{`host1 {
} }
host2 { localhost:2015 {
}`, false, 2}, }`, false, [][]address{
[]address{{"localhost", "1234"}},
[]address{{"localhost", "2015"}},
}},
{`""`, false, 0}, {`localhost:1234, http://host2`, false, [][]address{
[]address{{"localhost", "1234"}, {"host2", "http"}},
}},
{``, false, 0}, {`localhost:1234, http://host2,`, true, [][]address{}},
{`http://host1.com, http://host2.com {
}
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 { if test.shouldErr && err == nil {
t.Errorf("Test %d: Expected an error, but didn't get one", i) 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) 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", t.Errorf("Test %d: Expected %d server blocks, got %d",
i, test.numBlocks, len(results)) i, len(test.addresses), len(blocks))
continue 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)
}
}
}
} }
} }

View File

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

View File

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

View File

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

View File

@ -86,7 +86,7 @@ func (s *Server) Serve() error {
go func(vh virtualHost) { go func(vh virtualHost) {
// Wait for signal // Wait for signal
interrupt := make(chan os.Signal, 1) 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 <-interrupt
// Run callbacks // Run callbacks