all: close active listeners on error

Author: EugeneOne1

proxy/config.go

@@ -266,35 +266,42 @@ type PendingRequestsConfig struct {
 func (p *Proxy) validateConfig() (err error) {
 	err = p.UpstreamConfig.validate()
 	if err != nil {
-		return fmt.Errorf("validating general upstreams: %w", err)
+		return fmt.Errorf("general upstreams: %w", err)
 	}
 
 	err = ValidatePrivateConfig(p.PrivateRDNSUpstreamConfig, p.privateNets)
 	if err != nil {
 		if p.UsePrivateRDNS || errors.Is(err, upstream.ErrNoUpstreams) {
-			return fmt.Errorf("validating private RDNS upstreams: %w", err)
+			return fmt.Errorf("private RDNS upstreams: %w", err)
 		}
 	}
 
+	err = p.Fallbacks.validate()
 	// Allow [Proxy.Fallbacks] to be nil, but not empty.  nil means not to use
 	// fallbacks at all.
-	err = p.Fallbacks.validate()
 	if errors.Is(err, upstream.ErrNoUpstreams) {
-		return fmt.Errorf("validating fallbacks: %w", err)
+		return fmt.Errorf("fallbacks: %w", err)
 	}
 
 	err = p.validateRatelimit()
 	if err != nil {
-		return fmt.Errorf("validating ratelimit: %w", err)
+		return fmt.Errorf("ratelimit: %w", err)
 	}
 
 	switch p.UpstreamMode {
-	case "":
-		// Go on.
-	case UpstreamModeFastestAddr, UpstreamModeLoadBalance, UpstreamModeParallel:
+	case
+		"",
+		UpstreamModeFastestAddr,
+		UpstreamModeLoadBalance,
+		UpstreamModeParallel:
 		// Go on.
 	default:
-		return fmt.Errorf("bad upstream mode: %q", p.UpstreamMode)
+		return fmt.Errorf("upstream mode: %w: %q", errors.ErrBadEnumValue, p.UpstreamMode)
+	}
+
+	err = p.validateBasicAuth()
+	if err != nil {
+		return fmt.Errorf("basic auth: %w", err)
 	}
 
 	p.logConfigInfo()
@@ -368,9 +375,11 @@ func (p *Proxy) logConfigInfo() {
 
 // validateListenAddrs returns an error if the addresses are not configured
 // properly.
+//
+// TODO(e.burkov):  Move to configuration validation.
 func (p *Proxy) validateListenAddrs() (err error) {
 	if !p.hasListenAddrs() {
-		return errors.Error("no listen address specified")
+		return fmt.Errorf("listen addresses: %w", errors.ErrNoValue)
 	}
 
 	err = p.validateTLSConfig()

proxy/dns64.go

@@ -37,6 +37,8 @@ const (
 // is specified explicitly.  Each prefix also validated to be a valid IPv6 CIDR
 // with a maximum length of 96 bits.  The first specified prefix is then used to
 // synthesize AAAA records.
+//
+// TODO(e.burkov):  Split validation and initialization.
 func (p *Proxy) setupDNS64() (err error) {
 	if !p.Config.UseDNS64 {
 		return nil

proxy/proxy.go

@@ -27,6 +27,7 @@ import (
 	"github.com/AdguardTeam/golibs/service"
 	"github.com/AdguardTeam/golibs/syncutil"
 	"github.com/AdguardTeam/golibs/timeutil"
+	"github.com/AdguardTeam/golibs/validate"
 	"github.com/ameshkov/dnscrypt/v2"
 	"github.com/miekg/dns"
 	gocache "github.com/patrickmn/go-cache"
@@ -183,9 +184,9 @@ type Proxy struct {
 	// udpOOBSize is the size of the out-of-band data for UDP connections.
 	udpOOBSize int
 
-	// bindRetryNum is the number of retries for binding to an address for
+	// bindRetryCount is the number of retries for binding to an address for
 	// listening.  Zero means one attempt and no retries.
-	bindRetryNum uint
+	bindRetryCount uint
 
 	// bindRetryIvl is the interval between attempts to bind to an address for
 	// listening.
@@ -215,6 +216,8 @@ type Proxy struct {
 // New creates a new Proxy with the specified configuration.  c must not be nil.
 //
 // TODO(e.burkov):  Cover with tests.
+//
+// TODO(e.burkov):  Add context.
 func New(c *Config) (p *Proxy, err error) {
 	p = &Proxy{
 		Config: *c,
@@ -256,12 +259,6 @@ func New(c *Config) (p *Proxy, err error) {
 		return nil, err
 	}
 
-	// TODO(s.chzhen):  Consider moving to [Proxy.validateConfig].
-	err = p.validateBasicAuth()
-	if err != nil {
-		return nil, fmt.Errorf("basic auth: %w", err)
-	}
-
 	p.initCache()
 
 	if p.MaxGoroutines > 0 {
@@ -281,7 +278,7 @@ func New(c *Config) (p *Proxy, err error) {
 	}
 
 	if bindRetries := c.BindRetryConfig; bindRetries != nil && bindRetries.Enabled {
-		p.bindRetryNum = bindRetries.Count
+		p.bindRetryCount = bindRetries.Count
 		p.bindRetryIvl = bindRetries.Interval
 	}
 
@@ -290,6 +287,7 @@ func New(c *Config) (p *Proxy, err error) {
 		return nil, fmt.Errorf("setting up DNS64: %w", err)
 	}
 
+	// TODO(e.burkov):  Clone all mutable fields of Config.
 	p.RatelimitWhitelist = slices.Clone(p.RatelimitWhitelist)
 	slices.SortFunc(p.RatelimitWhitelist, netip.Addr.Compare)
 
@@ -324,11 +322,7 @@ func (p *Proxy) validateBasicAuth() (err error) {
 		return nil
 	}
 
-	if len(conf.HTTPSListenAddr) == 0 {
-		return errors.Error("no https addrs")
-	}
-
-	return nil
+	return validate.NotEmptySlice("https listen addrs", conf.HTTPSListenAddr)
 }
 
 // Returns true if proxy is started.  It is safe for concurrent use.
@@ -359,12 +353,15 @@ func (p *Proxy) Start(ctx context.Context) (err error) {
 		return err
 	}
 
-	err = p.configureListeners(ctx)
+	err = p.startListeners(ctx)
 	if err != nil {
-		return fmt.Errorf("configuring listeners: %w", err)
+		closeErr := errors.Join(p.closeListeners(nil)...)
+
+		return fmt.Errorf("configuring listeners: %w", errors.WithDeferred(err, closeErr))
 	}
 
-	p.startListeners()
+	p.serveListeners()
+
 	p.started = true
 
 	return nil
@@ -390,7 +387,8 @@ func closeAll[C io.Closer](errs []error, closers ...C) (appended []error) {
 	return errs
 }
 
-// Shutdown implements the [service.Interface] for *Proxy.
+// Shutdown implements the [service.Interface] for *Proxy.  It also closes the
+// configured upstream configurations.
 func (p *Proxy) Shutdown(ctx context.Context) (err error) {
 	p.logger.InfoContext(ctx, "stopping server")
 
@@ -404,65 +402,75 @@ func (p *Proxy) Shutdown(ctx context.Context) (err error) {
 		return nil
 	}
 
-	errs := closeAll(nil, p.tcpListen...)
+	errs := p.closeListeners(nil)
+
+	for _, u := range []*UpstreamConfig{
+		p.UpstreamConfig,
+		p.PrivateRDNSUpstreamConfig,
+		p.Fallbacks,
+	} {
+		if u != nil {
+			errs = closeAll(errs, u)
+		}
+	}
+
+	p.started = false
+
+	p.logger.InfoContext(ctx, "stopped dns proxy server")
+
+	err = errors.Join(errs...)
+	if err != nil {
+		return fmt.Errorf("stopping dns proxy server: %w", err)
+	}
+
+	return nil
+}
+
+// closeListeners closes all acrive listeners and returns the occurred errors.
+func (p *Proxy) closeListeners(errs []error) (res []error) {
+	res = errs
+
+	res = closeAll(res, p.tcpListen...)
 	p.tcpListen = nil
 
-	errs = closeAll(errs, p.udpListen...)
+	res = closeAll(res, p.udpListen...)
 	p.udpListen = nil
 
-	errs = closeAll(errs, p.tlsListen...)
+	res = closeAll(res, p.tlsListen...)
 	p.tlsListen = nil
 
 	if p.httpsServer != nil {
-		errs = closeAll(errs, p.httpsServer)
+		res = closeAll(res, p.httpsServer)
 		p.httpsServer = nil
 
 		// No need to close these since they're closed by httpsServer.Close().
 		p.httpsListen = nil
 	}
 
 	if p.h3Server != nil {
-		errs = closeAll(errs, p.h3Server)
+		res = closeAll(res, p.h3Server)
 		p.h3Server = nil
 	}
 
-	errs = closeAll(errs, p.h3Listen...)
+	res = closeAll(res, p.h3Listen...)
 	p.h3Listen = nil
 
-	errs = closeAll(errs, p.quicListen...)
+	res = closeAll(res, p.quicListen...)
 	p.quicListen = nil
 
-	errs = closeAll(errs, p.quicTransports...)
+	res = closeAll(res, p.quicTransports...)
 	p.quicTransports = nil
 
-	errs = closeAll(errs, p.quicConns...)
+	res = closeAll(res, p.quicConns...)
 	p.quicConns = nil
 
-	errs = closeAll(errs, p.dnsCryptUDPListen...)
+	res = closeAll(res, p.dnsCryptUDPListen...)
 	p.dnsCryptUDPListen = nil
 
-	errs = closeAll(errs, p.dnsCryptTCPListen...)
+	res = closeAll(res, p.dnsCryptTCPListen...)
 	p.dnsCryptTCPListen = nil
 
-	for _, u := range []*UpstreamConfig{
-		p.UpstreamConfig,
-		p.PrivateRDNSUpstreamConfig,
-		p.Fallbacks,
-	} {
-		if u != nil {
-			errs = closeAll(errs, u)
-		}
-	}
-
-	p.started = false
-
-	p.logger.InfoContext(ctx, "stopped dns proxy server")
-
-	if len(errs) > 0 {
-		return fmt.Errorf("stopping dns proxy server: %w", errors.Join(errs...))
-	}
-
-	return nil
+	return res
 }
 
 // addrFunc provides the address from the given A.

proxy/retry.go

@@ -32,7 +32,7 @@ func (p *Proxy) bindWithRetry(ctx context.Context, bindFunc func() (err error))
 
 	p.logger.WarnContext(ctx, "binding", "attempt", 1, slogutil.KeyError, err)
 
-	for attempt := uint(1); attempt <= p.bindRetryNum; attempt++ {
+	for attempt := uint(1); attempt <= p.bindRetryCount; attempt++ {
 		time.Sleep(p.bindRetryIvl)
 
 		retryErr := bindFunc()

proxy/retry_internal_test.go

@@ -76,9 +76,9 @@ func TestWithRetry(t *testing.T) {
 	}}
 
 	p := &Proxy{
-		logger:       slogutil.NewDiscardLogger(),
-		bindRetryNum: 1,
-		bindRetryIvl: 0,
+		logger:         slogutil.NewDiscardLogger(),
+		bindRetryCount: 1,
+		bindRetryIvl:   0,
 	}
 
 	for _, tc := range testCases {

proxy/server.go

@@ -15,8 +15,9 @@ import (
 	"github.com/quic-go/quic-go"
 )
 
-// configureListeners configures listeners.
-func (p *Proxy) configureListeners(ctx context.Context) (err error) {
+// startListeners configures listeners and starts listening each configured
+// address.  If it returns an error, all listeners should be closed manually.
+func (p *Proxy) startListeners(ctx context.Context) (err error) {
 	err = p.initUDPListeners(ctx)
 	if err != nil {
 		return err
@@ -50,8 +51,8 @@ func (p *Proxy) configureListeners(ctx context.Context) (err error) {
 	return nil
 }
 
-// startListeners starts listener loops.
-func (p *Proxy) startListeners() {
+// serveListeners starts serving the configured listeners.
+func (p *Proxy) serveListeners() {
 	for _, l := range p.udpListen {
 		go p.udpPacketLoop(l, p.requestsSema)
 	}

proxy/upstreams.go

@@ -327,20 +327,15 @@ func (p *configParser) includeToReserved(dnsUpstream upstream.Upstream, domains
 // considered valid if it contains at least a single default upstream.  Empty c
 // causes [upstream.ErrNoUpstreams].
 func (uc *UpstreamConfig) validate() (err error) {
-	const (
-		errNilConf   errors.Error = "upstream config is nil"
-		errNoDefault errors.Error = "no default upstreams specified"
-	)
-
 	switch {
 	case uc == nil:
-		return errNilConf
+		return errors.ErrNoValue
 	case len(uc.Upstreams) > 0:
 		return nil
 	case len(uc.DomainReservedUpstreams) == 0 && len(uc.SpecifiedDomainUpstreams) == 0:
 		return upstream.ErrNoUpstreams
 	default:
-		return errNoDefault
+		return fmt.Errorf("default upstreams: %w", errors.ErrNoValue)
 	}
 }
 

proxy/upstreams_internal_test.go

@@ -188,7 +188,7 @@ func TestUpstreamConfig_Validate(t *testing.T) {
 		},
 	}, {
 		name:    "no_default",
-		wantErr: errors.Error("no default upstreams specified"),
+		wantErr: errors.ErrNoValue,
 		in: []string{
 			"[/domain.example/]udp://upstream.example:53",
 			"[/another.domain.example/]#",
@@ -205,7 +205,9 @@ func TestUpstreamConfig_Validate(t *testing.T) {
 	}
 
 	t.Run("actual_nil", func(t *testing.T) {
-		assert.ErrorIs(t, (*UpstreamConfig)(nil).validate(), errors.Error("upstream config is nil"))
+		var c *UpstreamConfig
+
+		assert.Equal(t, c.validate(), errors.ErrNoValue)
 	})
 }
 

upstream/parallel.go

@@ -8,6 +8,8 @@ import (
 	"github.com/miekg/dns"
 )
 
+// TODO(e.burkov):  Consider using wrapped [errors.ErrNoValue] and
+// [errors.ErrEmptyValue] instead.
 const (
 	// ErrNoUpstreams is returned from the methods that expect at least a single
 	// upstream to work with when no upstreams specified.