diff --git a/config/loader/loader.go b/config/loader/loader.go index 5bd2a1c5..0d5af67a 100644 --- a/config/loader/loader.go +++ b/config/loader/loader.go @@ -23,7 +23,6 @@ import ( "github.com/go-gost/core/resolver" "github.com/go-gost/core/router" "github.com/go-gost/core/sd" - "github.com/go-gost/core/service" "github.com/go-gost/x/config" "github.com/go-gost/x/config/parsing" admission_parser "github.com/go-gost/x/config/parsing/admission" @@ -94,14 +93,12 @@ type named[T any] struct { v T } -// registerGroup replaces all entries in r with the given entries. -// Old entries are unregistered first; if any new entry fails to register, -// the group is left partially updated (matching the historical reload -// behavior for intra-group failures). +// registerGroup replaces all entries in r with the given entries. Old entries +// are unregistered first (see unregisterAll); if any new entry fails to +// register, the group is left partially updated (matching the historical +// reload behavior for intra-group failures). func registerGroup[T any](entries []named[T], r reg.Registry[T]) error { - for name := range r.GetAll() { - r.Unregister(name) - } + unregisterAll(r) for _, e := range entries { if err := r.Register(e.name, e.v); err != nil { return err @@ -110,6 +107,15 @@ func registerGroup[T any](entries []named[T], r reg.Registry[T]) error { return nil } +// unregisterAll removes every entry from r. registry.Unregister closes a value +// before deleting it when it implements io.Closer, so for services this frees +// the bound port (a service's Close closes its listener). +func unregisterAll[T any](r reg.Registry[T]) { + for name := range r.GetAll() { + r.Unregister(name) + } +} + // register parses config sections and registers them into the global // registries. Groups are processed in dependency order: leaf components // first, then hops, chains, and finally services. This ensures that @@ -300,21 +306,34 @@ func register(cfg *config.Config) error { // --- services (references chains, resolvers, hosts, recorders, // limiters, observers, hops from registries) --- - + // + // Services are the only component group whose construction binds a port: + // service_parser.ParseService calls listener.Init, which binds. The generic + // registerGroup "parse-all-then-swap" sequence used by the other groups + // would therefore bind every new listener while the old services are still + // listening, causing EADDRINUSE on SIGHUP reload (issue #754, regressed by + // 82e7e50). Instead, unregister all old services first — unregisterAll + // closes each one (a service implements io.Closer), freeing its port — + // then parse, bind, and register the new ones. + // + // Trade-off: a parse error in the loop below leaves the registry partially + // updated (the old services are already closed and only the services parsed + // before the failure are registered). The construct/bind split would be + // needed for atomic reload, but this restores the pre-82e7e50 behavior. { - var entries []named[service.Service] + unregisterAll(registry.ServiceRegistry()) + for _, c := range cfg.Services { svc, err := service_parser.ParseService(c) if err != nil { return err } if svc != nil { - entries = append(entries, named[service.Service]{c.Name, svc}) + if err := registry.ServiceRegistry().Register(c.Name, svc); err != nil { + return err + } } } - if err := registerGroup(entries, registry.ServiceRegistry()); err != nil { - return err - } } return nil diff --git a/config/loader/loader_test.go b/config/loader/loader_test.go index 776bf57d..eee7b941 100644 --- a/config/loader/loader_test.go +++ b/config/loader/loader_test.go @@ -3,8 +3,10 @@ package loader import ( "context" "errors" + "fmt" "io" "net" + "strings" "testing" "github.com/go-gost/core/handler" @@ -805,3 +807,129 @@ func TestRegister_ServiceUnknownListener(t *testing.T) { t.Fatal("expected error for unknown listener type, got nil") } } + +// bindingStubListener is a stubListener whose Init actually binds a TCP port +// via net.Listen, reproducing the EADDRINUSE condition the close-old-before- +// bind ordering in register() must prevent. Used only by reload-collision tests. +type bindingStubListener struct { + addr string + ln net.Listener +} + +func (l *bindingStubListener) Init(md metadata.Metadata) error { + ln, err := net.Listen("tcp", l.addr) + if err != nil { + return err + } + l.ln = ln + return nil +} + +func (l *bindingStubListener) Accept() (net.Conn, error) { + if l.ln == nil { + return nil, net.ErrClosed + } + return l.ln.Accept() +} + +func (l *bindingStubListener) Addr() net.Addr { + if l.ln != nil { + return l.ln.Addr() + } + return &net.TCPAddr{IP: net.IPv4zero, Port: 0} +} + +func (l *bindingStubListener) Close() error { + if l.ln == nil { + return nil + } + return l.ln.Close() +} + +// freeTCPPort returns the number of a TCP port that is free at call time by +// opening and immediately closing a listener on 127.0.0.1:0. There is a small +// race window before the caller rebinds, which is acceptable for tests. +func freeTCPPort(t *testing.T) int { + t.Helper() + l, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen for free port: %v", err) + } + addr := l.Addr().(*net.TCPAddr) + l.Close() + return addr.Port +} + +// TestRegister_ServiceReloadNoCollision verifies that calling register() twice +// with a service on a fixed port does NOT return EADDRINUSE on the second call +// (issue #754). Before the close-old-before-bind ordering, the second +// register() bound the new listener while the old one was still listening. +func TestRegister_ServiceReloadNoCollision(t *testing.T) { + port := freeTCPPort(t) + addr := fmt.Sprintf("127.0.0.1:%d", port) + + // Register a binding listener factory under a test-specific name so we + // don't interfere with the "tcp" save/restore in TestRegister_Services. + const factoryName = "tcp-binding-test" + origListener := registry.ListenerRegistry().Get(factoryName) + registry.ListenerRegistry().Register(factoryName, func(opts ...listener.Option) listener.Listener { + options := listener.Options{} + for _, opt := range opts { + opt(&options) + } + return &bindingStubListener{addr: options.Addr} + }) + // Reuse the inert "auto" handler stub. + origHandler := registry.HandlerRegistry().Get("auto") + registry.HandlerRegistry().Register("auto", func(opts ...handler.Option) handler.Handler { + return &stubHandler{} + }) + t.Cleanup(func() { + registry.ListenerRegistry().Unregister(factoryName) + registry.HandlerRegistry().Unregister("auto") + if origListener != nil { + registry.ListenerRegistry().Register(factoryName, origListener) + } + if origHandler != nil { + registry.HandlerRegistry().Register("auto", origHandler) + } + }) + + r := registry.ServiceRegistry() + const svcName = "reload-svc" + t.Cleanup(func() { r.Unregister(svcName) }) + + buildCfg := func() *config.Config { + return &config.Config{ + Services: []*config.ServiceConfig{ + { + Name: svcName, + Addr: addr, + Listener: &config.ListenerConfig{Type: factoryName}, + Handler: &config.HandlerConfig{Type: "auto"}, + }, + }, + } + } + + // First load: registers the service and binds the port. + if err := register(buildCfg()); err != nil { + t.Fatalf("first register: unexpected error: %v", err) + } + if !r.IsRegistered(svcName) { + t.Fatal("expected service registered after first load") + } + + // Second load simulates a SIGHUP reload. With the old ordering this bound + // the new listener while the old one was still open and returned + // "address already in use"; the fix closes old services first. + if err := register(buildCfg()); err != nil { + if strings.Contains(err.Error(), "address already in use") { + t.Fatalf("second register (reload): port collision not avoided: %v", err) + } + t.Fatalf("second register (reload): unexpected error: %v", err) + } + if !r.IsRegistered(svcName) { + t.Fatal("expected service registered after reload") + } +}