Make endpoint modules special

To support unusual configuration syntax, endpoint modules (imap, smtp,
etc) relied on rather awkward code using modName+instName+aliases as
arguments. This commit replaces old handling with use of special
signature similar to inlineArgs introduced in 1edd031.

Endpoint modules are placed in a separate 'registry' and use
different initialization callback signature for simplicity. This makes
them inaccessible for other modules, though they are not supposed to be
anyway.

Endpoint modules are initialized before other modules. This allows
detecting unused configuration blocks by checking for modules
that were not lazily initalized after endpoint initialization.
This relies on endpoint modules being essentially "roots" of
instances dependency tree.

Idea of "semantical module names" is completely dropped now and so
HACKING.md is updated to not mention it.

HACKING.md

@@ -67,19 +67,14 @@ A single module instance can have one or more names. The first name is called
 called "aliases" and only used by module.GetInstance (e.g. module instance can
 be fetched by any name).
 
-Some modules attach additional meaning to names. This is generally accepted
+Some module instances are defined "inline" in user configuration. That is.
-since it is better to have only a single instance managing one resource. For
+their configuration block is placed right where they are used. These instances
-example, module instance implementing forwarding to the downstream server can not
+have no instance names and are not added to the global map so they can not be
-reasonably enforce any limitations unless it is only one instance "controlling"
+accessed by modules other than one that used ConfigFromNode on the
-that downstream. Unique names requirement helps a lot here.
+corresponding config block. All arguments after the module name in an inline
-
+definition represent "inline arguments". They are passed to the module instance
-"Semantical names" idea explained above is not applied when modules instances
+directly and not used anyhow by other code (i.e. they are not guaranteed to be
-are defined "inline" (in place they are used in). These instances have no
+unique).
-instance names and are not added to the global map so they can not be accessed
-by modules other than one that used ConfigFromNode on the corresponding config
-block. All arguments after the module name in an inline definition represent
-"inline arguments". They are passed to the module instance directly and not
-used anyhow by other code (i.e. they are not guaranteed to be unique).
 
 ### A word on error logging
 
@@ -99,4 +94,4 @@ together, remote module will provide logs about concrete errors happened and
 queue module will provide information about tries made and scheduled to be made
 in the future.
 
-[1]: https://github.com/foxcpp/maddy/wiki/Dev:-Comments-on-design
+[1]: https://github.com/foxcpp/maddy/wiki/Dev:-Comments-on-design

config/module/modconfig.go

@@ -14,7 +14,6 @@ import (
 
 	"github.com/foxcpp/maddy/config"
 	"github.com/foxcpp/maddy/config/parser"
-	"github.com/foxcpp/maddy/log"
 	"github.com/foxcpp/maddy/module"
 )
 
@@ -25,8 +24,6 @@ func createInlineModule(modName string, args []string) (module.Module, error) {
 		return nil, fmt.Errorf("unknown module: %s", modName)
 	}
 
-	log.Debugln("module create", modName, args, "(inline)")
-
 	return newMod(modName, "", nil, args)
 }
 
@@ -35,7 +32,6 @@ func createInlineModule(modName string, args []string) (module.Module, error) {
 //
 // args must contain at least one argument, otherwise initInlineModule panics.
 func initInlineModule(modObj module.Module, globals map[string]interface{}, block *config.Node) error {
-	log.Debugln("module init", modObj.Name(), modObj.InstanceName(), "(inline)")
 	return modObj.Init(config.NewMap(globals, block))
 }
 

endpoint/imap/imap.go

@@ -27,8 +27,7 @@ import (
 )
 
 type Endpoint struct {
-	name      string
+	addrs     []string
-	aliases   []string
 	serv      *imapserver.Server
 	listeners []net.Listener
 	Auth      module.AuthProvider
@@ -41,16 +40,11 @@ type Endpoint struct {
 	Log log.Logger
 }
 
-func New(_, instName string, aliases, inlineArgs []string) (module.Module, error) {
+func New(modName string, addrs []string) (module.Module, error) {
-	if len(inlineArgs) != 0 {
-		return nil, errors.New("imap: inline arguments are not used")
-	}
 	endp := &Endpoint{
-		name:    instName,
+		addrs: addrs,
-		aliases: aliases,
+		Log:   log.Logger{Name: "imap"},
-		Log:     log.Logger{Name: "imap"},
 	}
-	endp.name = instName
 
 	return endp, nil
 }
@@ -83,12 +77,11 @@ func (endp *Endpoint) Init(cfg *config.Map) error {
 		return fmt.Errorf("imap: failed to init backend: nil update channel")
 	}
 
-	args := append([]string{endp.name}, endp.aliases...)
+	addresses := make([]config.Endpoint, 0, len(endp.addrs))
-	addresses := make([]config.Endpoint, 0, len(args))
+	for _, addr := range endp.addrs {
-	for _, addr := range args {
 		saddr, err := config.ParseEndpoint(addr)
 		if err != nil {
-			return fmt.Errorf("imap: invalid address: %s", endp.name)
+			return fmt.Errorf("imap: invalid address: %s", addr)
 		}
 		addresses = append(addresses, saddr)
 	}
@@ -154,7 +147,7 @@ func (endp *Endpoint) Name() string {
 }
 
 func (endp *Endpoint) InstanceName() string {
-	return endp.name
+	return "imap"
 }
 
 func (endp *Endpoint) Close() error {
@@ -204,7 +197,7 @@ func (endp *Endpoint) enableExtensions() error {
 }
 
 func init() {
-	module.Register("imap", New)
+	module.RegisterEndpoint("imap", New)
 
 	imap.CharsetReader = message.CharsetReader
 }

endpoint/smtp/smtp.go

@@ -216,7 +216,7 @@ type Endpoint struct {
 	Auth      module.AuthProvider
 	serv      *smtp.Server
 	name      string
-	aliases   []string
+	addrs     []string
 	listeners []net.Listener
 	pipeline  *msgpipeline.MsgPipeline
 	resolver  dns.Resolver
@@ -232,20 +232,17 @@ type Endpoint struct {
 }
 
 func (endp *Endpoint) Name() string {
-	return "smtp"
+	return endp.name
 }
 
 func (endp *Endpoint) InstanceName() string {
 	return endp.name
 }
 
-func New(modName, instName string, aliases, inlineArgs []string) (module.Module, error) {
+func New(modName string, addrs []string) (module.Module, error) {
-	if len(inlineArgs) != 0 {
-		return nil, fmt.Errorf("%s: inline arguments are not used", modName)
-	}
 	endp := &Endpoint{
-		name:       instName,
+		name:       modName,
-		aliases:    aliases,
+		addrs:      addrs,
 		submission: modName == "submission",
 		lmtp:       modName == "lmtp",
 		resolver:   net.DefaultResolver,
@@ -264,9 +261,8 @@ func (endp *Endpoint) Init(cfg *config.Map) error {
 		endp.Log.Debugf("authentication provider: %s %s", endp.Auth.(module.Module).Name(), endp.Auth.(module.Module).InstanceName())
 	}
 
-	args := append([]string{endp.name}, endp.aliases...)
+	addresses := make([]config.Endpoint, 0, len(endp.addrs))
-	addresses := make([]config.Endpoint, 0, len(args))
+	for _, addr := range endp.addrs {
-	for _, addr := range args {
 		saddr, err := config.ParseEndpoint(addr)
 		if err != nil {
 			return fmt.Errorf("smtp: invalid address: %s", addr)
@@ -441,9 +437,9 @@ func (endp *Endpoint) Close() error {
 }
 
 func init() {
-	module.Register("smtp", New)
+	module.RegisterEndpoint("smtp", New)
-	module.Register("submission", New)
+	module.RegisterEndpoint("submission", New)
-	module.Register("lmtp", New)
+	module.RegisterEndpoint("lmtp", New)
 
 	rand.Seed(time.Now().UnixNano())
 }

maddy.go

@@ -5,7 +5,6 @@ import (
 
 	"github.com/foxcpp/maddy/config"
 	"github.com/foxcpp/maddy/log"
-	"github.com/foxcpp/maddy/module"
 
 	// Import packages for side-effect of module registration.
 	_ "github.com/foxcpp/maddy/auth/external"
@@ -22,13 +21,7 @@ import (
 	_ "github.com/foxcpp/maddy/target/smtp_downstream"
 )
 
-type modInfo struct {
-	instance module.Module
-	cfg      config.Node
-}
-
 func Start(cfg []config.Node) error {
-	instances := make(map[string]modInfo)
 	globals := config.NewMap(nil, &config.Node{Children: cfg})
 	globals.String("hostname", false, false, "", nil)
 	globals.String("autogenerated_msg_domain", false, false, "", nil)
@@ -45,65 +38,17 @@ func Start(cfg []config.Node) error {
 
 	defer log.DefaultLogger.Out.Close()
 
-	for _, block := range unmatched {
+	insts, err := instancesFromConfig(globals.Values, unmatched)
-		var instName string
+	if err != nil {
-		var modAliases []string
+		return err
-		if len(block.Args) == 0 {
-			instName = block.Name
-		} else {
-			instName = block.Args[0]
-			modAliases = block.Args[1:]
-		}
-
-		modName := block.Name
-
-		factory := module.Get(modName)
-		if factory == nil {
-			return config.NodeErr(&block, "unknown module: %s", modName)
-		}
-
-		if module.HasInstance(instName) {
-			return config.NodeErr(&block, "config block named %s already exists", instName)
-		}
-
-		log.Debugln("module create", modName, instName)
-		inst, err := factory(modName, instName, modAliases, nil)
-		if err != nil {
-			return err
-		}
-
-		block := block
-		module.RegisterInstance(inst, config.NewMap(globals.Values, &block))
-		for _, alias := range modAliases {
-			if module.HasInstance(alias) {
-				return config.NodeErr(&block, "config block named %s already exists", alias)
-			}
-			module.RegisterAlias(alias, instName)
-			log.Debugln("module alias", alias, "->", instName)
-		}
-		instances[instName] = modInfo{instance: inst, cfg: block}
 	}
 
-	for _, inst := range instances {
+	handleSignals()
-		if module.Initialized[inst.instance.InstanceName()] {
-			log.Debugln("module init", inst.instance.Name(), inst.instance.InstanceName(), "skipped because it was lazily initialized before")
-			continue
-		}
 
-		module.Initialized[inst.instance.InstanceName()] = true
+	for _, inst := range insts {
-		log.Debugln("module init", inst.instance.Name(), inst.instance.InstanceName())
+		if closer, ok := inst.(io.Closer); ok {
-		if err := inst.instance.Init(config.NewMap(globals.Values, &inst.cfg)); err != nil {
-			return err
-		}
-	}
-
-	waitForSignal()
-
-	for _, inst := range instances {
-		if closer, ok := inst.instance.(io.Closer); ok {
-			log.Debugln("clean-up for module", inst.instance.Name(), inst.instance.InstanceName())
 			if err := closer.Close(); err != nil {
-				log.Printf("module %s (%s) close failed: %v", inst.instance.Name(), inst.instance.InstanceName(), err)
+				log.Printf("module %s (%s) close failed: %v", inst.Name(), inst.InstanceName(), err)
 			}
 		}
 	}

module.go

@@ -0,0 +1,100 @@
+package maddy
+
+import (
+	"github.com/foxcpp/maddy/config"
+	"github.com/foxcpp/maddy/log"
+	"github.com/foxcpp/maddy/module"
+)
+
+type modInfo struct {
+	instance module.Module
+	cfg      config.Node
+}
+
+func instancesFromConfig(globals map[string]interface{}, nodes []config.Node) ([]module.Module, error) {
+	var (
+		endpoints []modInfo
+		mods      []modInfo
+	)
+
+	for _, block := range nodes {
+		var instName string
+		var modAliases []string
+		if len(block.Args) == 0 {
+			instName = block.Name
+		} else {
+			instName = block.Args[0]
+			modAliases = block.Args[1:]
+		}
+
+		modName := block.Name
+
+		endpFactory := module.GetEndpoint(modName)
+		if endpFactory != nil {
+			inst, err := endpFactory(modName, block.Args)
+			if err != nil {
+				return nil, err
+			}
+
+			endpoints = append(endpoints, modInfo{instance: inst, cfg: block})
+			continue
+		}
+
+		factory := module.Get(modName)
+		if factory == nil {
+			return nil, config.NodeErr(&block, "unknown module: %s", modName)
+		}
+
+		if module.HasInstance(instName) {
+			return nil, config.NodeErr(&block, "config block named %s already exists", instName)
+		}
+
+		inst, err := factory(modName, instName, modAliases, nil)
+		if err != nil {
+			return nil, err
+		}
+
+		block := block
+		module.RegisterInstance(inst, config.NewMap(globals, &block))
+		for _, alias := range modAliases {
+			if module.HasInstance(alias) {
+				return nil, config.NodeErr(&block, "config block named %s already exists", alias)
+			}
+			module.RegisterAlias(alias, instName)
+		}
+		mods = append(mods, modInfo{instance: inst, cfg: block})
+	}
+
+	for _, endp := range endpoints {
+		if err := endp.instance.Init(config.NewMap(globals, &endp.cfg)); err != nil {
+			return nil, err
+		}
+	}
+
+	// We initialize all non-endpoint modules defined at top-level
+	// just for purpose of checking that they have a valid configuration.
+	//
+	// Modules that are actually used will be pulled by lazy initialization
+	// logic during endpoint initialization.
+	for _, inst := range mods {
+		if module.Initialized[inst.instance.InstanceName()] {
+			continue
+		}
+
+		log.Printf("%s (%s) is not used anywhere", inst.instance.InstanceName(), inst.instance.Name())
+
+		module.Initialized[inst.instance.InstanceName()] = true
+		if err := inst.instance.Init(config.NewMap(globals, &inst.cfg)); err != nil {
+			return nil, err
+		}
+	}
+
+	res := make([]module.Module, 0, len(mods)+len(endpoints))
+	for _, endp := range endpoints {
+		res = append(res, endp.instance)
+	}
+	for _, mod := range mods {
+		res = append(res, mod.instance)
+	}
+	return res, nil
+}

module/instances.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 
 	"github.com/foxcpp/maddy/config"
-	"github.com/foxcpp/maddy/log"
 )
 
 var (
@@ -64,11 +63,9 @@ func GetInstance(name string) (Module, error) {
 
 	// Break circular dependencies.
 	if Initialized[name] {
-		log.Debugln("module init", mod.mod.Name(), name, "(lazy init skipped)")
 		return mod.mod, nil
 	}
 
-	log.Debugln("module init", mod.mod.Name(), name, "(lazy)")
 	Initialized[name] = true
 	if err := mod.mod.Init(mod.cfg); err != nil {
 		return mod.mod, err

module/module.go

@@ -55,3 +55,17 @@ type Module interface {
 // If module is defined inline, instName will be empty and all values
 // specified after module name in configuration will be in inlineArgs.
 type FuncNewModule func(modName, instName string, aliases, inlineArgs []string) (Module, error)
+
+// FuncNewEndpoint is a function that creates new instance of endpoint
+// module.
+//
+// Compared to regular modules, endpoint module instances are:
+// - Not registered in the global registry.
+// - Can't be defined inline.
+// - Don't have an unique name
+// - All config arguments are always passed as an 'addrs' slice and not used as
+// names.
+//
+// As a consequence of having no per-instance name, InstanceName of the module
+// object always returns the same value as Name.
+type FuncNewEndpoint func(modName string, addrs []string) (Module, error)

module/registry.go

@@ -4,8 +4,11 @@ import (
 	"sync"
 )
 
-var modules = make(map[string]FuncNewModule)
+var (
-var modulesLock sync.RWMutex
+	modules     = make(map[string]FuncNewModule)
+	endpoints   = make(map[string]FuncNewEndpoint)
+	modulesLock sync.RWMutex
+)
 
 // Register adds module factory function to global registry.
 //
@@ -26,6 +29,8 @@ func Register(name string, factory FuncNewModule) {
 
 // Get returns module from global registry.
 //
+// This function does not return endpoint-type modules, use GetEndpoint for
+// that.
 // Nil is returned if no module with specified name is registered.
 func Get(name string) FuncNewModule {
 	modulesLock.RLock()
@@ -33,3 +38,28 @@ func Get(name string) FuncNewModule {
 
 	return modules[name]
 }
+
+// GetEndpoints returns an endpoint module from global registry.
+//
+// Nil is returned if no module with specified name is registered.
+func GetEndpoint(name string) FuncNewEndpoint {
+	modulesLock.RLock()
+	defer modulesLock.RUnlock()
+
+	return endpoints[name]
+}
+
+// RegisterEndpoint registers an endpoint module.
+//
+// See FuncNewEndpoint for information about
+// differences of endpoint modules from regular modules.
+func RegisterEndpoint(name string, factory FuncNewEndpoint) {
+	modulesLock.Lock()
+	defer modulesLock.Unlock()
+
+	if _, ok := endpoints[name]; ok {
+		panic("Register: module with specified name is already registered: " + name)
+	}
+
+	endpoints[name] = factory
+}

signal.go

@@ -10,7 +10,13 @@ import (
 	"github.com/foxcpp/maddy/log"
 )
 
-func waitForSignal() os.Signal {
+// handleSignals function creates and listens on OS signals channel.
+//
+// OS-specific signals that correspond to the program termination
+// (SIGTERM, SIGHUP, SIGINT) will cause this function to return.
+//
+// SIGUSR1 will call reinitLogging without returning.
+func handleSignals() os.Signal {
 	sig := make(chan os.Signal, 5)
 	signal.Notify(sig, os.Interrupt, syscall.SIGTERM, syscall.SIGHUP, syscall.SIGINT, syscall.SIGUSR1)
 
@@ -21,7 +27,7 @@ func waitForSignal() os.Signal {
 			reinitLogging()
 		default:
 			go func() {
-				s := waitForSignal()
+				s := handleSignals()
 				log.Printf("forced shutdown due to signal (%v)!", s)
 				os.Exit(1)
 			}()

signal_nonposix.go

@@ -10,7 +10,7 @@ import (
 	"github.com/foxcpp/maddy/log"
 )
 
-func waitForSignal() os.Signal {
+func handleSignals() os.Signal {
 	sig := make(chan os.Signal, 5)
 	signal.Notify(sig, os.Interrupt, syscall.SIGTERM, syscall.SIGHUP, syscall.SIGINT)