From c473cdfbd35523db2b51189a0c86aba969562a7b Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Mon, 18 May 2026 15:59:15 -0500 Subject: [PATCH 01/23] connmgr: Support whitelisting. Currently the whitelisting logic happens in the server which makes it inaccessible to the connection manager. In order to pave the way for supporting various connection-related logic that currently happens in the server, but ideally should be happening in the connection manager, this adds basic support for whitelisting CIDR prefixes to the connection manager. The connection manager config struct now accepts a slice of prefixes and a new method named IsWhitelisted is added. Note that this only adds support . It does not update anything to use the new functionality yet. --- internal/connmgr/connmanager.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/internal/connmgr/connmanager.go b/internal/connmgr/connmanager.go index 453e4a33d3..a2df96398b 100644 --- a/internal/connmgr/connmanager.go +++ b/internal/connmgr/connmanager.go @@ -12,6 +12,7 @@ import ( "errors" "fmt" "net" + "net/netip" "strconv" "sync" "sync/atomic" @@ -260,6 +261,10 @@ type Config struct { // DialTimeout specifies the amount of time to wait for a connection to // complete before giving up. DialTimeout time.Duration + + // Whitelists specifies CIDR address prefixes to whitelist. Whitelisted + // addresses are exempt from banning and certain connection limits. + Whitelists []netip.Prefix } // ConnManager provides a manager to handle network connections. @@ -317,6 +322,22 @@ type ConnManager struct { connIDByAddr map[string]uint64 } +// IsWhitelisted returns whether the IP address is included in the whitelisted +// networks and IPs. +func (cm *ConnManager) IsWhitelisted(addr *addrmgr.NetAddress) bool { + if len(cm.cfg.Whitelists) == 0 { + return false + } + + ip, _ := netip.AddrFromSlice(addr.IP) + for _, prefix := range cm.cfg.Whitelists { + if prefix.Contains(ip) { + return true + } + } + return false +} + // checkShutdown returns [ErrShutdown] when the connection manager quit channel // has been closed. func (cm *ConnManager) checkShutdown() error { From 220dea05bc6f7715e45194253ca9537377e91b9b Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Wed, 20 May 2026 19:21:44 -0500 Subject: [PATCH 02/23] connmgr: Add whitelist detection tests. This adds tests to ensure the new whitelist detection method works as expected. --- internal/connmgr/connmanager_test.go | 87 ++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index ecbb0ccf0c..b9e1f512a8 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -219,6 +219,93 @@ func TestNewConfig(t *testing.T) { }) } +// TestIsWhitelisted ensures [ConnManager.IsWhitelisted] works as expected. +func TestIsWhitelisted(t *testing.T) { + type perManagerTest struct { + addr string // address to test against whitelist + whitelisted bool // expected whitelisted result + } + + tests := []struct { + name string // test description + prefixes []netip.Prefix // CIDR prefixes to whitelist + perManagerTests []perManagerTest // tests to run against the prefixes + }{{ + name: "no whitelisted entries", + prefixes: nil, + perManagerTests: []perManagerTest{ + {"1.2.3.4:18555", false}, + {"127.0.0.1:18555", false}, + }, + }, { + name: "single /32 IPv4 entry", + prefixes: []netip.Prefix{ + netip.MustParsePrefix("1.2.3.4/32"), + }, + perManagerTests: []perManagerTest{ + {"1.2.3.4:18555", true}, + {"1.2.3.4:9108", true}, + {"[::1.2.3.4]:18555", false}, // IPv4 in IPv6 + {"1.2.3.5:18555", false}, + }, + }, { + name: "single /128 IPv6 entry", + prefixes: []netip.Prefix{ + netip.MustParsePrefix("::1.2.3.4/128"), + }, + perManagerTests: []perManagerTest{ + {"[::1.2.3.4]:18555", true}, + {"[::1.2.3.4]:9108", true}, + {"1.2.3.4:18555", false}, // IPv4 doesn't match IPv4 in IPv6 + {"[::1.2.3.5]:9108", false}, + }, + }, { + name: "mixed IPv4 and IPv6 with different prefix lengths", + prefixes: []netip.Prefix{ + netip.MustParsePrefix("12.13.14.0/24"), + netip.MustParsePrefix("20.21.22.23/8"), + netip.MustParsePrefix("fe80::/64"), + }, + perManagerTests: []perManagerTest{ + {"12.13.14.1:18555", true}, + {"12.13.14.255:18555", true}, + {"12.13.15.0:18555", false}, + {"20.0.0.0:18555", true}, + {"20.0.0.0:9108", true}, + {"20.255.255.255:18555", true}, + {"20.255.255.255:9108", true}, + {"21.0.0.0:18555", false}, + {"[fe80::1]:18555", true}, + {"[fe80::1]:9108", true}, + {"[fe80::ffff:ffff:ffff:ffff]:18555", true}, + {"[fe80::ffff:ffff:ffff:ffff]:1234", true}, + {"[fe80::1:ffff:ffff:ffff:ffff]:18555", false}, + }, + }} + + for _, test := range tests { + // Parse the whitelist entries for the test. + cmgr := newTestConnManager(t, &Config{ + Dial: mockDialer, + Whitelists: test.prefixes, + }) + + for _, pmTest := range test.perManagerTests { + mAddr := mockAddr{"tcp", pmTest.addr} + addr, err := stdlibNetAddrToAddrMgrNetAddr(mAddr) + if err != nil { + t.Fatalf("%q-%q: failed to parse address: %v", test.name, + pmTest.addr, err) + } + if got := cmgr.IsWhitelisted(addr); got != pmTest.whitelisted { + t.Errorf("%q-%q: mismatched result -- got %v, want %v", + test.name, pmTest.addr, got, pmTest.whitelisted) + continue + } + } + } +} + // assertConnID ensures the provided connection has the given ID. func assertConnID(t *testing.T, conn *Conn, wantID uint64) { t.Helper() From 45898a977acef1cd70521d4e1dfd931bd1730af3 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Mon, 18 May 2026 15:59:20 -0500 Subject: [PATCH 03/23] server: Integrate connmgr whitelisting. This modifies the server to pass in the parsed whitelist entries to the connection manager config and the relevant code to make use of the new method it exposes. Finally, it removes the no longer used local isWhitelisted method. --- server.go | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/server.go b/server.go index f61afaf33f..3a7b1b671e 100644 --- a/server.go +++ b/server.go @@ -513,6 +513,7 @@ func newServerPeer(s *server, conn *connmgr.Conn, remoteAddr *addrmgr.NetAddress conn: conn, remoteAddr: remoteAddr, persistent: s.connManager.IsPersistent(conn.ID()), + isWhitelisted: s.connManager.IsWhitelisted(remoteAddr), knownAddresses: apbf.NewFilter(maxKnownAddrsPerPeer, knownAddrsFPRate), quit: make(chan struct{}), getDataQueue: make(chan []*wire.InvVect, maxConcurrentGetDataReqs), @@ -2289,7 +2290,6 @@ func (s *server) inboundPeerConnected(ctx context.Context, conn *connmgr.Conn) { sp := newServerPeer(s, conn, remoteNetAddr) sp.Peer = peer.NewInboundPeer(newPeerConfig(sp), conn) - sp.isWhitelisted = isWhitelisted(remoteNetAddr) if err := sp.Handshake(ctx, sp.OnVersion); err != nil { srvrLog.Debugf("Failed handshake for inbound peer %s: %v", remoteNetAddr, err) @@ -2323,7 +2323,6 @@ func (s *server) outboundPeerConnected(ctx context.Context, conn *connmgr.Conn) sp := newServerPeer(s, conn, remoteNetAddr) sp.Peer = peer.NewOutboundPeer(newPeerConfig(sp), conn.RemoteAddr(), conn) - sp.isWhitelisted = isWhitelisted(remoteNetAddr) if err := sp.Handshake(ctx, sp.OnVersion); err != nil { srvrLog.Debugf("Failed handshake for outbound peer %s: %v", conn.RemoteAddr(), err) @@ -4367,6 +4366,7 @@ func newServer(ctx context.Context, profiler *profileServer, s.outboundPeerConnected(ctx, conn) }, GetNewAddress: newAddressFunc, + Whitelists: cfg.whitelists, }) if err != nil { return nil, err @@ -4631,19 +4631,3 @@ func addLocalAddress(addrMgr *addrmgr.AddrManager, addr string, services wire.Se return nil } - -// isWhitelisted returns whether the IP address is included in the whitelisted -// networks and IPs. -func isWhitelisted(addr *addrmgr.NetAddress) bool { - if len(cfg.whitelists) == 0 { - return false - } - - ip, _ := netip.AddrFromSlice(addr.IP) - for _, prefix := range cfg.whitelists { - if prefix.Contains(ip) { - return true - } - } - return false -} From 600d4dfe6669e25188ee8f4cbaa3da3843d78e36 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Thu, 21 May 2026 17:10:47 -0500 Subject: [PATCH 04/23] connmgr: Update README.md for whitelist support. --- internal/connmgr/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/connmgr/README.md b/internal/connmgr/README.md index b9299d864f..cf43ac92a4 100644 --- a/internal/connmgr/README.md +++ b/internal/connmgr/README.md @@ -33,6 +33,8 @@ The following is a brief overview of the key features: - Supports manual connection establishment via `Connect` - Duplicate address prevention - Rejects duplicate connections to and from the same address (host:port) +- Whitelist support + - CIDR-based whitelists that allow bypassing certain limits and restrictions - Rich managed connections via `Conn` - Connection types for differentiated handling - Automatic cleanup on connection close From 6003b590d1fe8087a3f89ba680d5c7ca54d42dd0 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Tue, 19 May 2026 14:11:47 -0500 Subject: [PATCH 05/23] connmgr: Add try acquire support to semaphore. This adds a new TryAcquire method to the context-aware semaphore. As the name implies, the method supports conditionally acquiring the semaphore only when resources are immediately available. In other words, it will not block when there are no resources immediately available. --- internal/connmgr/semaphore.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/internal/connmgr/semaphore.go b/internal/connmgr/semaphore.go index fb7d7eed4a..5f831d9be3 100644 --- a/internal/connmgr/semaphore.go +++ b/internal/connmgr/semaphore.go @@ -27,6 +27,27 @@ func (s semaphore) Acquire(ctx context.Context) bool { return true } +// TryAcquire attempts to acquire the semaphore without blocking when there are +// no resources immediately available. +// +// It returns true with a nil error on success. It returns false with a nil +// error when the semaphore is at capacity and no permit is available. +// +// Finally, it returns false with the error associated with the context +// immediately when the context is already canceled or timed out at the time of +// the call. It does not attempt to acquire the semaphore in that case. +func (s semaphore) TryAcquire(ctx context.Context) (bool, error) { + if ctx.Err() != nil { + return false, ctx.Err() + } + select { + case s <- struct{}{}: + return true, nil + default: + } + return false, nil +} + // Release release the semaphore. func (s semaphore) Release() { select { From 77306016dbd2d44927f1a4e7615e86969f304de8 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Tue, 19 May 2026 14:11:48 -0500 Subject: [PATCH 06/23] connmgr: Add semaphore try acquire tests. This adds tests for the new TryAcquire method on the context-aware semaphore to ensure the semantics work as expected. --- internal/connmgr/semaphore_test.go | 75 +++++++++++++++++++++++++++++- 1 file changed, 73 insertions(+), 2 deletions(-) diff --git a/internal/connmgr/semaphore_test.go b/internal/connmgr/semaphore_test.go index 9542176df1..1d7d5b7501 100644 --- a/internal/connmgr/semaphore_test.go +++ b/internal/connmgr/semaphore_test.go @@ -6,6 +6,7 @@ package connmgr import ( "context" + "errors" "testing" "time" ) @@ -21,10 +22,24 @@ func TestSemaphore(t *testing.T) { return sem.Acquire(ctx) } + // Create a closure that tries to acquire a semaphore via the nonblocking + // method with a timeout. + timedTryAcquire := func(sem semaphore, timeout time.Duration) (bool, error) { + ctx, cancel := context.WithTimeout(ctx, timeout) + if timeout == 0 { + cancel() + } else { + defer cancel() + } + return sem.TryAcquire(ctx) + } + // perSemTest describes a test to run against the same semaphore. type perSemTest struct { name string // test description numAcquires uint32 // num to acquire + numTries uint32 // num to try to acquire via nonblocking method + cancelTry bool // whether or not to cancel nonblocking try numReleases uint32 // num to release } @@ -69,6 +84,45 @@ func TestSemaphore(t *testing.T) { numReleases: 5, }}, want: []bool{true, true, true, true, true, true, true, false}, + }, { + name: "nonblocking tryacquire and blocking acquire mixed", + cap: 3, + perSemTests: []perSemTest{{ + name: "cap 3 (0 acquired): try 1, release 2", + numTries: 1, + numReleases: 2, + }, { + name: "cap 3 (0 acquired): acquire 2, try 1, release 1", + numAcquires: 2, + numTries: 1, + numReleases: 1, + }, { + name: "cap 3 (2 acquired): acquire 1, try 2, release 3", + numAcquires: 1, + numTries: 2, + numReleases: 3, + }}, + want: []bool{true, true, true, true, true, false, false}, + }, { + name: "nonblocking tryacquire with canceled context", + cap: 1, + perSemTests: []perSemTest{{ + name: "cap 1 (0 acquired): try 1 (canceled), release 0", + numTries: 1, + cancelTry: true, + numReleases: 0, + }, { + name: "cap 1 (0 acquired): acquire 1, try 1, release 1", + numAcquires: 1, + numTries: 1, + numReleases: 1, + }, { + name: "cap 1 (0 acquired): try 2, release 1", + numAcquires: 0, + numTries: 2, + numReleases: 1, + }}, + want: []bool{false, true, false, true, false}, }} for _, test := range tests { @@ -77,13 +131,30 @@ func TestSemaphore(t *testing.T) { sem := makeSemaphore(test.cap) results := make([]bool, 0, len(test.want)) - // Perform each sequence of acquires and releases as specified by the - // per semaphore tests. + // Perform each sequence of acquires, try acquires, and releases as + // specified by the per semaphore tests. for _, psTest := range test.perSemTests { const timeout = 10 * time.Millisecond for range psTest.numAcquires { results = append(results, timedAcquire(sem, timeout)) } + for range psTest.numTries { + // Override timeout with a duration 0 and expected error when + // the flag to force the context for the try acquire to be + // canceled is specified. + var wantErr error + tryTimeout := timeout + if psTest.cancelTry { + tryTimeout = 0 + wantErr = context.DeadlineExceeded + } + acquired, err := timedTryAcquire(sem, tryTimeout) + if !errors.Is(err, wantErr) { + t.Fatalf("%q: unexpected try acquire error: got %v, want %v", + psTest.name, err, wantErr) + } + results = append(results, acquired) + } for range psTest.numReleases { sem.Release() } From 627bf8094e3998ccb822eef1ada31cd81cad21cd Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Tue, 19 May 2026 14:11:49 -0500 Subject: [PATCH 07/23] connmgr: Limit total overall normal connections. The current overall total connection limits are enforced by the server rather than the connection manager. This is not ideal for many reasons, but one of the most important consequences is that it makes DoS attacks easier. Another example of some less than ideal behavior that it allows is that some rare combinations of events can lead to temporary extra connection churn. It is much more robust and natural to perform the limiting in the connection manager itself via semaphores. That approach not only significantly hardens the server against DoS attacks and solves various edge cases present in the current code, it also paves the way for even more advanced features such as traffic shaping in the future. To that end, this adds semaphore-based limiting for the total overall number of normal connections to the connection manager and removes the relevant current limiting for it from the server. Normal connections are the automatic outbound, manual outbound, and inbound connections. Persistent connections, on the other hand, are not subject to the limit since they have their own limiting. This is consistent with them not being subject to the automatic target outbound limit either. --- internal/connmgr/connmanager.go | 138 +++++++++++++++++++++++++++----- internal/connmgr/error.go | 4 + internal/connmgr/error_test.go | 1 + internal/connmgr/log.go | 9 +++ rpcadaptors.go | 8 -- server.go | 12 +-- 6 files changed, 133 insertions(+), 39 deletions(-) diff --git a/internal/connmgr/connmanager.go b/internal/connmgr/connmanager.go index a2df96398b..ac1bd885fe 100644 --- a/internal/connmgr/connmanager.go +++ b/internal/connmgr/connmanager.go @@ -44,6 +44,10 @@ const ( // base times the number of retries that have been done. defaultMaxRetryDuration = time.Minute * 5 + // defaultMaxNormalConns is the default maximum number of normal inbound, + // outbound, and pending connections to permit. + defaultMaxNormalConns = 125 + // defaultTargetOutbound is the default number of outbound connections to // maintain. defaultTargetOutbound = 8 @@ -233,11 +237,25 @@ type Config struct { // connections in that case. OnAccept func(*Conn) + // MaxNormalConns is the maximum number of normal inbound, outbound, and + // pending connections to permit. Defaults to 125. + // + // Persistent connections do not count against this limit. They have their + // own maximum defined by [MaxPersistent]. + // + // Whitelisted connections and some connections with special permissions are + // also exempt. As a result, the total number of connections may exceed + // this value. + MaxNormalConns uint32 + // TargetOutbound is the number of outbound network connections to maintain // automatically. Defaults to 8. // // Persistent connections do not count against this value. They have their // own maximum limit defined by [MaxPersistent]. + // + // This will be forced to the smaller of the specified value (or its default + // value when unspecified) and [Config.MaxNormalConns]. TargetOutbound uint32 // RetryDuration is the duration to wait before retrying connection @@ -290,10 +308,16 @@ type ConnManager struct { // It is a buffered channel with size [MaxPersistent]. runPersistentChan chan *persistentEntry - // outboundSem limits the number of active outbound connections. It does - // not apply to persistent connections which are separately limited to - // [MaxPersistent]. - activeOutboundsSem semaphore + // These semaphores are used to enforce max limits on the number of + // connections of different kinds. They do not apply to persistent + // connections which are separately limited to [MaxPersistent]. + // + // totalNormalConnsSem limits the total overall number of normal inbound, + // outbound, and pending connections. + // + // outboundSem limits the number of active outbound connections. + totalNormalConnsSem semaphore + activeOutboundsSem semaphore // The fields below this point are all protected by the connection mutex. connMtx sync.Mutex @@ -694,9 +718,13 @@ func (cm *ConnManager) dial(ctx context.Context, addr net.Addr, connType Connect // the connection manager. // // Attempts to dial addresses that already have an established, pending, or -// persistent connection will return an error as described below. +// persistent connection or would exceed max allowed limits will return an error +// as described below. +// +// The connection will have type [ConnTypeManual] and the following connection +// limits are enforced: // -// The connection will have type [ConnTypeManual]. +// - Total normal connections ([Config.MaxNormalConns]) // // Note that the context parameter to this function and the lifecycle context // may be independent. @@ -710,6 +738,8 @@ func (cm *ConnManager) dial(ctx context.Context, addr net.Addr, connType Connect // to the address // - [ErrAlreadyConnected] when there is already an established connection to // the address +// - [ErrMaxNormalConns] when there are already the maximum allowed number of +// normal connections (inbound, outbound, and pending) // - [ErrShutdown] when the connection manager is shutting down // - [context.Canceled] or [context.DeadlineExceeded] depending on the // provided context or when the dialer fails to establish a connection @@ -717,7 +747,21 @@ func (cm *ConnManager) dial(ctx context.Context, addr net.Addr, connType Connect // // This function is safe for concurrent access. func (cm *ConnManager) Connect(ctx context.Context, addr net.Addr) (*Conn, error) { - conn, err := cm.dial(ctx, addr, ConnTypeManual, nil, nil) + acquired, err := cm.totalNormalConnsSem.TryAcquire(ctx) + if err != nil { + if sErr := cm.checkShutdown(); sErr != nil { + return nil, sErr + } + return nil, err + } + if !acquired { + maxAllowed := cm.cfg.MaxNormalConns + str := fmt.Sprintf("a maximum of %d %s is allowed", maxAllowed, + pickNoun(maxAllowed, "connection", "connections")) + return nil, MakeError(ErrMaxNormalConns, str) + } + onClose := cm.totalNormalConnsSem.Release + conn, err := cm.dial(ctx, addr, ConnTypeManual, onClose, nil) if err != nil { return nil, err } @@ -847,6 +891,18 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) defer log.Tracef("Listener handler done for %s", listener.Addr()) for ctx.Err() == nil { + // The following is intentionally implementing active connection + // shedding by accepting connections and then immediately disconnecting + // them after the [net.Listener.Accept] call if any policies are + // violated. + // + // Reversing it and blocking until a permit is available and only then + // calling Accept would cause the connections to build up in the kernel. + // Then, since the kernel will still create the 3-way handshake, clients + // would connect and hang until their own timeouts are hit, and, + // eventually, the entire service could appear entirely down if the SYN + // queue were to fill. It also would not allow implementing better + // additional policies. netConn, err := listener.Accept() if err != nil { // Only log the error if not forcibly shutting down. @@ -881,7 +937,29 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) } cm.connMtx.Unlock() - go func(netConn net.Conn) { + // Require a permit to allow the inbound connection unless the address + // has special permissions (e.g. whitelisted). + // + // Attempt to acquire a permit via a non-blocking call and immediately + // disconnect if unsuccessful so that all blocking happens on + // [net.Listener.Accept] for the reasons described above. + requirePermit := !cm.IsWhitelisted(rAddr) + if requirePermit { + acquired, err := cm.totalNormalConnsSem.TryAcquire(ctx) + if err != nil { + netConn.Close() + continue + } + if !acquired { + maxAllowed := cm.cfg.MaxNormalConns + log.Debugf("Dropped connection from %v: a maximum of %d %s is "+ + "allowed", rAddr, maxAllowed, pickNoun(maxAllowed, + "connection", "connections")) + netConn.Close() + continue + } + } + go func(netConn net.Conn, requirePermit bool) { // Create a new connection instance with the next globally unique // connection ID, add an entry to the map that tracks all active // connections, and invoke the configured accept callback with it. @@ -897,6 +975,9 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) cm.connMtx.Unlock() log.Debugf("Disconnected from %v (id: %d, type: %v)", rAddr, id, connType) + if requirePermit { + cm.totalNormalConnsSem.Release() + } } conn = newConn(cm, netConn, id, connType, rAddr, onClose) cm.connMtx.Lock() @@ -905,7 +986,7 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) log.Debugf("Accepted connection from %v (id: %d, type: %v)", rAddr, id, connType) cm.cfg.OnAccept(conn) - }(netConn) + }(netConn, requirePermit) } } @@ -1147,16 +1228,28 @@ func (cm *ConnManager) targetOutboundHandler(ctx context.Context) { return } + // Wait for a permit to make another overall connection. This limits + // the total number of normal connections while the previous limits the + // total number of automatic outbound connections. + if !cm.totalNormalConnsSem.Acquire(ctx) { + cm.activeOutboundsSem.Release() + return + } + addr, err := cm.cfg.GetNewAddress() if err != nil { failedAttempts.Add(1) log.Debugf("Failed to get address for outbound connection: %v", err) + cm.totalNormalConnsSem.Release() cm.activeOutboundsSem.Release() continue } go func(addr net.Addr) { - onClose := cm.activeOutboundsSem.Release + onClose := func() { + cm.totalNormalConnsSem.Release() + cm.activeOutboundsSem.Release() + } conn, err := cm.dial(ctx, addr, ConnTypeOutbound, onClose, nil) if err != nil { failedAttempts.Add(1) @@ -1252,23 +1345,28 @@ func New(cfg *Config) (*ConnManager, error) { if cfg.Dial == nil { return nil, MakeError(ErrDialNil, "dial cannot be nil") } - // Default to sane values + // Default to sane values. if cfg.RetryDuration <= 0 { cfg.RetryDuration = defaultRetryDuration } + if cfg.MaxNormalConns == 0 { + cfg.MaxNormalConns = defaultMaxNormalConns + } if cfg.TargetOutbound == 0 { cfg.TargetOutbound = defaultTargetOutbound } + cfg.TargetOutbound = min(cfg.TargetOutbound, cfg.MaxNormalConns) cm := ConnManager{ - cfg: *cfg, // Copy so caller can't mutate - quit: make(chan struct{}), - maxRetryDuration: defaultMaxRetryDuration, - runPersistentChan: make(chan *persistentEntry, MaxPersistent), - activeOutboundsSem: makeSemaphore(cfg.TargetOutbound), - persistent: make(map[uint64]*persistentEntry, MaxPersistent), - pending: make(map[uint64]*pendingConnInfo), - active: make(map[uint64]*Conn, cfg.TargetOutbound), - connIDByAddr: make(map[string]uint64), + cfg: *cfg, // Copy so caller can't mutate + quit: make(chan struct{}), + maxRetryDuration: defaultMaxRetryDuration, + runPersistentChan: make(chan *persistentEntry, MaxPersistent), + totalNormalConnsSem: makeSemaphore(cfg.MaxNormalConns), + activeOutboundsSem: makeSemaphore(cfg.TargetOutbound), + persistent: make(map[uint64]*persistentEntry, MaxPersistent), + pending: make(map[uint64]*pendingConnInfo), + active: make(map[uint64]*Conn, cfg.TargetOutbound), + connIDByAddr: make(map[string]uint64), } return &cm, nil } diff --git a/internal/connmgr/error.go b/internal/connmgr/error.go index 9c87dad6e3..6b313d89f1 100644 --- a/internal/connmgr/error.go +++ b/internal/connmgr/error.go @@ -22,6 +22,10 @@ const ( // already has an established connection. ErrAlreadyConnected = ErrorKind("ErrAlreadyConnected") + // ErrMaxNormalConns indicates a connection attempt (inbound or outbound) + // would exceed the maximum allowed number of normal connections. + ErrMaxNormalConns = ErrorKind("ErrMaxNormalConns") + // ErrMaxPersistent indicates an attempt to add more than the maximum // allowed number of persistent connections. ErrMaxPersistent = ErrorKind("ErrMaxPersistent") diff --git a/internal/connmgr/error_test.go b/internal/connmgr/error_test.go index d4e5d2262e..b3881bf7c6 100644 --- a/internal/connmgr/error_test.go +++ b/internal/connmgr/error_test.go @@ -19,6 +19,7 @@ func TestErrorKindStringer(t *testing.T) { {ErrDialNil, "ErrDialNil"}, {ErrAlreadyPending, "ErrAlreadyPending"}, {ErrAlreadyConnected, "ErrAlreadyConnected"}, + {ErrMaxNormalConns, "ErrMaxNormalConns"}, {ErrMaxPersistent, "ErrMaxPersistent"}, {ErrDuplicatePersistent, "ErrDuplicatePersistent"}, {ErrNotFound, "ErrNotFound"}, diff --git a/internal/connmgr/log.go b/internal/connmgr/log.go index 4bf44f5799..f6ba6f5f48 100644 --- a/internal/connmgr/log.go +++ b/internal/connmgr/log.go @@ -19,3 +19,12 @@ var log = slog.Disabled func UseLogger(logger slog.Logger) { log = logger } + +// pickNoun returns the singular or plural form of a noun depending on the count +// n. +func pickNoun[T ~uint32 | ~uint64](n T, singular, plural string) string { + if n == 1 { + return singular + } + return plural +} diff --git a/rpcadaptors.go b/rpcadaptors.go index a6826e0471..b88fd5bee8 100644 --- a/rpcadaptors.go +++ b/rpcadaptors.go @@ -130,14 +130,6 @@ func (cm *rpcConnManager) Connect(ctx context.Context, addr string, permanent bo return err } - // Limit max number of total peers. - cm.server.peerState.Lock() - count := cm.server.peerState.count() - cm.server.peerState.Unlock() - if count >= cfg.MaxPeers { - return errors.New("max peers reached") - } - // Attempt to add a persistent peer when requested. connManager := cm.server.connManager if permanent { diff --git a/server.go b/server.go index 3a7b1b671e..5d019fdb4c 100644 --- a/server.go +++ b/server.go @@ -2703,17 +2703,6 @@ func (s *server) handleAddPeer(sp *serverPeer) bool { return false } - // Limit max number of total peers. However, allow whitelisted inbound - // peers regardless. - if state.count()+1 > cfg.MaxPeers && !isInboundWhitelisted { - srvrLog.Infof("Max peers reached [%d] - disconnecting peer %s", - cfg.MaxPeers, sp) - sp.Disconnect() - // TODO: how to handle permanent peers here? - // they should be rescheduled. - return false - } - // Add the new peer. if sp.Inbound() { state.inboundPeers[sp.ID()] = sp @@ -4359,6 +4348,7 @@ func newServer(ctx context.Context, profiler *profileServer, s.inboundPeerConnected(ctx, conn) }, RetryDuration: connectionRetryInterval, + MaxNormalConns: uint32(cfg.MaxPeers), TargetOutbound: s.targetOutbound, Dial: s.attemptDcrdDial, DialTimeout: cfg.DialTimeout, From a347447e9be861b717837119ed80401bc7619d81 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Tue, 19 May 2026 14:11:50 -0500 Subject: [PATCH 08/23] connmgr: Add total max normal conns tests. This adds tests to ensure that the new max normal connection limiting properly enforces the limit including automatic outbound, manual outbound, and inbound connections. It also ensures that it not applied to persistent connections. --- internal/connmgr/connmanager_test.go | 174 +++++++++++++++++++++++++++ 1 file changed, 174 insertions(+) diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index b9e1f512a8..d914b3ecc8 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -1710,3 +1710,177 @@ func TestRejectDuplicateConns(t *testing.T) { wg.Wait() assertConnManagerCleanShutdown(t, cmgr) } + +// TestMaxNormalConns ensures the connection manager limits the total number of +// normal connections to [Config.MaxNormalConns] including automatic outbound, +// manual outbound, and inbound connections. It also ensures that it is not +// applied to persistent connections. +func TestMaxNormalConns(t *testing.T) { + t.Parallel() + + // nextAddr is a convenience func to return a new unique address with every + // invocation. + var numAddrs atomic.Uint32 + nextAddr := func() net.Addr { + addrStr := fmt.Sprintf("10.0.0.%d:18555", numAddrs.Add(1)) + return mustParseAddrPort(addrStr) + } + + // Create an address that will be whitelisted. + whitelistedIP := netip.MustParseAddr("220.0.0.1") + whitelistedAddr := mustParseAddrPort(whitelistedIP.String() + ":1024") + + // Constants for the number of various normal connection types to test + // overall max normal connection limits. + const ( + targetOutbound = 3 + targetManual = 4 + targetInbound = 5 + maxNormalConns = targetOutbound + targetManual + targetInbound + ) + connected := make(chan *Conn) + disconnected := make(chan *Conn) + inboundConns := make(chan *Conn) + listener := newMockListener("127.0.0.1:9108") + var pauseTargetOutbound atomic.Bool + var totalPausedAddrs atomic.Uint32 + hitMaxFailedAttempts := make(chan struct{}) + cmgr := newTestConnManager(t, &Config{ + Listeners: []net.Listener{listener}, + MaxNormalConns: maxNormalConns, + TargetOutbound: targetOutbound, + RetryDuration: 50 * time.Millisecond, + Dial: mockDialer, + Whitelists: []netip.Prefix{netip.PrefixFrom(whitelistedIP, 32)}, + OnAccept: func(conn *Conn) { + inboundConns <- conn + }, + GetNewAddress: func() (net.Addr, error) { + if pauseTargetOutbound.Load() { + total := totalPausedAddrs.Add(1) + if total == maxFailedAttempts { + hitMaxFailedAttempts <- struct{}{} + } + return nil, errors.New("network down") + } + return nextAddr(), nil + }, + OnConnection: func(conn *Conn) { + connected <- conn + }, + OnDisconnection: func(conn *Conn) { + disconnected <- conn + }, + }) + cmgr.maxRetryDuration = cmgr.cfg.RetryDuration + ctx, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + + // Wait for the expected number of target outbound conns to be established. + outbounds := make([]*Conn, 0, targetOutbound) + for len(outbounds) < targetOutbound { + conn := assertConnReceived(t, connected, 0, ConnTypeOutbound) + outbounds = append(outbounds, conn) + } + assertConnManagerInternalState(t, cmgr) + + // Establish target number of inbounds to the listener and wait for them to + // be established. + go func() { + for range targetInbound { + listener.Connect(nextAddr()) + } + }() + inbounds := make([]*Conn, 0, targetInbound) + for len(inbounds) < targetInbound { + conn := assertConnReceived(t, inboundConns, 0, ConnTypeInbound) + inbounds = append(inbounds, conn) + } + assertConnManagerInternalState(t, cmgr) + + // Establish target number of manual connections and wait for them to be + // established. + go func() { + for range targetManual { + go cmgr.Connect(ctx, nextAddr()) + } + }() + manualConns := make([]*Conn, 0, targetManual+1) + for len(manualConns) < targetManual { + conn := assertConnReceived(t, connected, 0, ConnTypeManual) + manualConns = append(manualConns, conn) + } + assertConnManagerInternalState(t, cmgr) + + // Ensure manual connections that would exceed the max allowed normal + // connections are rejected. + _, err := cmgr.Connect(ctx, nextAddr()) + if !errors.Is(err, ErrMaxNormalConns) { + t.Fatalf("did not reject manual connection at max allowed, err: %v", err) + } + assertConnManagerInternalState(t, cmgr) + + // Ensure inbound connections that would exceed the max allowed normal + // connections are rejected. + go listener.Connect(nextAddr()) + assertNoConnReceived(t, inboundConns) + assertConnManagerInternalState(t, cmgr) + + // Ensure inbound connections from whitelisted addresses are exempt from the + // max allowed normal connections by establishing one while at the limit. + go listener.Connect(whitelistedAddr) + conn := assertConnReceived(t, inboundConns, 0, ConnTypeInbound) + assertConnManagerInternalState(t, cmgr) + + // Ensure closing the whitelisted connection does not release a permit it + // never acquired by asserting inbound connections that would exceed the max + // allowed normal connections are still rejected after the close. + conn.Close() + assertConnReceived(t, disconnected, conn.ID(), ConnTypeInbound) + go listener.Connect(nextAddr()) + assertNoConnReceived(t, inboundConns) + assertConnManagerInternalState(t, cmgr) + + // Pause the target outbound dials and remove one of the target outbound + // connections to make room for another manual connection. Then wait for + // the max failures to be hit so attempts are paused for a retry timeout. + pauseTargetOutbound.Store(true) + outboundConn := outbounds[0] + outboundConn.Close() + assertConnReceived(t, disconnected, outboundConn.ID(), ConnTypeOutbound) + select { + case <-hitMaxFailedAttempts: + time.Sleep(connTestReceiveTimeout) + case <-time.After(maxFailedAttempts * connTestReceiveTimeout): + t.Fatal("did not reach max failed attempts before timeout") + } + assertConnManagerInternalState(t, cmgr) + + // Establish another manual connection to take the place of the target + // outbound connection that was just closed and wait for it to be + // established. + go cmgr.Connect(ctx, nextAddr()) + assertConnReceived(t, connected, 0, ConnTypeManual) + assertConnManagerInternalState(t, cmgr) + + // Unpause the target outbound dials and ensure no additional automatic + // outbound connections are made despite being under the target outbound due + // to max total conns. + pauseTargetOutbound.Store(false) + assertNoConnReceivedTimeout(t, connected, connTestNonReceiveTimeout+ + cmgr.cfg.RetryDuration) + assertConnManagerInternalState(t, cmgr) + + // Ensure persistent connections are not subject to the max total normal + // connections by adding one and waiting for it to be established. + connID, err := cmgr.AddPersistent(nextAddr()) + if err != nil { + t.Fatalf("failed to add persistent connection: %v", err) + } + assertConnReceived(t, connected, connID, ConnTypeManual) + assertConnManagerInternalState(t, cmgr) + + // Ensure clean shutdown of connection manager. + shutdown() + wg.Wait() + assertConnManagerCleanShutdown(t, cmgr) +} From b525f3c59e34c1c6fa327a3e8c7544daa6e7e4fe Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Thu, 21 May 2026 17:14:30 -0500 Subject: [PATCH 09/23] connmgr: Update README.md for total conn limits. --- internal/connmgr/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/connmgr/README.md b/internal/connmgr/README.md index cf43ac92a4..850f2620e1 100644 --- a/internal/connmgr/README.md +++ b/internal/connmgr/README.md @@ -31,6 +31,8 @@ The following is a brief overview of the key features: with exponential backoff on disconnect - Manual connections - Supports manual connection establishment via `Connect` +- Connection limits + - Limits total normal (non-persistent) connections to `MaxNormalConns` - Duplicate address prevention - Rejects duplicate connections to and from the same address (host:port) - Whitelist support From ed8d59bb58b9822dc2d191697d30924f3af23d48 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Tue, 26 May 2026 20:50:11 -0500 Subject: [PATCH 10/23] connmgr: Wait for all goroutines to finish. This modifies the persistent connection and target outbound handler shutdown logic to wait for any goroutines they have launched to finish before returning. This ensures there are no dangling goroutines from them once Run returns. --- internal/connmgr/connmanager.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/internal/connmgr/connmanager.go b/internal/connmgr/connmanager.go index ac1bd885fe..7c13dbaa48 100644 --- a/internal/connmgr/connmanager.go +++ b/internal/connmgr/connmanager.go @@ -1169,6 +1169,10 @@ func (cm *ConnManager) runPersistent(ctx context.Context, connID uint64, addr ne // persistentConnsHandler handles launching individual goroutines for persistent // connections. func (cm *ConnManager) persistentConnsHandler(ctx context.Context) { + // Ensure all persistent handlers are done before returning. + var wg sync.WaitGroup + defer wg.Wait() + for { select { case entry := <-cm.runPersistentChan: @@ -1176,7 +1180,11 @@ func (cm *ConnManager) persistentConnsHandler(ctx context.Context) { cm.connMtx.Lock() entry.cancel = cancel cm.connMtx.Unlock() - go cm.runPersistent(pCtx, entry.id, entry.addr) + wg.Add(1) + go func() { + cm.runPersistent(pCtx, entry.id, entry.addr) + wg.Done() + }() case <-ctx.Done(): return @@ -1193,6 +1201,10 @@ func (cm *ConnManager) targetOutboundHandler(ctx context.Context) { log.Trace("Starting target outbound handler") defer log.Trace("Target outbound handler done") + // Ensure potential pending dial cleanup is done before returning. + var wg sync.WaitGroup + defer wg.Wait() + // failedAttempts tracks the total number of failed outbound connection // attempts since the last successful connection. It is primarily used to // detect network outages in order to impose a retry timeout on achieving @@ -1245,7 +1257,9 @@ func (cm *ConnManager) targetOutboundHandler(ctx context.Context) { continue } + wg.Add(1) go func(addr net.Addr) { + defer wg.Done() onClose := func() { cm.totalNormalConnsSem.Release() cm.activeOutboundsSem.Release() From 88406e88fc9d7785c57c7404c456641434330a6b Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Tue, 26 May 2026 20:50:13 -0500 Subject: [PATCH 11/23] connmgr: Use concrete addrmgr types in test. This updates the primary parsing method in the connection manager tests to return a concrete addrmgr address instead of a stdlib net.TCPAddr. The goal is to eventually use the concrete address type almost everywhere to avoid a lot of the less than ideal address reparsing and repeated host/port splitting and joining. --- internal/connmgr/connmanager_test.go | 34 +++++++++++++--------------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index d914b3ecc8..edc6f0a096 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -17,6 +17,8 @@ import ( "sync/atomic" "testing" "time" + + "github.com/decred/dcrd/addrmgr/v4" ) const ( @@ -33,16 +35,13 @@ const ( connTestNonReceiveTimeout = 20 * time.Millisecond ) -// mustParseAddrPort parses the provided address into a [*net.TCPAddr] and will -// panic if there is an error. It will only (and must only) be called with -// hard-coded, and therefore known good, addresses. -func mustParseAddrPort(addr string) *net.TCPAddr { +// mustParseAddrPort parses the provided address into a [*addrmgr.NetAddress] +// and will panic if there is an error. It will only (and must only) be called +// with hard-coded, and therefore known good, addresses. +func mustParseAddrPort(addr string) *addrmgr.NetAddress { addrPort := netip.MustParseAddrPort(addr) - return &net.TCPAddr{ - IP: addrPort.Addr().AsSlice(), - Port: int(addrPort.Port()), - Zone: addrPort.Addr().Zone(), - } + return addrmgr.NewNetAddressFromIPPort(addrPort.Addr().AsSlice(), + addrPort.Port(), 0) } // runConnMgrAsync invokes the Run method on the passed connection manager in a @@ -329,7 +328,7 @@ func assertConnType(t *testing.T, conn *Conn, wantType ConnectionType) { // pendingAddrConnID returns the connection ID associated with the pending // connection attempt for the provided address. The second return value will be // false if no pending attempt is found. -func pendingAddrConnID(cm *ConnManager, addr net.Addr) (uint64, bool) { +func pendingAddrConnID(cm *ConnManager, addr *addrmgr.NetAddress) (uint64, bool) { cm.connMtx.Lock() defer cm.connMtx.Unlock() addrStr := addr.String() @@ -343,7 +342,7 @@ func pendingAddrConnID(cm *ConnManager, addr net.Addr) (uint64, bool) { // assertPendingAddr ensures there is a pending connection with the given // address. -func assertPendingAddr(t *testing.T, cm *ConnManager, addr net.Addr) { +func assertPendingAddr(t *testing.T, cm *ConnManager, addr *addrmgr.NetAddress) { t.Helper() if _, ok := pendingAddrConnID(cm, addr); !ok { @@ -353,7 +352,7 @@ func assertPendingAddr(t *testing.T, cm *ConnManager, addr net.Addr) { // assertRemovedPersistent ensures there are no persistent conns with the // provided address. -func assertRemovedPersistent(t *testing.T, cm *ConnManager, addr net.Addr) { +func assertRemovedPersistent(t *testing.T, cm *ConnManager, addr *addrmgr.NetAddress) { t.Helper() if _, ok := cm.FindPersistentAddrID(addr); ok { @@ -871,9 +870,8 @@ func TestRetryPersistent(t *testing.T) { connected := make(chan *Conn) disconnected := make(chan *Conn) cmgr := newTestConnManager(t, &Config{ - RetryDuration: time.Millisecond, - TargetOutbound: 1, - Dial: mockDialer, + RetryDuration: time.Millisecond, + Dial: mockDialer, OnConnection: func(conn *Conn) { connected <- conn }, @@ -935,7 +933,7 @@ func TestMaxPersistent(t *testing.T) { _, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) var numAddrs uint32 - nextAddr := func() net.Addr { + nextAddr := func() *addrmgr.NetAddress { numAddrs++ addrStr := fmt.Sprintf("127.0.0.%d:18555", numAddrs) return mustParseAddrPort(addrStr) @@ -943,7 +941,7 @@ func TestMaxPersistent(t *testing.T) { // Add the maximum allowed number of persistent conns. connIDs := make([]uint64, 0, MaxPersistent) - addrs := make([]net.Addr, 0, MaxPersistent) + addrs := make([]*addrmgr.NetAddress, 0, MaxPersistent) for range MaxPersistent { addr := nextAddr() connID, err := cmgr.AddPersistent(addr) @@ -1721,7 +1719,7 @@ func TestMaxNormalConns(t *testing.T) { // nextAddr is a convenience func to return a new unique address with every // invocation. var numAddrs atomic.Uint32 - nextAddr := func() net.Addr { + nextAddr := func() *addrmgr.NetAddress { addrStr := fmt.Sprintf("10.0.0.%d:18555", numAddrs.Add(1)) return mustParseAddrPort(addrStr) } From 0635bd36d0e905f67c321f75f02633d81df3ddeb Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Tue, 26 May 2026 20:50:14 -0500 Subject: [PATCH 12/23] connmgr: Use concrete addr type more often. This modifies the address handling to parse the stdlib net.Addr to a concrete addrmgr address earlier in the connect and persistent paths rather than doing it in the dial method and switch the callback to get new addresses to return the concrete type. The server is updated to simply the return the chosen address which no longer needs to be converted. Note that this is theoretically a semantics change because the code in server previously potentially resolved the address via DNS and that no longer is the case. In practice, nothing is really changing in terms of resolution though because the address manager only ever works with resolved addresses since it needs to gossip addresses via the wire protocol which only deals with resolved addresses. --- internal/connmgr/connmanager.go | 44 +++++++++++++++++++--------- internal/connmgr/connmanager_test.go | 8 ++--- server.go | 6 ++-- 3 files changed, 37 insertions(+), 21 deletions(-) diff --git a/internal/connmgr/connmanager.go b/internal/connmgr/connmanager.go index 7c13dbaa48..4479ed81bc 100644 --- a/internal/connmgr/connmanager.go +++ b/internal/connmgr/connmanager.go @@ -271,7 +271,7 @@ type Config struct { // GetNewAddress is a way to get an address to make a network connection // to. If nil, no new connections will be made automatically. - GetNewAddress func() (net.Addr, error) + GetNewAddress func() (*addrmgr.NetAddress, error) // Dial connects to the address on the named network. Dial func(ctx context.Context, network, addr string) (net.Conn, error) @@ -377,6 +377,12 @@ func (cm *ConnManager) checkShutdown() error { // stdlibNetAddrToAddrMgrNetAddr converts the provided standard lib [net.Addr] // to a concrete address manager address. func stdlibNetAddrToAddrMgrNetAddr(addr net.Addr) (*addrmgr.NetAddress, error) { + // Fast path for most addresses. + if na, ok := addr.(*addrmgr.NetAddress); ok { + return na, nil + } + + // Fall back to slower string parsing. host, portStr, err := net.SplitHostPort(addr.String()) if err != nil { str := fmt.Sprintf("unable to split address %q", addr) @@ -575,7 +581,7 @@ func (cm *ConnManager) rejectDuplicateAddr(addr *addrmgr.NetAddress) error { // before the timeout configured for the connection manager // // This function is safe for concurrent access. -func (cm *ConnManager) dial(ctx context.Context, addr net.Addr, connType ConnectionType, onClose func(), persistentConnID *uint64) (*Conn, error) { +func (cm *ConnManager) dial(ctx context.Context, addr *addrmgr.NetAddress, connType ConnectionType, onClose func(), persistentConnID *uint64) (*Conn, error) { var skipOnClose bool defer func() { if !skipOnClose && onClose != nil { @@ -592,11 +598,6 @@ func (cm *ConnManager) dial(ctx context.Context, addr net.Addr, connType Connect return nil, ctx.Err() } - rAddr, err := stdlibNetAddrToAddrMgrNetAddr(addr) - if err != nil { - return nil, err - } - // Reject attempts to dial addresses that are already connected (or in the // process of it). Additionally, reject attempts to dial existing // persistent addresses unless a persistent connection ID was provided @@ -609,7 +610,7 @@ func (cm *ConnManager) dial(ctx context.Context, addr net.Addr, connType Connect rejectFn = cm.rejectConnectedAddr } cm.connMtx.Lock() - if err := rejectFn(rAddr); err != nil { + if err := rejectFn(addr); err != nil { cm.connMtx.Unlock() log.Debugf("Rejected connection: %v", err) return nil, err @@ -633,7 +634,7 @@ func (cm *ConnManager) dial(ctx context.Context, addr net.Addr, connType Connect } else { connID = cm.nextConnID.Add(1) } - info := &pendingConnInfo{connID, rAddr, cancel} + info := &pendingConnInfo{connID, addr, cancel} cm.addPendingInfo(info) cm.connMtx.Unlock() defer func() { @@ -705,7 +706,7 @@ func (cm *ConnManager) dial(ctx context.Context, addr net.Addr, connType Connect // Create a new connection instance with the connection ID and type and add // an entry to the map that tracks all active connections. - conn = newConn(cm, netConn, connID, connType, rAddr, dialOnClose) + conn = newConn(cm, netConn, connID, connType, addr, dialOnClose) cm.addActiveConn(conn) cm.connMtx.Unlock() @@ -747,6 +748,11 @@ func (cm *ConnManager) dial(ctx context.Context, addr net.Addr, connType Connect // // This function is safe for concurrent access. func (cm *ConnManager) Connect(ctx context.Context, addr net.Addr) (*Conn, error) { + rAddr, err := stdlibNetAddrToAddrMgrNetAddr(addr) + if err != nil { + return nil, err + } + acquired, err := cm.totalNormalConnsSem.TryAcquire(ctx) if err != nil { if sErr := cm.checkShutdown(); sErr != nil { @@ -761,7 +767,7 @@ func (cm *ConnManager) Connect(ctx context.Context, addr net.Addr) (*Conn, error return nil, MakeError(ErrMaxNormalConns, str) } onClose := cm.totalNormalConnsSem.Release - conn, err := cm.dial(ctx, addr, ConnTypeManual, onClose, nil) + conn, err := cm.dial(ctx, rAddr, ConnTypeManual, onClose, nil) if err != nil { return nil, err } @@ -1083,7 +1089,7 @@ func (cm *ConnManager) FindPersistentAddrID(addr net.Addr) (uint64, bool) { // increasing backoff, up to a maximum for repeated failed attempts. // // This MUST be run as a goroutine. -func (cm *ConnManager) runPersistent(ctx context.Context, connID uint64, addr net.Addr) { +func (cm *ConnManager) runPersistent(ctx context.Context, connID uint64, addr *addrmgr.NetAddress) { // Ensure the connection is closed when the goroutine exits. var conn *Conn defer func() { @@ -1192,6 +1198,16 @@ func (cm *ConnManager) persistentConnsHandler(ctx context.Context) { } } +// pickOutboundAddr returns an address suitable for establishing a new outbound +// connection. +// +// It simply delegates to [Config.GetNewAddress] for now. +// +// This function is safe for concurrent access. +func (cm *ConnManager) pickOutboundAddr() (*addrmgr.NetAddress, error) { + return cm.cfg.GetNewAddress() +} + // targetOutboundHandler attempts to automatically maintain the target number of // outbound connections configured via [Config.TargetOutbound] when initially // creating the connection manager. @@ -1248,7 +1264,7 @@ func (cm *ConnManager) targetOutboundHandler(ctx context.Context) { return } - addr, err := cm.cfg.GetNewAddress() + addr, err := cm.pickOutboundAddr() if err != nil { failedAttempts.Add(1) log.Debugf("Failed to get address for outbound connection: %v", err) @@ -1258,7 +1274,7 @@ func (cm *ConnManager) targetOutboundHandler(ctx context.Context) { } wg.Add(1) - go func(addr net.Addr) { + go func(addr *addrmgr.NetAddress) { defer wg.Done() onClose := func() { cm.totalNormalConnsSem.Release() diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index edc6f0a096..ca4c544130 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -794,7 +794,7 @@ func TestTargetOutbound(t *testing.T) { cmgr := newTestConnManager(t, &Config{ TargetOutbound: targetOutbound, Dial: mockDialer, - GetNewAddress: func() (net.Addr, error) { + GetNewAddress: func() (*addrmgr.NetAddress, error) { addrStr := fmt.Sprintf("127.0.0.%d:18555", nextAddr.Add(1)) return mustParseAddrPort(addrStr), nil }, @@ -827,7 +827,7 @@ func TestDoubleClose(t *testing.T) { cmgr := newTestConnManager(t, &Config{ TargetOutbound: 1, Dial: mockDialer, - GetNewAddress: func() (net.Addr, error) { + GetNewAddress: func() (*addrmgr.NetAddress, error) { return mustParseAddrPort("127.0.0.1:18555"), nil }, OnConnection: func(conn *Conn) { @@ -1080,7 +1080,7 @@ func TestNetworkFailure(t *testing.T) { TargetOutbound: targetOutbound, RetryDuration: retryTimeout, Dial: errDialer, - GetNewAddress: func() (net.Addr, error) { + GetNewAddress: func() (*addrmgr.NetAddress, error) { addrStr := fmt.Sprintf("127.0.0.%d:18555", nextAddr.Add(1)) return mustParseAddrPort(addrStr), nil }, @@ -1753,7 +1753,7 @@ func TestMaxNormalConns(t *testing.T) { OnAccept: func(conn *Conn) { inboundConns <- conn }, - GetNewAddress: func() (net.Addr, error) { + GetNewAddress: func() (*addrmgr.NetAddress, error) { if pauseTargetOutbound.Load() { total := totalPausedAddrs.Add(1) if total == maxFailedAttempts { diff --git a/server.go b/server.go index 5d019fdb4c..b9deed7435 100644 --- a/server.go +++ b/server.go @@ -4285,7 +4285,7 @@ func newServer(ctx context.Context, profiler *profileServer, // to specified peers and actively avoid advertising and connecting to // discovered peers in order to prevent it from becoming a public test // network. - var newAddressFunc func() (net.Addr, error) + var newAddressFunc func() (*addrmgr.NetAddress, error) if !cfg.SimNet && !cfg.RegNet && len(cfg.ConnectPeers) == 0 { filter := func(addrType addrmgr.NetAddressType) bool { switch addrType { @@ -4297,7 +4297,7 @@ func newServer(ctx context.Context, profiler *profileServer, } return false } - newAddressFunc = func() (net.Addr, error) { + newAddressFunc = func() (*addrmgr.NetAddress, error) { for tries := 0; tries < 100; tries++ { addr := s.addrManager.GetAddress(filter) if addr == nil { @@ -4331,7 +4331,7 @@ func newServer(ctx context.Context, profiler *profileServer, continue } - return addrStringToNetAddr(netAddr.Key()) + return netAddr, nil } return nil, errors.New("no valid connect address") From 1c0c23adfada2bd63947e41b4e9594e1bca4899e Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Thu, 21 May 2026 16:44:17 -0500 Subject: [PATCH 13/23] connmgr: Limit max connections per host. Similar to the recent total normal connection limiting, the current per-host connection limits are enforced by the server. For similar reasons, it is much more robust and natural to perform the limiting early in the connection manager. With that in mind, this implements the per-host connection limiting in the connection manager and removes the relevant current limiting for it from the server. The limiting is applied to inbound, outbound, and persistent connections. The new limiting is handled early in both the inbound and outbound paths now which allows it to take advantage of fast connection shedding for inbound connections and to preemptively prevent all outbound attempts that would exceed the limit regardless of source. It also provides the flexibility to apply independent special permissions in the future. This also slightly changes the semantics to exempt whitelisted addresses for both inbound and outbound connections as opposed to only inbound connections. --- internal/connmgr/connmanager.go | 118 +++++++++++++++++++++++++++++++- internal/connmgr/error.go | 4 ++ server.go | 38 ++-------- 3 files changed, 126 insertions(+), 34 deletions(-) diff --git a/internal/connmgr/connmanager.go b/internal/connmgr/connmanager.go index 4479ed81bc..50f008d1f8 100644 --- a/internal/connmgr/connmanager.go +++ b/internal/connmgr/connmanager.go @@ -48,6 +48,11 @@ const ( // outbound, and pending connections to permit. defaultMaxNormalConns = 125 + // defaultMaxConnsPerHost is the default maximum number of connections with + // the same host to permit. It does not apply to whitelisted or loopback + // addresses. + defaultMaxConnsPerHost = 5 + // defaultTargetOutbound is the default number of outbound connections to // maintain. defaultTargetOutbound = 8 @@ -248,6 +253,21 @@ type Config struct { // this value. MaxNormalConns uint32 + // MaxConnsPerHost is the maximum number of connections with the same host + // to permit. Defaults to 5. + // + // This applies to inbound, outbound, and persistent connections. However, + // in practice, it is highly unlikely that outbound connections will hit the + // default limit (unless intentionally connecting manually) because: + // + // - connections to the same host:port are rejected and it is extremely rare + // for the same host to serve multiple instances on different ports + // - all automatic outbound connections are heavily biased toward different + // network groups + // + // This limit is not applied to whitelisted or loopback connections. + MaxConnsPerHost uint32 + // TargetOutbound is the number of outbound network connections to maintain // automatically. Defaults to 8. // @@ -344,6 +364,11 @@ type ConnManager struct { // (host:port). It is kept in sync with the persistent, pending, and active // maps and is primarily used to efficiently reject duplicate connections. connIDByAddr map[string]uint64 + + // perHostCounts provides fast O(1) lookup of the number of entries per + // host. It is kept in sync with the persistent, pending, and active maps + // and is primarily used to efficiently enforce per-host connection limits. + perHostCounts map[string]uint32 } // IsWhitelisted returns whether the IP address is included in the whitelisted @@ -409,6 +434,23 @@ func stdlibNetAddrToAddrMgrNetAddr(addr net.Addr) (*addrmgr.NetAddress, error) { return netAddr, nil } +// addrHostKey returns the host portion of the passed address as a string +// suitable for use as a map key. +func addrHostKey(addr *addrmgr.NetAddress) string { + return net.IP(addr.IP).String() +} + +// decrementPerHostCount decrements the reference count for the provided host +// and cleans up the associated entry when there are no more references. +// +// This function MUST be called with the connection mutex held (writes). +func (cm *ConnManager) decrementPerHostCount(hostKey string) { + cm.perHostCounts[hostKey]-- + if cm.perHostCounts[hostKey] == 0 { + delete(cm.perHostCounts, hostKey) + } +} + // addPendingInfo adds information about a pending connection attempt to the // local state. // @@ -417,6 +459,7 @@ func (cm *ConnManager) addPendingInfo(info *pendingConnInfo) { cm.pending[info.id] = info if _, ok := cm.persistent[info.id]; !ok { cm.connIDByAddr[info.addr.String()] = info.id + cm.perHostCounts[addrHostKey(info.addr)]++ } } @@ -427,6 +470,7 @@ func (cm *ConnManager) removePendingInfo(info *pendingConnInfo) { delete(cm.pending, info.id) if _, ok := cm.persistent[info.id]; !ok { delete(cm.connIDByAddr, info.addr.String()) + cm.decrementPerHostCount(addrHostKey(info.addr)) } } @@ -437,6 +481,7 @@ func (cm *ConnManager) addActiveConn(conn *Conn) { cm.active[conn.id] = conn if _, ok := cm.persistent[conn.id]; !ok { cm.connIDByAddr[conn.remoteAddr.String()] = conn.id + cm.perHostCounts[addrHostKey(&conn.remoteAddr)]++ } } @@ -454,6 +499,7 @@ func (cm *ConnManager) removeActiveConn(conn *Conn) { delete(cm.active, conn.id) if _, ok := cm.persistent[conn.id]; !ok { delete(cm.connIDByAddr, conn.remoteAddr.String()) + cm.decrementPerHostCount(addrHostKey(&conn.remoteAddr)) } } @@ -463,6 +509,7 @@ func (cm *ConnManager) removeActiveConn(conn *Conn) { func (cm *ConnManager) addPersistentEntry(entry *persistentEntry) { cm.persistent[entry.id] = entry cm.connIDByAddr[entry.addr.String()] = entry.id + cm.perHostCounts[addrHostKey(entry.addr)]++ } // removePersistentEntry removes a persistent connection entry from the local @@ -475,6 +522,7 @@ func (cm *ConnManager) removePersistentEntry(entry *persistentEntry) { _, active := cm.active[entry.id] if !pending && !active { delete(cm.connIDByAddr, entry.addr.String()) + cm.decrementPerHostCount(addrHostKey(entry.addr)) } } @@ -547,6 +595,39 @@ func (cm *ConnManager) rejectDuplicateAddr(addr *addrmgr.NetAddress) error { return nil } +// rejectMaxConnsPerHost returns an error if adding an additional connection +// with the provided host address would exceed [Config.MaxConnsPerHost] and is +// not exempt. +// +// The persistent flag signals the check is a dial for a persistent connection +// that has already been registered. +// +// This function MUST be called with the connection mutex held (reads). +func (cm *ConnManager) rejectMaxConnsPerHost(addr *addrmgr.NetAddress, isWhitelisted, isPersistent bool) error { + // Whitelisted and loopback addresses are exempt. + isLoopback := net.IP(addr.IP).IsLoopback() + if isWhitelisted || isLoopback { + return nil + } + + maxAllowed := cm.cfg.MaxConnsPerHost + numConns := cm.perHostCounts[addrHostKey(addr)] + + // Persistent connections already own a slot in the per-host counts, so + // decrement to compensate. The count should always be at least one for a + // persistent connection, but be paranoid and double check for safety. + if isPersistent && numConns > 0 { + numConns-- + } + if numConns+1 > maxAllowed { + str := fmt.Sprintf("a maximum of %d %s per host is allowed", maxAllowed, + pickNoun(maxAllowed, "connection", "connections")) + return MakeError(ErrMaxConnsPerHost, str) + } + + return nil +} + // dial attempts to connect to the provided address and returns a connection // configured with the provided params on success. // @@ -559,6 +640,10 @@ func (cm *ConnManager) rejectDuplicateAddr(addr *addrmgr.NetAddress) error { // and pending connections are rejected when a non-nil persistent connection ID // is passed. // +// The following connection limits are enforced: +// +// - Total connections with the same host ([Config.MaxConnsPerHost]) +// // On success, the returned connection is configured to remove itself from the // set of all active connections and invoke the provided on close callback (if // set) when it is closed. @@ -575,6 +660,8 @@ func (cm *ConnManager) rejectDuplicateAddr(addr *addrmgr.NetAddress) error { // to the address // - [ErrAlreadyConnected] when there is already an established connection to // the address +// - [ErrMaxConnsPerHost] when there are already the maximum allowed number of +// connections (pending, active, and persistent) with the same host // - [ErrShutdown] when the connection manager is shutting down // - [context.Canceled] or [context.DeadlineExceeded] depending on the // provided context or when the dialer fails to establish a connection @@ -597,6 +684,7 @@ func (cm *ConnManager) dial(ctx context.Context, addr *addrmgr.NetAddress, connT if ctx.Err() != nil { return nil, ctx.Err() } + isWhitelisted := cm.IsWhitelisted(addr) // Reject attempts to dial addresses that are already connected (or in the // process of it). Additionally, reject attempts to dial existing @@ -616,6 +704,15 @@ func (cm *ConnManager) dial(ctx context.Context, addr *addrmgr.NetAddress, connT return nil, err } + // Limit the max number of connections per host. + isPersistent := persistentConnID != nil + err := cm.rejectMaxConnsPerHost(addr, isWhitelisted, isPersistent) + if err != nil { + cm.connMtx.Unlock() + log.Debugf("Rejected connection to %v: %v", addr, err) + return nil, err + } + // Apply a dial timeout if requested. Otherwise, use a regular cancel // context to support canceling the pending connection later. var cancel context.CancelFunc @@ -726,6 +823,7 @@ func (cm *ConnManager) dial(ctx context.Context, addr *addrmgr.NetAddress, connT // limits are enforced: // // - Total normal connections ([Config.MaxNormalConns]) +// - Total connections with the same host ([Config.MaxConnsPerHost]) // // Note that the context parameter to this function and the lifecycle context // may be independent. @@ -741,6 +839,8 @@ func (cm *ConnManager) dial(ctx context.Context, addr *addrmgr.NetAddress, connT // the address // - [ErrMaxNormalConns] when there are already the maximum allowed number of // normal connections (inbound, outbound, and pending) +// - [ErrMaxConnsPerHost] when there are already the maximum allowed number of +// connections (pending, active, and persistent) with the same host // - [ErrShutdown] when the connection manager is shutting down // - [context.Canceled] or [context.DeadlineExceeded] depending on the // provided context or when the dialer fails to establish a connection @@ -925,6 +1025,7 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) netConn.Close() continue } + isWhitelisted := cm.IsWhitelisted(rAddr) // Reject connections with the same host:port as any existing pending, // established, or persistent connections. Note that this does NOT @@ -933,7 +1034,7 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) // // The aforementioned behavior is intentional as it allows connections // from the same host to be independently limited to more than one - // elsewhere. + // below. cm.connMtx.Lock() if err := cm.rejectDuplicateAddr(rAddr); err != nil { cm.connMtx.Unlock() @@ -941,6 +1042,15 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) netConn.Close() continue } + + // Limit the max number of connections per host. + err = cm.rejectMaxConnsPerHost(rAddr, isWhitelisted, false) + if err != nil { + cm.connMtx.Unlock() + log.Debugf("Dropped connection from %v: %v", rAddr, err) + netConn.Close() + continue + } cm.connMtx.Unlock() // Require a permit to allow the inbound connection unless the address @@ -949,7 +1059,7 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) // Attempt to acquire a permit via a non-blocking call and immediately // disconnect if unsuccessful so that all blocking happens on // [net.Listener.Accept] for the reasons described above. - requirePermit := !cm.IsWhitelisted(rAddr) + requirePermit := !isWhitelisted if requirePermit { acquired, err := cm.totalNormalConnsSem.TryAcquire(ctx) if err != nil { @@ -1382,6 +1492,9 @@ func New(cfg *Config) (*ConnManager, error) { if cfg.MaxNormalConns == 0 { cfg.MaxNormalConns = defaultMaxNormalConns } + if cfg.MaxConnsPerHost == 0 { + cfg.MaxConnsPerHost = defaultMaxConnsPerHost + } if cfg.TargetOutbound == 0 { cfg.TargetOutbound = defaultTargetOutbound } @@ -1397,6 +1510,7 @@ func New(cfg *Config) (*ConnManager, error) { pending: make(map[uint64]*pendingConnInfo), active: make(map[uint64]*Conn, cfg.TargetOutbound), connIDByAddr: make(map[string]uint64), + perHostCounts: make(map[string]uint32), } return &cm, nil } diff --git a/internal/connmgr/error.go b/internal/connmgr/error.go index 6b313d89f1..966475894d 100644 --- a/internal/connmgr/error.go +++ b/internal/connmgr/error.go @@ -26,6 +26,10 @@ const ( // would exceed the maximum allowed number of normal connections. ErrMaxNormalConns = ErrorKind("ErrMaxNormalConns") + // ErrMaxConnsPerHost indicates a connection attempt (inbound or outbound) + // would exceed the maximum allowed number of connections per host. + ErrMaxConnsPerHost = ErrorKind("ErrMaxConnsPerHost") + // ErrMaxPersistent indicates an attempt to add more than the maximum // allowed number of persistent connections. ErrMaxPersistent = ErrorKind("ErrMaxPersistent") diff --git a/server.go b/server.go index b9deed7435..9db24745b1 100644 --- a/server.go +++ b/server.go @@ -2618,20 +2618,6 @@ func (s *server) considerReportedAddr(from *serverPeer, addr *wire.NetAddress) { s.considerReportedAddrOutbound(from, addr) } -// connectionsWithIP returns the number of connections with the given IP. -// -// This function MUST be called with the embedded mutex locked (for reads). -func (ps *peerState) connectionsWithIP(ip net.IP) int { - var total int - ps.forAllPeers(func(sp *serverPeer) { - if ip.Equal(sp.remoteAddr.IP) { - total++ - } - - }) - return total -} - // handleAddPeer deals with adding new peers and includes logic such as // categorizing the type of peer, limiting the maximum allowed number of peers, // and local external address resolution. @@ -2690,19 +2676,6 @@ func (s *server) handleAddPeer(sp *serverPeer) bool { defer state.Unlock() state.Lock() - // Limit max number of connections from a single IP. However, allow - // whitelisted inbound peers and localhost connections regardless. - isInboundWhitelisted := sp.isWhitelisted && sp.Inbound() - peerIP := net.IP(sp.remoteAddr.IP) - if cfg.MaxSameIP > 0 && !isInboundWhitelisted && !peerIP.IsLoopback() && - state.connectionsWithIP(peerIP)+1 > cfg.MaxSameIP { - - srvrLog.Infof("Max connections with %s reached [%d] - disconnecting "+ - "peer", sp, cfg.MaxSameIP) - sp.Disconnect() - return false - } - // Add the new peer. if sp.Inbound() { state.inboundPeers[sp.ID()] = sp @@ -4347,11 +4320,12 @@ func newServer(ctx context.Context, profiler *profileServer, OnAccept: func(conn *connmgr.Conn) { s.inboundPeerConnected(ctx, conn) }, - RetryDuration: connectionRetryInterval, - MaxNormalConns: uint32(cfg.MaxPeers), - TargetOutbound: s.targetOutbound, - Dial: s.attemptDcrdDial, - DialTimeout: cfg.DialTimeout, + RetryDuration: connectionRetryInterval, + MaxNormalConns: uint32(cfg.MaxPeers), + MaxConnsPerHost: uint32(cfg.MaxSameIP), + TargetOutbound: s.targetOutbound, + Dial: s.attemptDcrdDial, + DialTimeout: cfg.DialTimeout, OnConnection: func(conn *connmgr.Conn) { s.outboundPeerConnected(ctx, conn) }, From 0015801acec74b291febbdf63d60258002a243d6 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Thu, 21 May 2026 16:44:17 -0500 Subject: [PATCH 14/23] connmgr: Add max per-host conn tests. This adds tests to ensure that the new max connections per host limiting properly enforces the limit including automatic outbound, manual outbound, inbound, and persistent connections. It also tests whitelisted addresses are exempt. --- internal/connmgr/connmanager_test.go | 209 ++++++++++++++++++++++++++- 1 file changed, 207 insertions(+), 2 deletions(-) diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index ca4c544130..30a59b4c12 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -143,14 +143,18 @@ func assertConnManagerInternalState(t *testing.T, cm *ConnManager) { // Assert the pending and active maps are mutually exclusive for both conn // IDs and addrs. // - // Also build a map of addrs to conn IDs in the pending, active, and - // persistent maps for the checks below. + // Also build a map of addrs to conn IDs and tally the per host counts in + // the pending, active, and persistent maps for the checks below. connIDByAddr := make(map[string]uint64) + perHostCounts := make(map[string]uint32) for id, info := range cm.pending { if _, ok := cm.active[id]; ok { t.Fatalf("conn ID %d is both pending and active", id) } connIDByAddr[info.addr.String()] = id + if _, ok := cm.persistent[id]; !ok { + perHostCounts[addrHostKey(info.addr)]++ + } } for id, conn := range cm.active { if _, ok := cm.pending[id]; ok { @@ -161,6 +165,9 @@ func assertConnManagerInternalState(t *testing.T, cm *ConnManager) { t.Fatalf("addr %s is both pending and active", addrStr) } connIDByAddr[addrStr] = id + if _, ok := cm.persistent[id]; !ok { + perHostCounts[addrHostKey(&conn.remoteAddr)]++ + } } for id, entry := range cm.persistent { // Assert the conn ID of established/pending persistent conns matches. @@ -169,6 +176,7 @@ func assertConnManagerInternalState(t *testing.T, cm *ConnManager) { t.Fatalf("conn ID for addr %s mismatch: %d != %d", addrStr, existingID, id) } + perHostCounts[addrHostKey(entry.addr)]++ connIDByAddr[addrStr] = id } @@ -178,6 +186,13 @@ func assertConnManagerInternalState(t *testing.T, cm *ConnManager) { t.Fatalf("mismatched conn ID by addr maps\ngot: %v\nwant %v", cm.connIDByAddr, connIDByAddr) } + + // Assert the per host counts match the values obtained from manually + // tallying them. + if !reflect.DeepEqual(cm.perHostCounts, perHostCounts) { + t.Fatalf("mismatched per host count maps\ngot: %v\nwant %v", + cm.perHostCounts, perHostCounts) + } } // assertConnManagerCleanShutdown ensures the internal state of the passed @@ -202,6 +217,10 @@ func assertConnManagerCleanShutdown(t *testing.T, cm *ConnManager) { t.Fatalf("conn ID by addr map not empty: %d entries", len(cm.connIDByAddr)) } + if len(cm.perHostCounts) != 0 { + t.Fatalf("per host counts map not empty: %d entries", + len(cm.perHostCounts)) + } } // TestNewConfig tests that new ConnManager config is validated as expected. @@ -1882,3 +1901,189 @@ func TestMaxNormalConns(t *testing.T) { wg.Wait() assertConnManagerCleanShutdown(t, cmgr) } + +// TestMaxConnsPerHost ensures the connection manager limits the total number of +// connections with the same host to [Config.MaxConnsPerHost] including +// automatic outbound, manual outbound, inbound, and persistent connections. It +// also tests whitelisted addresses are exempt. +func TestMaxConnsPerHost(t *testing.T) { + t.Parallel() + + // nextSameHost is a convenience func to return a new address to the same IP + // with a different port on every invocation. + var nextPort atomic.Uint32 + nextSameHost := func() *addrmgr.NetAddress { + addrStr := fmt.Sprintf("10.10.0.1:%d", nextPort.Add(1)+1024) + return mustParseAddrPort(addrStr) + } + + // nextSameHostWhitelisted is a convenience func to return a new address to + // the same whitelisted IP with a different port on every invocation. + allowedIP := netip.MustParseAddr("10.20.0.1") + nextSameWhitelistedHost := func() *addrmgr.NetAddress { + addrStr := fmt.Sprintf("%s:%d", allowedIP, nextPort.Add(1)+1024) + return mustParseAddrPort(addrStr) + } + + const maxConnsPerHost = 3 + const retryTimeout = 50 * time.Millisecond + connected := make(chan *Conn, 1) + disconnected := make(chan *Conn, 1) + inboundConns := make(chan *Conn) + listener := newMockListener("127.0.0.1:9108") + var pauseTargetOutbound atomic.Bool + var totalPausedAddrs atomic.Uint32 + hitMaxFailedAttempts := make(chan struct{}) + cmgr := newTestConnManager(t, &Config{ + Listeners: []net.Listener{listener}, + MaxNormalConns: 30, // High enough to not interfere with per-host tests. + MaxConnsPerHost: maxConnsPerHost, + TargetOutbound: maxConnsPerHost, + RetryDuration: retryTimeout, + Dial: mockDialer, + Whitelists: []netip.Prefix{netip.PrefixFrom(allowedIP, 32)}, + OnAccept: func(conn *Conn) { + inboundConns <- conn + }, + GetNewAddress: func() (*addrmgr.NetAddress, error) { + if pauseTargetOutbound.Load() { + total := totalPausedAddrs.Add(1) + if total == maxFailedAttempts { + close(hitMaxFailedAttempts) + } + return nil, errors.New("network down") + } + return nextSameHost(), nil + }, + OnConnection: func(conn *Conn) { + connected <- conn + }, + OnDisconnection: func(conn *Conn) { + disconnected <- conn + }, + }) + cmgr.maxRetryDuration = cmgr.cfg.RetryDuration + ctx, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + + // Wait for the maximum allowed non-whitelisted per-host automatic outbound + // conns. + outboundConns := make([]*Conn, 0, maxConnsPerHost) + for len(outboundConns) < maxConnsPerHost { + conn := assertConnReceived(t, connected, 0, ConnTypeOutbound) + outboundConns = append(outboundConns, conn) + } + assertConnManagerInternalState(t, cmgr) + + // Ensure non-whitelisted manual connections that would exceed the max + // allowed per-host connections are rejected. + _, err := cmgr.Connect(ctx, nextSameHost()) + if !errors.Is(err, ErrMaxConnsPerHost) { + t.Fatalf("did not reject manual connection at per-host limit, err: %v", + err) + } + assertConnManagerInternalState(t, cmgr) + + // Ensure non-whitelisted inbound connections that would exceed the max + // allowed per-host connections are rejected. + go listener.Connect(nextSameHost()) + assertNoConnReceived(t, inboundConns) + assertConnManagerInternalState(t, cmgr) + + // Ensure whitelisted manual connections are allowed to exceed the per-host + // limit. + for range maxConnsPerHost + 1 { + go cmgr.Connect(ctx, nextSameWhitelistedHost()) + assertConnReceived(t, connected, 0, ConnTypeManual) + } + + // Ensure whitelisted inbound connections are allowed to exceed the per-host + // limit. + go listener.Connect(nextSameWhitelistedHost()) + assertConnReceived(t, inboundConns, 0, ConnTypeInbound) + assertConnManagerInternalState(t, cmgr) + + // Ensure whitelisted persistent connections are allowed to exceed the + // per-host limit. + connID, err := cmgr.AddPersistent(nextSameWhitelistedHost()) + if err != nil { + t.Fatalf("failed to add persistent connection: %v", err) + } + assertConnReceived(t, connected, connID, ConnTypeManual) + assertConnManagerInternalState(t, cmgr) + + // Pause the target outbound dials and remove one of the target outbound + // connections to make room for another manual connection with the same + // host. Then wait for the max failures to be hit so attempts are paused + // for a retry timeout. + pauseTargetOutbound.Store(true) + outboundConn := outboundConns[0] + outboundConn.Close() + assertConnReceived(t, disconnected, outboundConn.ID(), ConnTypeOutbound) + select { + case <-hitMaxFailedAttempts: + time.Sleep(connTestReceiveTimeout) + case <-time.After(maxFailedAttempts * connTestReceiveTimeout): + t.Fatal("did not reach max failed attempts before timeout") + } + + // Ensure a new non-whitelisted manual connection to the same host now + // succeeds. + go cmgr.Connect(ctx, nextSameHost()) + assertConnReceived(t, connected, 0, ConnTypeManual) + assertConnManagerInternalState(t, cmgr) + + // Unpause the target outbound dials and ensure no additional automatic + // outbound connections to the same host are made despite being under the + // target outbound. + noConnWaitTimeout := connTestReceiveTimeout + cmgr.cfg.RetryDuration + pauseTargetOutbound.Store(false) + assertNoConnReceivedTimeout(t, connected, noConnWaitTimeout) + assertConnManagerInternalState(t, cmgr) + + // nextSameHost2 is a convenience func to return a new address to the same + // IP with a different port on every invocation. It uses a different IP + // than the one used above to start with a clean per-host count. + var nextPort2 atomic.Uint32 + nextSameHost2 := func() *addrmgr.NetAddress { + addrStr := fmt.Sprintf("20.30.0.1:%d", nextPort2.Add(1)+1024) + return mustParseAddrPort(addrStr) + } + + // Ensure persistent connections can achieve the per-host limit. + persistentConns := make([]*Conn, 0, maxConnsPerHost) + for len(persistentConns) < maxConnsPerHost { + connID, err := cmgr.AddPersistent(nextSameHost2()) + if err != nil { + t.Fatalf("failed to add persistent connection: %v", err) + } + conn := assertConnReceived(t, connected, connID, ConnTypeManual) + persistentConns = append(persistentConns, conn) + } + assertConnManagerInternalState(t, cmgr) + + // Ensure persistent connection reconnects are allowed at the per-host + // limit. Wait for the disconnect first and then for the reconnect to be + // established. + persistentConn := persistentConns[0] + connID = persistentConn.ID() + persistentConn.Close() + assertConnReceived(t, disconnected, connID, ConnTypeManual) + assertConnManagerInternalState(t, cmgr) + timeout := retryTimeout + connTestReceiveTimeout + assertConnReceivedTimeout(t, connected, timeout, connID, ConnTypeManual) + assertConnManagerInternalState(t, cmgr) + + // Ensure persistent connections are also subject to the max per-host + // connections by adding one and confirming it is NOT established. + _, err = cmgr.AddPersistent(nextSameHost2()) + if err != nil { + t.Fatalf("failed to add persistent connection: %v", err) + } + assertNoConnReceivedTimeout(t, connected, noConnWaitTimeout) + assertConnManagerInternalState(t, cmgr) + + // Ensure clean shutdown of connection manager. + shutdown() + wg.Wait() + assertConnManagerCleanShutdown(t, cmgr) +} From fdfca08b94c1db22869eaa9c892ece6ab4d466a5 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Thu, 21 May 2026 17:20:32 -0500 Subject: [PATCH 15/23] connmgr: Update README.md for per-host conn limits. --- internal/connmgr/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/connmgr/README.md b/internal/connmgr/README.md index 850f2620e1..30ebaef01a 100644 --- a/internal/connmgr/README.md +++ b/internal/connmgr/README.md @@ -33,6 +33,8 @@ The following is a brief overview of the key features: - Supports manual connection establishment via `Connect` - Connection limits - Limits total normal (non-persistent) connections to `MaxNormalConns` + - Limits per-host connections to `MaxConnsPerHost` with exemptions for + whitelisted and loopback addresses - Duplicate address prevention - Rejects duplicate connections to and from the same address (host:port) - Whitelist support From 9c4999ebc53b77fe022289603fd4bd098722cc91 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Sun, 5 Jul 2026 20:13:48 -0500 Subject: [PATCH 16/23] connmgr: Rework to use per-host permits -- to be squashed --- internal/connmgr/connmanager.go | 280 +++++++++++++++++++++++--------- internal/connmgr/error.go | 4 + internal/connmgr/error_test.go | 2 + 3 files changed, 206 insertions(+), 80 deletions(-) diff --git a/internal/connmgr/connmanager.go b/internal/connmgr/connmanager.go index 50f008d1f8..869065a555 100644 --- a/internal/connmgr/connmanager.go +++ b/internal/connmgr/connmanager.go @@ -7,6 +7,11 @@ // and persistent network connections with retry logic. package connmgr +// The required lock ordering that must be followed to avoid deadlocks is as +// follows: +// 1. connMtx +// 2. perHostCountsMtx + import ( "context" "errors" @@ -48,11 +53,6 @@ const ( // outbound, and pending connections to permit. defaultMaxNormalConns = 125 - // defaultMaxConnsPerHost is the default maximum number of connections with - // the same host to permit. It does not apply to whitelisted or loopback - // addresses. - defaultMaxConnsPerHost = 5 - // defaultTargetOutbound is the default number of outbound connections to // maintain. defaultTargetOutbound = 8 @@ -254,7 +254,8 @@ type Config struct { MaxNormalConns uint32 // MaxConnsPerHost is the maximum number of connections with the same host - // to permit. Defaults to 5. + // to permit. Setting this to 0 disables the limiting. Defaults to 0 + // (unlimited). // // This applies to inbound, outbound, and persistent connections. However, // in practice, it is highly unlikely that outbound connections will hit the @@ -339,6 +340,13 @@ type ConnManager struct { totalNormalConnsSem semaphore activeOutboundsSem semaphore + // perHostCounts provides fast O(1) lookup of the number of entries per + // host. It is used to efficiently enforce per-host connection limits. + // + // It is protected by the associated mutex. + perHostCountsMtx sync.Mutex + perHostCounts map[string]uint32 + // The fields below this point are all protected by the connection mutex. connMtx sync.Mutex @@ -364,11 +372,6 @@ type ConnManager struct { // (host:port). It is kept in sync with the persistent, pending, and active // maps and is primarily used to efficiently reject duplicate connections. connIDByAddr map[string]uint64 - - // perHostCounts provides fast O(1) lookup of the number of entries per - // host. It is kept in sync with the persistent, pending, and active maps - // and is primarily used to efficiently enforce per-host connection limits. - perHostCounts map[string]uint32 } // IsWhitelisted returns whether the IP address is included in the whitelisted @@ -440,17 +443,6 @@ func addrHostKey(addr *addrmgr.NetAddress) string { return net.IP(addr.IP).String() } -// decrementPerHostCount decrements the reference count for the provided host -// and cleans up the associated entry when there are no more references. -// -// This function MUST be called with the connection mutex held (writes). -func (cm *ConnManager) decrementPerHostCount(hostKey string) { - cm.perHostCounts[hostKey]-- - if cm.perHostCounts[hostKey] == 0 { - delete(cm.perHostCounts, hostKey) - } -} - // addPendingInfo adds information about a pending connection attempt to the // local state. // @@ -459,7 +451,6 @@ func (cm *ConnManager) addPendingInfo(info *pendingConnInfo) { cm.pending[info.id] = info if _, ok := cm.persistent[info.id]; !ok { cm.connIDByAddr[info.addr.String()] = info.id - cm.perHostCounts[addrHostKey(info.addr)]++ } } @@ -470,7 +461,6 @@ func (cm *ConnManager) removePendingInfo(info *pendingConnInfo) { delete(cm.pending, info.id) if _, ok := cm.persistent[info.id]; !ok { delete(cm.connIDByAddr, info.addr.String()) - cm.decrementPerHostCount(addrHostKey(info.addr)) } } @@ -481,7 +471,6 @@ func (cm *ConnManager) addActiveConn(conn *Conn) { cm.active[conn.id] = conn if _, ok := cm.persistent[conn.id]; !ok { cm.connIDByAddr[conn.remoteAddr.String()] = conn.id - cm.perHostCounts[addrHostKey(&conn.remoteAddr)]++ } } @@ -499,7 +488,6 @@ func (cm *ConnManager) removeActiveConn(conn *Conn) { delete(cm.active, conn.id) if _, ok := cm.persistent[conn.id]; !ok { delete(cm.connIDByAddr, conn.remoteAddr.String()) - cm.decrementPerHostCount(addrHostKey(&conn.remoteAddr)) } } @@ -509,7 +497,6 @@ func (cm *ConnManager) removeActiveConn(conn *Conn) { func (cm *ConnManager) addPersistentEntry(entry *persistentEntry) { cm.persistent[entry.id] = entry cm.connIDByAddr[entry.addr.String()] = entry.id - cm.perHostCounts[addrHostKey(entry.addr)]++ } // removePersistentEntry removes a persistent connection entry from the local @@ -522,7 +509,6 @@ func (cm *ConnManager) removePersistentEntry(entry *persistentEntry) { _, active := cm.active[entry.id] if !pending && !active { delete(cm.connIDByAddr, entry.addr.String()) - cm.decrementPerHostCount(addrHostKey(entry.addr)) } } @@ -595,39 +581,6 @@ func (cm *ConnManager) rejectDuplicateAddr(addr *addrmgr.NetAddress) error { return nil } -// rejectMaxConnsPerHost returns an error if adding an additional connection -// with the provided host address would exceed [Config.MaxConnsPerHost] and is -// not exempt. -// -// The persistent flag signals the check is a dial for a persistent connection -// that has already been registered. -// -// This function MUST be called with the connection mutex held (reads). -func (cm *ConnManager) rejectMaxConnsPerHost(addr *addrmgr.NetAddress, isWhitelisted, isPersistent bool) error { - // Whitelisted and loopback addresses are exempt. - isLoopback := net.IP(addr.IP).IsLoopback() - if isWhitelisted || isLoopback { - return nil - } - - maxAllowed := cm.cfg.MaxConnsPerHost - numConns := cm.perHostCounts[addrHostKey(addr)] - - // Persistent connections already own a slot in the per-host counts, so - // decrement to compensate. The count should always be at least one for a - // persistent connection, but be paranoid and double check for safety. - if isPersistent && numConns > 0 { - numConns-- - } - if numConns+1 > maxAllowed { - str := fmt.Sprintf("a maximum of %d %s per host is allowed", maxAllowed, - pickNoun(maxAllowed, "connection", "connections")) - return MakeError(ErrMaxConnsPerHost, str) - } - - return nil -} - // dial attempts to connect to the provided address and returns a connection // configured with the provided params on success. // @@ -660,7 +613,6 @@ func (cm *ConnManager) rejectMaxConnsPerHost(addr *addrmgr.NetAddress, isWhiteli // to the address // - [ErrAlreadyConnected] when there is already an established connection to // the address -// - [ErrMaxConnsPerHost] when there are already the maximum allowed number of // connections (pending, active, and persistent) with the same host // - [ErrShutdown] when the connection manager is shutting down // - [context.Canceled] or [context.DeadlineExceeded] depending on the @@ -684,7 +636,6 @@ func (cm *ConnManager) dial(ctx context.Context, addr *addrmgr.NetAddress, connT if ctx.Err() != nil { return nil, ctx.Err() } - isWhitelisted := cm.IsWhitelisted(addr) // Reject attempts to dial addresses that are already connected (or in the // process of it). Additionally, reject attempts to dial existing @@ -704,15 +655,6 @@ func (cm *ConnManager) dial(ctx context.Context, addr *addrmgr.NetAddress, connT return nil, err } - // Limit the max number of connections per host. - isPersistent := persistentConnID != nil - err := cm.rejectMaxConnsPerHost(addr, isWhitelisted, isPersistent) - if err != nil { - cm.connMtx.Unlock() - log.Debugf("Rejected connection to %v: %v", addr, err) - return nil, err - } - // Apply a dial timeout if requested. Otherwise, use a regular cancel // context to support canceling the pending connection later. var cancel context.CancelFunc @@ -811,6 +753,93 @@ func (cm *ConnManager) dial(ctx context.Context, addr *addrmgr.NetAddress, connT return conn, nil } +// noop is a no-op function used as the release callback for addresses that are +// exempt from per-host limiting (e.g. whitelisted, loopback, or when limiting +// is disabled). This allows uniform release handling by callers. +func noop() {} + +// needsHostPermit returns whether or not the provided address requires a +// per-host permit. +func (cm *ConnManager) needsHostPermit(addr *addrmgr.NetAddress) bool { + // Per-host limiting is disabled. + if cm.cfg.MaxConnsPerHost == 0 { + return false + } + + // Whitelisted and loopback addresses are exempt. + isWhitelisted := cm.IsWhitelisted(addr) + isLoopback := net.IP(addr.IP).IsLoopback() + if isWhitelisted || isLoopback { + return false + } + return true +} + +// releaseHostPermitFn returns a function that itself releases a host permit for +// the given host address. +// +// This function and the returned function are both safe for concurrent access. +func (cm *ConnManager) releaseHostPermitFn(addr *addrmgr.NetAddress) func() { + hostKey := addrHostKey(addr) + return func() { + cm.perHostCountsMtx.Lock() + cm.perHostCounts[hostKey]-- + if cm.perHostCounts[hostKey] == 0 { + delete(cm.perHostCounts, hostKey) + } + cm.perHostCountsMtx.Unlock() + } +} + +// reserveHostPermit attempts to acquire a permit for the provided host address. +// It returns [ErrMaxConnsPerHost] when no slot is available. Otherwise, it is +// up to the caller to release the permit when it is no longer needed. +// +// This function MUST be called with the per-host counts mutex held (writes). +func (cm *ConnManager) reserveHostPermit(addr *addrmgr.NetAddress) error { + hostKey := addrHostKey(addr) + + maxAllowed := cm.cfg.MaxConnsPerHost + numConns := cm.perHostCounts[hostKey] + if numConns+1 > maxAllowed { + str := fmt.Sprintf("a maximum of %d %s per host is allowed", maxAllowed, + pickNoun(maxAllowed, "connection", "connections")) + return MakeError(ErrMaxConnsPerHost, str) + } + + cm.perHostCounts[hostKey]++ + return nil +} + +// maybeReserveHostPermit attempts to acquire a permit for the provided host +// address and returns a function to release the permit. The caller MUST +// invoke the returned function when no error is returned and the permit is no +// longer needed. +// +// When the host address is exempt from per-host limiting, no slot is reserved +// and the returned release function does nothing. As a result, it is always +// safe to invoke the returned release function when no error is returned. +// +// It returns [ErrMaxConnsPerHost] when a slot is required and no slot is +// available. +// +// This function is safe for concurrent access. +func (cm *ConnManager) maybeReserveHostPermit(addr *addrmgr.NetAddress) (func(), error) { + // Nothing to do when no permit is required. + if !cm.needsHostPermit(addr) { + return noop, nil + } + + cm.perHostCountsMtx.Lock() + if err := cm.reserveHostPermit(addr); err != nil { + cm.perHostCountsMtx.Unlock() + return nil, err + } + cm.perHostCountsMtx.Unlock() + + return cm.releaseHostPermitFn(addr), nil +} + // Connect assigns an ID and dials a connection to the provided address using // the provided context and the dial function configured when initially creating // the connection manager. @@ -853,20 +882,31 @@ func (cm *ConnManager) Connect(ctx context.Context, addr net.Addr) (*Conn, error return nil, err } + // Limit the max number of connections per host unless exempt. + releaseHostPermit, err := cm.maybeReserveHostPermit(rAddr) + if err != nil { + return nil, err + } + acquired, err := cm.totalNormalConnsSem.TryAcquire(ctx) if err != nil { + releaseHostPermit() if sErr := cm.checkShutdown(); sErr != nil { return nil, sErr } return nil, err } if !acquired { + releaseHostPermit() maxAllowed := cm.cfg.MaxNormalConns str := fmt.Sprintf("a maximum of %d %s is allowed", maxAllowed, pickNoun(maxAllowed, "connection", "connections")) return nil, MakeError(ErrMaxNormalConns, str) } - onClose := cm.totalNormalConnsSem.Release + onClose := func() { + cm.totalNormalConnsSem.Release() + releaseHostPermit() + } conn, err := cm.dial(ctx, rAddr, ConnTypeManual, onClose, nil) if err != nil { return nil, err @@ -1042,16 +1082,15 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) netConn.Close() continue } + cm.connMtx.Unlock() - // Limit the max number of connections per host. - err = cm.rejectMaxConnsPerHost(rAddr, isWhitelisted, false) + // Limit the max number of connections per host unless exempt. + releaseHostPermit, err := cm.maybeReserveHostPermit(rAddr) if err != nil { - cm.connMtx.Unlock() log.Debugf("Dropped connection from %v: %v", rAddr, err) netConn.Close() continue } - cm.connMtx.Unlock() // Require a permit to allow the inbound connection unless the address // has special permissions (e.g. whitelisted). @@ -1063,10 +1102,12 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) if requirePermit { acquired, err := cm.totalNormalConnsSem.TryAcquire(ctx) if err != nil { + releaseHostPermit() netConn.Close() continue } if !acquired { + releaseHostPermit() maxAllowed := cm.cfg.MaxNormalConns log.Debugf("Dropped connection from %v: a maximum of %d %s is "+ "allowed", rAddr, maxAllowed, pickNoun(maxAllowed, @@ -1094,6 +1135,7 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) if requirePermit { cm.totalNormalConnsSem.Release() } + releaseHostPermit() } conn = newConn(cm, netConn, id, connType, rAddr, onClose) cm.connMtx.Lock() @@ -1106,6 +1148,46 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) } } +// rejectMaxPersistentsPerHost returns an error if registering another +// persistent connection for the provided host address would exceed +// [Config.MaxConnsPerHost] and is not exempt. +// +// Only other persistent connections are counted. This allows persistent +// connections to be registered even when all slots are currently used by +// temporary connections. The new persistent connection will retry until a +// slot becomes available. +// +// This function MUST be called with the connection mutex held (reads). +func (cm *ConnManager) rejectMaxPersistentsPerHost(addr *addrmgr.NetAddress) error { + // Nothing to do when no permit is required. + if !cm.needsHostPermit(addr) { + return nil + } + + // Count the number of registered persistent connections that already exist + // for the host address. + // + // Note that this intentionally does not interact with the per-host slot + // reservations or counts map for the reasons described in the function + // comment. + hostKey := addrHostKey(addr) + var count uint32 + for _, entry := range cm.persistent { + if addrHostKey(entry.addr) == hostKey { + count++ + } + } + + maxAllowed := cm.cfg.MaxConnsPerHost + if count+1 > maxAllowed { + str := fmt.Sprintf("a maximum of %d persistent %s per host is allowed", + maxAllowed, pickNoun(maxAllowed, "connection", "connections")) + return MakeError(ErrMaxPersistentPerHost, str) + } + + return nil +} + // AddPersistent adds an address the connection manager will attempt to always // maintain an established connection with until the persistent connection entry // is removed via [ConnManager.Remove] or the context associated with @@ -1117,6 +1199,10 @@ func (cm *ConnManager) listenHandler(ctx context.Context, listener net.Listener) // A maximum of [MaxPersistent] connections may be added. Attempting to add any // more will return [ErrMaxPersistent]. // +// A maximum of [Config.MaxConnsPerHost] connections to the same host address +// may be added unless the address is exempt. Attempting to add any more will +// return [ErrMaxPersistentPerHost]. +// // Adding a duplicate persistent address will return [ErrDuplicatePersistent] // and adding addresses that already have an established or pending connection // will return [ErrAlreadyConnected] or [ErrAlreadyPending], respectively. @@ -1157,6 +1243,11 @@ func (cm *ConnManager) AddPersistent(addr net.Addr) (uint64, error) { return 0, err } + // Limit the max number of persistent connections per host. + if err := cm.rejectMaxPersistentsPerHost(rAddr); err != nil { + return 0, err + } + entry := &persistentEntry{id: cm.nextConnID.Add(1), addr: rAddr} cm.addPersistentEntry(entry) log.Debugf("Added persistent connection to %v (id: %d)", addr, entry.id) @@ -1208,11 +1299,22 @@ func (cm *ConnManager) runPersistent(ctx context.Context, connID uint64, addr *a } }() + // Determine if the address requires limiting the max number of connections + // and setup a release func accordingly. The permit is acquired and + // released on a per-attempt basis which allows retrying until a slot + // becomes available. + requireHostPermit := cm.needsHostPermit(addr) + releaseHostPermit := noop + if requireHostPermit { + releaseHostPermit = cm.releaseHostPermitFn(addr) + } + // Setup a callback that notifies a disconnect channel for use below and // start with the channel signaled. disconnected := make(chan struct{}, 1) disconnected <- struct{}{} onClose := func() { + releaseHostPermit() disconnected <- struct{}{} } @@ -1263,6 +1365,17 @@ func (cm *ConnManager) runPersistent(ctx context.Context, connID uint64, addr *a retryAfter = nil } + // Limit the max number of connections per host unless exempt. + if requireHostPermit { + cm.perHostCountsMtx.Lock() + if err := cm.reserveHostPermit(addr); err != nil { + cm.perHostCountsMtx.Unlock() + log.Debugf("Rejected connection to %v: %v", addr, err) + continue + } + cm.perHostCountsMtx.Unlock() + } + lastAttempt = time.Now() var err error conn, err = cm.dial(ctx, addr, ConnTypeManual, onClose, &connID) @@ -1383,10 +1496,19 @@ func (cm *ConnManager) targetOutboundHandler(ctx context.Context) { continue } + // Limit the max number of connections per host unless exempt. + releaseHostPermit, err := cm.maybeReserveHostPermit(addr) + if err != nil { + cm.totalNormalConnsSem.Release() + cm.activeOutboundsSem.Release() + continue + } + wg.Add(1) go func(addr *addrmgr.NetAddress) { defer wg.Done() onClose := func() { + releaseHostPermit() cm.totalNormalConnsSem.Release() cm.activeOutboundsSem.Release() } @@ -1485,6 +1607,7 @@ func New(cfg *Config) (*ConnManager, error) { if cfg.Dial == nil { return nil, MakeError(ErrDialNil, "dial cannot be nil") } + // Default to sane values. if cfg.RetryDuration <= 0 { cfg.RetryDuration = defaultRetryDuration @@ -1492,9 +1615,6 @@ func New(cfg *Config) (*ConnManager, error) { if cfg.MaxNormalConns == 0 { cfg.MaxNormalConns = defaultMaxNormalConns } - if cfg.MaxConnsPerHost == 0 { - cfg.MaxConnsPerHost = defaultMaxConnsPerHost - } if cfg.TargetOutbound == 0 { cfg.TargetOutbound = defaultTargetOutbound } diff --git a/internal/connmgr/error.go b/internal/connmgr/error.go index 966475894d..9b65877ec9 100644 --- a/internal/connmgr/error.go +++ b/internal/connmgr/error.go @@ -34,6 +34,10 @@ const ( // allowed number of persistent connections. ErrMaxPersistent = ErrorKind("ErrMaxPersistent") + // ErrMaxPersistentPerHost indicates an attempt to add more than the maximum + // allowed number of persistent connections with the same host address. + ErrMaxPersistentPerHost = ErrorKind("ErrMaxPersistentPerHost") + // ErrDuplicatePersistent indicates an attempt to add a persistent // connection to an address that already exists. ErrDuplicatePersistent = ErrorKind("ErrDuplicatePersistent") diff --git a/internal/connmgr/error_test.go b/internal/connmgr/error_test.go index b3881bf7c6..4e351ad184 100644 --- a/internal/connmgr/error_test.go +++ b/internal/connmgr/error_test.go @@ -20,7 +20,9 @@ func TestErrorKindStringer(t *testing.T) { {ErrAlreadyPending, "ErrAlreadyPending"}, {ErrAlreadyConnected, "ErrAlreadyConnected"}, {ErrMaxNormalConns, "ErrMaxNormalConns"}, + {ErrMaxConnsPerHost, "ErrMaxConnsPerHost"}, {ErrMaxPersistent, "ErrMaxPersistent"}, + {ErrMaxPersistentPerHost, "ErrMaxPersistentPerHost"}, {ErrDuplicatePersistent, "ErrDuplicatePersistent"}, {ErrNotFound, "ErrNotFound"}, {ErrUnsupportedAddr, "ErrUnsupportedAddr"}, From f9975881db75fb545fe2493574c778aeb2567ec7 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Sun, 5 Jul 2026 20:24:00 -0500 Subject: [PATCH 17/23] connmgr: Update tests for reworked per-host limit -- to be squashed --- internal/connmgr/connmanager_test.go | 120 ++++++++++++++++++--------- 1 file changed, 83 insertions(+), 37 deletions(-) diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index 30a59b4c12..fbb707d66d 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -122,9 +122,9 @@ func newTestConnManager(t *testing.T, cfg *Config) *ConnManager { return cmgr } -// assertConnManagerInternalState ensures the internal state of the passed +// assertInternalConnState ensures the internal connection state of the passed // connection manager instance is coherent. -func assertConnManagerInternalState(t *testing.T, cm *ConnManager) { +func assertInternalConnState(t *testing.T, cm *ConnManager) { t.Helper() cm.connMtx.Lock() @@ -142,19 +142,12 @@ func assertConnManagerInternalState(t *testing.T, cm *ConnManager) { // Assert the pending and active maps are mutually exclusive for both conn // IDs and addrs. - // - // Also build a map of addrs to conn IDs and tally the per host counts in - // the pending, active, and persistent maps for the checks below. connIDByAddr := make(map[string]uint64) - perHostCounts := make(map[string]uint32) for id, info := range cm.pending { if _, ok := cm.active[id]; ok { t.Fatalf("conn ID %d is both pending and active", id) } connIDByAddr[info.addr.String()] = id - if _, ok := cm.persistent[id]; !ok { - perHostCounts[addrHostKey(info.addr)]++ - } } for id, conn := range cm.active { if _, ok := cm.pending[id]; ok { @@ -165,9 +158,6 @@ func assertConnManagerInternalState(t *testing.T, cm *ConnManager) { t.Fatalf("addr %s is both pending and active", addrStr) } connIDByAddr[addrStr] = id - if _, ok := cm.persistent[id]; !ok { - perHostCounts[addrHostKey(&conn.remoteAddr)]++ - } } for id, entry := range cm.persistent { // Assert the conn ID of established/pending persistent conns matches. @@ -176,7 +166,6 @@ func assertConnManagerInternalState(t *testing.T, cm *ConnManager) { t.Fatalf("conn ID for addr %s mismatch: %d != %d", addrStr, existingID, id) } - perHostCounts[addrHostKey(entry.addr)]++ connIDByAddr[addrStr] = id } @@ -186,6 +175,31 @@ func assertConnManagerInternalState(t *testing.T, cm *ConnManager) { t.Fatalf("mismatched conn ID by addr maps\ngot: %v\nwant %v", cm.connIDByAddr, connIDByAddr) } +} + +// assertInternalConnState ensures the internal per-host state of the passed +// connection manager instance is coherent. +func assertInternalPerHostState(t *testing.T, cm *ConnManager) { + t.Helper() + + cm.connMtx.Lock() + defer cm.connMtx.Unlock() + + cm.perHostCountsMtx.Lock() + defer cm.perHostCountsMtx.Unlock() + + perHostCounts := make(map[string]uint32) + for _, info := range cm.pending { + if cm.needsHostPermit(info.addr) { + perHostCounts[addrHostKey(info.addr)]++ + } + } + for _, conn := range cm.active { + addr := &conn.remoteAddr + if cm.needsHostPermit(addr) { + perHostCounts[addrHostKey(addr)]++ + } + } // Assert the per host counts match the values obtained from manually // tallying them. @@ -195,32 +209,49 @@ func assertConnManagerInternalState(t *testing.T, cm *ConnManager) { } } +// assertConnManagerInternalState ensures the internal state of the passed +// connection manager instance is coherent. +func assertConnManagerInternalState(t *testing.T, cm *ConnManager) { + t.Helper() + + assertInternalConnState(t, cm) + assertInternalPerHostState(t, cm) +} + // assertConnManagerCleanShutdown ensures the internal state of the passed // connection manager is fully cleaned up as expected. It must only be called // after [ConnManager.Run] returns. func assertConnManagerCleanShutdown(t *testing.T, cm *ConnManager) { t.Helper() - cm.connMtx.Lock() - defer cm.connMtx.Unlock() + func() { + cm.connMtx.Lock() + defer cm.connMtx.Unlock() - if len(cm.active) != 0 { - t.Fatalf("active map is not empty: %d entries", len(cm.active)) - } - if len(cm.pending) != 0 { - t.Fatalf("pending map is not empty: %d entries", len(cm.pending)) - } - if len(cm.persistent) != 0 { - t.Fatalf("persistent map is not empty: %d entries", len(cm.persistent)) - } - if len(cm.connIDByAddr) != 0 { - t.Fatalf("conn ID by addr map not empty: %d entries", - len(cm.connIDByAddr)) - } - if len(cm.perHostCounts) != 0 { - t.Fatalf("per host counts map not empty: %d entries", - len(cm.perHostCounts)) - } + if len(cm.active) != 0 { + t.Fatalf("active map is not empty: %d entries", len(cm.active)) + } + if len(cm.pending) != 0 { + t.Fatalf("pending map is not empty: %d entries", len(cm.pending)) + } + if len(cm.persistent) != 0 { + t.Fatalf("persistent map is not empty: %d entries", len(cm.persistent)) + } + if len(cm.connIDByAddr) != 0 { + t.Fatalf("conn ID by addr map not empty: %d entries", + len(cm.connIDByAddr)) + } + }() + + func() { + cm.perHostCountsMtx.Lock() + defer cm.perHostCountsMtx.Unlock() + + if len(cm.perHostCounts) != 0 { + t.Fatalf("per host counts map not empty: %d entries", + len(cm.perHostCounts)) + } + }() } // TestNewConfig tests that new ConnManager config is validated as expected. @@ -2040,6 +2071,21 @@ func TestMaxConnsPerHost(t *testing.T) { assertNoConnReceivedTimeout(t, connected, noConnWaitTimeout) assertConnManagerInternalState(t, cmgr) + // Ensure persistent connections for a host that already has all available + // host slots used by non-persistent conns can still be added but their + // connection attempts fail while there are no slots available. + // + // Then remove it to avoid interfering with the following tests. + connID, err = cmgr.AddPersistent(nextSameHost()) + if err != nil { + t.Fatalf("failed to add persistent connection: %v", err) + } + assertNoConnReceived(t, connected) + assertConnManagerInternalState(t, cmgr) + if err := cmgr.Remove(connID); err != nil { + t.Fatalf("unexpected remove err: %v", err) + } + // nextSameHost2 is a convenience func to return a new address to the same // IP with a different port on every invocation. It uses a different IP // than the one used above to start with a clean per-host count. @@ -2073,13 +2119,13 @@ func TestMaxConnsPerHost(t *testing.T) { assertConnReceivedTimeout(t, connected, timeout, connID, ConnTypeManual) assertConnManagerInternalState(t, cmgr) - // Ensure persistent connections are also subject to the max per-host - // connections by adding one and confirming it is NOT established. + // Ensure persistent connection registrations are limited to the max + // per-host connections by attempting to add one and confirming it is + // rejected. _, err = cmgr.AddPersistent(nextSameHost2()) - if err != nil { - t.Fatalf("failed to add persistent connection: %v", err) + if !errors.Is(err, ErrMaxPersistentPerHost) { + t.Fatalf("did not reject > max persistent per host, err: %v", err) } - assertNoConnReceivedTimeout(t, connected, noConnWaitTimeout) assertConnManagerInternalState(t, cmgr) // Ensure clean shutdown of connection manager. From ca629fc73ea2f070c9e57e43bb6ce213a6a89332 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Sat, 23 May 2026 14:43:42 -0500 Subject: [PATCH 18/23] connmgr: Separate mock conn and addr test code. This moves the code related to mock addresses, connections, and listeners to a separate file. This helps keep it from cluttering up the main test code and also makes it easier to reuse in other packages. --- internal/connmgr/connmanager_test.go | 107 ------------------------ internal/connmgr/mockconn_test.go | 120 +++++++++++++++++++++++++++ 2 files changed, 120 insertions(+), 107 deletions(-) create mode 100644 internal/connmgr/mockconn_test.go diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index fbb707d66d..0d45802ddd 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -9,7 +9,6 @@ import ( "context" "errors" "fmt" - "io" "net" "net/netip" "reflect" @@ -58,56 +57,6 @@ func runConnMgrAsync(ctx context.Context, cmgr *ConnManager) (context.Context, c return ctx, cancel, &wg } -// mockAddr mocks a network address. -type mockAddr struct { - net, address string -} - -func (m mockAddr) Network() string { return m.net } -func (m mockAddr) String() string { return m.address } - -// mockConn mocks a network connection by implementing the net.Conn interface. -type mockConn struct { - io.Reader - io.Writer - io.Closer - - // local network, address for the connection. - lnet, laddr string - - // remote network, address for the connection. - rAddr net.Addr -} - -// LocalAddr returns the local address for the connection. -func (c mockConn) LocalAddr() net.Addr { - return &mockAddr{c.lnet, c.laddr} -} - -// RemoteAddr returns the remote address for the connection. -func (c mockConn) RemoteAddr() net.Addr { - return &mockAddr{c.rAddr.Network(), c.rAddr.String()} -} - -// Close handles closing the connection. -func (c mockConn) Close() error { - return nil -} - -func (c mockConn) SetDeadline(t time.Time) error { return nil } -func (c mockConn) SetReadDeadline(t time.Time) error { return nil } -func (c mockConn) SetWriteDeadline(t time.Time) error { return nil } - -// mockDialer mocks the net.Dial interface by returning a mock connection to -// the given address. -func mockDialer(ctx context.Context, network, addr string) (net.Conn, error) { - r, w := io.Pipe() - c := &mockConn{rAddr: &mockAddr{network, addr}} - c.Reader = r - c.Writer = w - return c, ctx.Err() -} - // newTestConnManager returns a new connection manager with the provided // configuration and some timeout tweaks so that it is suitable for use in the // tests. @@ -1517,62 +1466,6 @@ func TestConnectContext(t *testing.T) { assertConnManagerCleanShutdown(t, cmgr) } -// mockListener implements the net.Listener interface and is used to test -// code that deals with net.Listeners without having to actually make any real -// connections. -type mockListener struct { - localAddr string - provideConn chan net.Conn -} - -// Accept returns a mock connection when it receives a signal via the Connect -// function. -// -// This is part of the net.Listener interface. -func (m *mockListener) Accept() (net.Conn, error) { - for conn := range m.provideConn { - return conn, nil - } - return nil, errors.New("network connection closed") -} - -// Close closes the mock listener which will cause any blocked Accept -// operations to be unblocked and return errors. -// -// This is part of the net.Listener interface. -func (m *mockListener) Close() error { - close(m.provideConn) - return nil -} - -// Addr returns the address the mock listener was configured with. -// -// This is part of the net.Listener interface. -func (m *mockListener) Addr() net.Addr { - return &mockAddr{"tcp", m.localAddr} -} - -// Connect fakes a connection to the mock listener from the provided remote -// address. It will cause the Accept function to return a mock connection -// configured with the provided remote address and the local address for the -// mock listener. -func (m *mockListener) Connect(addr net.Addr) { - m.provideConn <- &mockConn{ - laddr: m.localAddr, - lnet: "tcp", - rAddr: addr, - } -} - -// newMockListener returns a new mock listener for the provided local address -// and port. No ports are actually opened. -func newMockListener(localAddr string) *mockListener { - return &mockListener{ - localAddr: localAddr, - provideConn: make(chan net.Conn), - } -} - // TestListeners ensures providing listeners to the connection manager along // with an accept callback works properly. func TestListeners(t *testing.T) { diff --git a/internal/connmgr/mockconn_test.go b/internal/connmgr/mockconn_test.go new file mode 100644 index 0000000000..36150e5b89 --- /dev/null +++ b/internal/connmgr/mockconn_test.go @@ -0,0 +1,120 @@ +// Copyright (c) 2016 The btcsuite developers +// Copyright (c) 2019-2026 The Decred developers +// Use of this source code is governed by an ISC +// license that can be found in the LICENSE file. + +package connmgr + +import ( + "context" + "errors" + "io" + "net" + "time" +) + +// mockAddr mocks a network address. +type mockAddr struct { + net, address string +} + +func (m mockAddr) Network() string { return m.net } +func (m mockAddr) String() string { return m.address } + +// mockConn mocks a network connection by implementing the net.Conn interface. +type mockConn struct { + io.Reader + io.Writer + io.Closer + + // local network, address for the connection. + lnet, laddr string + + // remote network, address for the connection. + rAddr net.Addr +} + +// LocalAddr returns the local address for the connection. +func (c mockConn) LocalAddr() net.Addr { + return &mockAddr{c.lnet, c.laddr} +} + +// RemoteAddr returns the remote address for the connection. +func (c mockConn) RemoteAddr() net.Addr { + return &mockAddr{c.rAddr.Network(), c.rAddr.String()} +} + +// Close handles closing the connection. +func (c mockConn) Close() error { + return nil +} + +func (c mockConn) SetDeadline(t time.Time) error { return nil } +func (c mockConn) SetReadDeadline(t time.Time) error { return nil } +func (c mockConn) SetWriteDeadline(t time.Time) error { return nil } + +// mockDialer mocks the net.Dial interface by returning a mock connection to +// the given address. +func mockDialer(ctx context.Context, network, addr string) (net.Conn, error) { + r, w := io.Pipe() + c := &mockConn{rAddr: &mockAddr{network, addr}} + c.Reader = r + c.Writer = w + return c, ctx.Err() +} + +// mockListener implements the net.Listener interface and is used to test +// code that deals with net.Listeners without having to actually make any real +// connections. +type mockListener struct { + localAddr string + provideConn chan net.Conn +} + +// Accept returns a mock connection when it receives a signal via the Connect +// function. +// +// This is part of the net.Listener interface. +func (m *mockListener) Accept() (net.Conn, error) { + for conn := range m.provideConn { + return conn, nil + } + return nil, errors.New("network connection closed") +} + +// Close closes the mock listener which will cause any blocked Accept +// operations to be unblocked and return errors. +// +// This is part of the net.Listener interface. +func (m *mockListener) Close() error { + close(m.provideConn) + return nil +} + +// Addr returns the address the mock listener was configured with. +// +// This is part of the net.Listener interface. +func (m *mockListener) Addr() net.Addr { + return &mockAddr{"tcp", m.localAddr} +} + +// Connect fakes a connection to the mock listener from the provided remote +// address. It will cause the Accept function to return a mock connection +// configured with the provided remote address and the local address for the +// mock listener. +func (m *mockListener) Connect(addr net.Addr) { + m.provideConn <- &mockConn{ + laddr: m.localAddr, + lnet: "tcp", + rAddr: addr, + } +} + +// newMockListener returns a new mock listener for the provided local address +// and port. No ports are actually opened. +func newMockListener(localAddr string) *mockListener { + return &mockListener{ + localAddr: localAddr, + provideConn: make(chan net.Conn), + } +} From fb36add390348fc4bbc4779a5b3378a5e8ee010e Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Sat, 23 May 2026 14:55:36 -0500 Subject: [PATCH 19/23] connmgr: Use more modern t.Cleanup in tests. Most of the tests in this package were written well before some of the more modern test conveniences like t.Cleanup were added. As a result, almost all of the tests repeat the code related to waiting for and asserting clean shutdown. This updates the tests so that the main method used to run the connection manager in all of the tests now registers a cleanup func via t.Cleanup to wait for clean shutdown and assert the internal state is clean as expected. This approach is much less error prone and convenient. --- internal/connmgr/connmanager_test.go | 159 ++++++++------------------- 1 file changed, 43 insertions(+), 116 deletions(-) diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index 0d45802ddd..a01992ddbd 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -43,17 +43,31 @@ func mustParseAddrPort(addr string) *addrmgr.NetAddress { addrPort.Port(), 0) } -// runConnMgrAsync invokes the Run method on the passed connection manager in a -// separate goroutine and returns a cancelable context and wait group the caller -// can use to shutdown the connection manager and wait for clean shutdown. -func runConnMgrAsync(ctx context.Context, cmgr *ConnManager) (context.Context, context.CancelFunc, *sync.WaitGroup) { +// runConnMgrAsync invokes [ConnManager.Run] on the passed connection manager in +// a separate goroutine and returns a cancelable context and wait group the +// caller can use to shutdown the connection manager and wait for clean +// shutdown. +// +// It also registers a test cleanup func that waits for shutdown and asserts the +// internal state of the connection manager is empty as expected. +func runConnMgrAsync(t *testing.T, ctx context.Context, cm *ConnManager) (context.Context, context.CancelFunc, *sync.WaitGroup) { + t.Helper() + ctx, cancel := context.WithCancel(ctx) var wg sync.WaitGroup wg.Add(1) go func() { - cmgr.Run(ctx) + cm.Run(ctx) wg.Done() }() + t.Cleanup(func() { + t.Helper() + + cancel() + wg.Wait() + assertConnManagerCleanShutdown(t, cm) + }) + return ctx, cancel, &wg } @@ -91,6 +105,11 @@ func assertInternalConnState(t *testing.T, cm *ConnManager) { // Assert the pending and active maps are mutually exclusive for both conn // IDs and addrs. + // + // Also build maps of the following data in the pending, active, and + // persistent maps: + // + // - addrs to conn IDs connIDByAddr := make(map[string]uint64) for id, info := range cm.pending { if _, ok := cm.active[id]; ok { @@ -426,7 +445,7 @@ func TestConnectMode(t *testing.T) { connected <- conn }, }) - ctx, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) addr := mustParseAddrPort("127.0.0.1:18555") go cmgr.Connect(ctx, addr) @@ -435,11 +454,6 @@ func TestConnectMode(t *testing.T) { assertConnReceived(t, connected, 0, ConnTypeManual) assertNoConnReceived(t, connected) assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestDisconnect ensures that [ConnManager.Disconnect] properly disconnects @@ -481,7 +495,7 @@ func TestDisconnect(t *testing.T) { disconnected <- conn }, }) - ctx, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) // Attempt a connection to a localhost IP. notifyDialed.Store(true) @@ -597,11 +611,6 @@ func TestDisconnect(t *testing.T) { } assertConnReceived(t, disconnected, connID, ConnTypeManual) assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestRemove ensures that [ConnManager.Remove] properly removes pending and @@ -644,7 +653,7 @@ func TestRemove(t *testing.T) { disconnected <- conn }, }) - ctx, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) // Ensure removing an ID that doesn't exist returns the expected error. if err := cmgr.Remove(^uint64(0)); !errors.Is(err, ErrNotFound) { @@ -774,11 +783,6 @@ func TestRemove(t *testing.T) { } assertConnReceived(t, disconnected, connID, ConnTypeManual) assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestTargetOutbound tests the target number of outbound connections @@ -801,7 +805,7 @@ func TestTargetOutbound(t *testing.T) { connected <- conn }, }) - _, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cmgr) // Ensure only the expected number of target outbound conns are established // and no more. @@ -810,11 +814,6 @@ func TestTargetOutbound(t *testing.T) { } assertNoConnReceived(t, connected) assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestDoubleClose ensures closing a connection multiple times is a noop after @@ -833,7 +832,7 @@ func TestDoubleClose(t *testing.T) { connected <- conn }, }) - _, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cmgr) // Wait for the connection to be established. conn := assertConnReceived(t, connected, 0, ConnTypeOutbound) @@ -855,11 +854,6 @@ func TestDoubleClose(t *testing.T) { t.Fatal("connection closed more than once") } assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestRetryPersistent tests that persistent connections are retried. @@ -878,7 +872,7 @@ func TestRetryPersistent(t *testing.T) { disconnected <- conn }, }) - _, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cmgr) addr := mustParseAddrPort("127.0.0.1:18555") connID, err := cmgr.AddPersistent(addr) @@ -905,11 +899,6 @@ func TestRetryPersistent(t *testing.T) { assertConnReceived(t, disconnected, connID, ConnTypeManual) assertRemovedPersistent(t, cmgr, addr) assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestMaxPersistent ensures [ConnManager.AddPersistent] limits the maximum @@ -929,7 +918,7 @@ func TestMaxPersistent(t *testing.T) { disconnected <- conn }, }) - _, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cmgr) var numAddrs uint32 nextAddr := func() *addrmgr.NetAddress { @@ -991,11 +980,6 @@ func TestMaxPersistent(t *testing.T) { t.Fatalf("failed to add persistent connection %v: %v", addr, err) } assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestMaxRetryDuration tests the maximum retry duration. @@ -1031,7 +1015,7 @@ func TestMaxRetryDuration(t *testing.T) { connected <- conn }, }) - _, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cmgr) connID, err := cmgr.AddPersistent(mustParseAddrPort("127.0.0.1:18555")) if err != nil { @@ -1048,11 +1032,6 @@ func TestMaxRetryDuration(t *testing.T) { const timeout = connTestReceiveTimeout + networkUpTimeout assertConnReceivedTimeout(t, connected, timeout, connID, ConnTypeManual) assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestNetworkFailure tests that the connection manager handles a network @@ -1088,7 +1067,7 @@ func TestNetworkFailure(t *testing.T) { conn.RemoteAddr()) }, }) - _, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + _, shutdown, wg := runConnMgrAsync(t, context.Background(), cmgr) // Shutdown the connection manager after the max failed attempts is reached // and an additional retry duration has passed and then wait for the @@ -1112,8 +1091,6 @@ func TestNetworkFailure(t *testing.T) { t.Fatalf("unexpected number of dials - got %v, want <= %v", gotDials, wantMaxDials) } - - assertConnManagerCleanShutdown(t, cmgr) } // TestMultipleFailedConns ensures that the connection manager remains @@ -1142,7 +1119,7 @@ func TestMultipleFailedConns(t *testing.T) { Dial: errDialer, }) cmgr.maxRetryDuration = maxRetryDuration - _, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cmgr) // Establish several connection requests to localhost IPs. for i := range targetFailed { @@ -1177,11 +1154,6 @@ func TestMultipleFailedConns(t *testing.T) { t.Fatal("timeout servicing connmgr requests") } assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestShutdownFailedConns tests that failed connections are ignored after @@ -1201,7 +1173,7 @@ func TestShutdownFailedConns(t *testing.T) { Dial: waitDialer, }) cmgr.maxRetryDuration = retryTimeout - _, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cmgr) // Add a persistent connection. addr := mustParseAddrPort("127.0.0.1:18555") @@ -1219,11 +1191,6 @@ func TestShutdownFailedConns(t *testing.T) { t.Fatal("timeout waiting for dial") } time.Sleep(connTestNonReceiveTimeout) - shutdown() - - // Ensure clean shutdown of connection manager. - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestRemovePendingConnection ensures that removing a pending outbound @@ -1244,7 +1211,7 @@ func TestRemovePendingConnection(t *testing.T) { cmgr := newTestConnManager(t, &Config{ Dial: indefiniteDialer, }) - ctx, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) // Establish a connection request to a localhost IP. addr := mustParseAddrPort("127.0.0.1:18555") @@ -1279,11 +1246,6 @@ func TestRemovePendingConnection(t *testing.T) { t.Fatalf("connection %s is still pending", addr) } assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestCancelIgnoreDelayedConnection tests that a canceled pending persistent @@ -1322,7 +1284,7 @@ func TestCancelIgnoreDelayedConnection(t *testing.T) { connected <- conn }, }) - _, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cmgr) // Establish a persistent connection to a localhost IP. addr := mustParseAddrPort("127.0.0.1:18555") @@ -1354,11 +1316,6 @@ func TestCancelIgnoreDelayedConnection(t *testing.T) { // properly elapse. assertNoConnReceivedTimeout(t, connected, 5*retryTimeout) assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestDialTimeout ensure [Config.Timeout] works as intended by creating a @@ -1385,7 +1342,7 @@ func TestDialTimeout(t *testing.T) { Dial: timeoutDialer, DialTimeout: dialTimeout, }) - ctx, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) // Establish a connection to a localhost IP. addr := mustParseAddrPort("127.0.0.1:18555") @@ -1400,11 +1357,6 @@ func TestDialTimeout(t *testing.T) { t.Fatal("timeout waiting for dial cancellation") } assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestConnectContext ensures the [ConnManager.Connect] method works as intended @@ -1423,7 +1375,7 @@ func TestConnectContext(t *testing.T) { cmgr := newTestConnManager(t, &Config{ Dial: indefiniteDialer, }) - ctx, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) // Establish a connection request to a localhost IP with a separate context // that can be canceled. @@ -1459,11 +1411,6 @@ func TestConnectContext(t *testing.T) { t.Fatal("timeout waiting for dial cancellation") } assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestListeners ensures providing listeners to the connection manager along @@ -1484,7 +1431,7 @@ func TestListeners(t *testing.T) { }, Dial: mockDialer, }) - _, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cmgr) // Fake a couple of mock connections to each of the listeners. go func() { @@ -1501,11 +1448,6 @@ func TestListeners(t *testing.T) { assertConnReceived(t, receivedConns, 0, ConnTypeInbound) } assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestRejectDuplicateConns ensures duplicate addresses are rejected. This @@ -1543,7 +1485,7 @@ func TestRejectDuplicateConns(t *testing.T) { disconnected <- conn }, }) - ctx, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) // Dial a manual connection and wait for it to become pending. addr := mustParseAddrPort("127.0.0.1:18555") @@ -1645,11 +1587,6 @@ func TestRejectDuplicateConns(t *testing.T) { err) } assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestMaxNormalConns ensures the connection manager limits the total number of @@ -1714,7 +1651,7 @@ func TestMaxNormalConns(t *testing.T) { }, }) cmgr.maxRetryDuration = cmgr.cfg.RetryDuration - ctx, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) // Wait for the expected number of target outbound conns to be established. outbounds := make([]*Conn, 0, targetOutbound) @@ -1819,11 +1756,6 @@ func TestMaxNormalConns(t *testing.T) { } assertConnReceived(t, connected, connID, ConnTypeManual) assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } // TestMaxConnsPerHost ensures the connection manager limits the total number of @@ -1887,7 +1819,7 @@ func TestMaxConnsPerHost(t *testing.T) { }, }) cmgr.maxRetryDuration = cmgr.cfg.RetryDuration - ctx, shutdown, wg := runConnMgrAsync(context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) // Wait for the maximum allowed non-whitelisted per-host automatic outbound // conns. @@ -2020,9 +1952,4 @@ func TestMaxConnsPerHost(t *testing.T) { t.Fatalf("did not reject > max persistent per host, err: %v", err) } assertConnManagerInternalState(t, cmgr) - - // Ensure clean shutdown of connection manager. - shutdown() - wg.Wait() - assertConnManagerCleanShutdown(t, cmgr) } From 2f41cecf3ea386cf4d9a1d7cef43fcfde50b0f2b Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Sat, 23 May 2026 15:02:59 -0500 Subject: [PATCH 20/23] connmgr: Consistent naming in tests. This makes the name of the connection manager instances in the test code match the naming used in the implmentation code. --- internal/connmgr/connmanager_test.go | 414 +++++++++++++-------------- 1 file changed, 207 insertions(+), 207 deletions(-) diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index a01992ddbd..c3d5de3f37 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -77,12 +77,12 @@ func runConnMgrAsync(t *testing.T, ctx context.Context, cm *ConnManager) (contex func newTestConnManager(t *testing.T, cfg *Config) *ConnManager { t.Helper() - cmgr, err := New(cfg) + cm, err := New(cfg) if err != nil { t.Fatalf("New: unexpected error: %v", err) } - cmgr.maxRetryDuration = defaultTestMaxRetryDuration - return cmgr + cm.maxRetryDuration = defaultTestMaxRetryDuration + return cm } // assertInternalConnState ensures the internal connection state of the passed @@ -302,7 +302,7 @@ func TestIsWhitelisted(t *testing.T) { for _, test := range tests { // Parse the whitelist entries for the test. - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Dial: mockDialer, Whitelists: test.prefixes, }) @@ -314,7 +314,7 @@ func TestIsWhitelisted(t *testing.T) { t.Fatalf("%q-%q: failed to parse address: %v", test.name, pmTest.addr, err) } - if got := cmgr.IsWhitelisted(addr); got != pmTest.whitelisted { + if got := cm.IsWhitelisted(addr); got != pmTest.whitelisted { t.Errorf("%q-%q: mismatched result -- got %v, want %v", test.name, pmTest.addr, got, pmTest.whitelisted) continue @@ -438,22 +438,22 @@ func TestConnectMode(t *testing.T) { t.Parallel() connected := make(chan *Conn) - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ TargetOutbound: 2, Dial: mockDialer, OnConnection: func(conn *Conn) { connected <- conn }, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) addr := mustParseAddrPort("127.0.0.1:18555") - go cmgr.Connect(ctx, addr) + go cm.Connect(ctx, addr) // Ensure that only a single connection is received. assertConnReceived(t, connected, 0, ConnTypeManual) assertNoConnReceived(t, connected) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestDisconnect ensures that [ConnManager.Disconnect] properly disconnects @@ -486,7 +486,7 @@ func TestDisconnect(t *testing.T) { } return conn, err } - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Dial: pendingDialer, OnConnection: func(conn *Conn) { connected <- conn @@ -495,14 +495,14 @@ func TestDisconnect(t *testing.T) { disconnected <- conn }, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) // Attempt a connection to a localhost IP. notifyDialed.Store(true) waitForPending.Store(true) notifyCanceled.Store(true) addr := mustParseAddrPort("127.0.0.1:18555") - go cmgr.Connect(ctx, addr) + go cm.Connect(ctx, addr) // Wait for the connection manager to attempt to dial and ensure the // connection is marked as pending while the dialer is blocked. @@ -511,15 +511,15 @@ func TestDisconnect(t *testing.T) { case <-time.After(time.Millisecond * 5): t.Fatal("timeout waiting for dial") } - assertPendingAddr(t, cmgr, addr) - assertConnManagerInternalState(t, cmgr) + assertPendingAddr(t, cm, addr) + assertConnManagerInternalState(t, cm) // Disconnect the connection attempt while it's still pending. - connID, _ := pendingAddrConnID(cmgr, addr) - if err := cmgr.Disconnect(connID); err != nil { + connID, _ := pendingAddrConnID(cm, addr) + if err := cm.Disconnect(connID); err != nil { t.Fatalf("unexpected disconnect err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Allow the dialer to proceed with the disconnected connection attempt and // then wait for the dialer to signal the context associated with the dial @@ -534,37 +534,37 @@ func TestDisconnect(t *testing.T) { case <-time.After(time.Millisecond * 5): t.Fatal("timeout waiting for cancel") } - if _, ok := pendingAddrConnID(cmgr, addr); ok { + if _, ok := pendingAddrConnID(cm, addr); ok { t.Fatalf("connection %s is still pending", addr) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Start a connection attempt and wait for it to be established. notifyDialed.Store(false) waitForPending.Store(false) notifyCanceled.Store(false) - go cmgr.Connect(ctx, addr) + go cm.Connect(ctx, addr) conn := assertConnReceived(t, connected, 0, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Disconnect the established connection and wait for the disconnect // notification to ensure it is disconnected as intended. connID = conn.ID() - if err := cmgr.Disconnect(connID); err != nil { + if err := cm.Disconnect(connID); err != nil { t.Fatalf("unexpected disconnect err: %v", err) } assertConnReceived(t, disconnected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Add a persistent connection back to the same address. notifyDialed.Store(true) waitForPending.Store(true) notifyCanceled.Store(true) - connID, err := cmgr.AddPersistent(addr) + connID, err := cm.AddPersistent(addr) if err != nil { t.Fatalf("failed to add persistent connection: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Wait for the connection manager to attempt to dial and ensure the // connection is marked as pending while the dialer is blocked. @@ -573,14 +573,14 @@ func TestDisconnect(t *testing.T) { case <-time.After(time.Millisecond * 5): t.Fatal("timeout waiting for dial") } - assertPendingAddr(t, cmgr, addr) - assertConnManagerInternalState(t, cmgr) + assertPendingAddr(t, cm, addr) + assertConnManagerInternalState(t, cm) // Disconnect the persistent connection attempt while it's still pending. - if err := cmgr.Disconnect(connID); err != nil { + if err := cm.Disconnect(connID); err != nil { t.Fatalf("unexpected disconnect err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Allow the dialer to proceed with the disconnected persistent connection // attempt and then wait for the dialer to signal the context associated @@ -602,15 +602,15 @@ func TestDisconnect(t *testing.T) { // Wait for the retry to be established. assertConnReceived(t, connected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Disconnect the established persistent connection and wait for the // disconnect notification to ensure it is disconnected as intended. - if err := cmgr.Disconnect(connID); err != nil { + if err := cm.Disconnect(connID); err != nil { t.Fatalf("unexpected disconnect err: %v", err) } assertConnReceived(t, disconnected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestRemove ensures that [ConnManager.Remove] properly removes pending and @@ -644,7 +644,7 @@ func TestRemove(t *testing.T) { } return conn, err } - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Dial: pendingDialer, OnConnection: func(conn *Conn) { connected <- conn @@ -653,10 +653,10 @@ func TestRemove(t *testing.T) { disconnected <- conn }, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) // Ensure removing an ID that doesn't exist returns the expected error. - if err := cmgr.Remove(^uint64(0)); !errors.Is(err, ErrNotFound) { + if err := cm.Remove(^uint64(0)); !errors.Is(err, ErrNotFound) { t.Fatalf("mismatched remove error: got %v, want %v", err, ErrNotFound) } @@ -665,7 +665,7 @@ func TestRemove(t *testing.T) { waitForPending.Store(true) notifyCanceled.Store(true) addr := mustParseAddrPort("127.0.0.1:18555") - go cmgr.Connect(ctx, addr) + go cm.Connect(ctx, addr) // Wait for the connection manager to attempt to dial and ensure the // connection is marked as pending while the dialer is blocked. @@ -674,12 +674,12 @@ func TestRemove(t *testing.T) { case <-time.After(time.Millisecond * 5): t.Fatal("timeout waiting for dial") } - assertPendingAddr(t, cmgr, addr) - assertConnManagerInternalState(t, cmgr) + assertPendingAddr(t, cm, addr) + assertConnManagerInternalState(t, cm) // Remove the connection attempt while it's still pending. - connID, _ := pendingAddrConnID(cmgr, addr) - if err := cmgr.Remove(connID); err != nil { + connID, _ := pendingAddrConnID(cm, addr) + if err := cm.Remove(connID); err != nil { t.Fatalf("unexpected remove err: %v", err) } @@ -696,37 +696,37 @@ func TestRemove(t *testing.T) { case <-time.After(time.Millisecond * 5): t.Fatal("timeout waiting for cancel") } - if _, ok := pendingAddrConnID(cmgr, addr); ok { + if _, ok := pendingAddrConnID(cm, addr); ok { t.Fatalf("connection %s is still pending", addr) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Start a connection attempt and wait for it to be established. notifyDialed.Store(false) waitForPending.Store(false) notifyCanceled.Store(false) - go cmgr.Connect(ctx, addr) + go cm.Connect(ctx, addr) conn := assertConnReceived(t, connected, 0, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Remove the established connection and wait for the disconnect // notification to ensure it is disconnected as intended. connID = conn.ID() - if err := cmgr.Remove(connID); err != nil { + if err := cm.Remove(connID); err != nil { t.Fatalf("unexpected disconnect err: %v", err) } assertConnReceived(t, disconnected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Add a persistent connection back to the same address. notifyDialed.Store(true) waitForPending.Store(true) notifyCanceled.Store(true) - connID, err := cmgr.AddPersistent(addr) + connID, err := cm.AddPersistent(addr) if err != nil { t.Fatalf("failed to add persistent connection: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Wait for the connection manager to attempt to dial and ensure the // connection is marked as pending while the dialer is blocked. @@ -735,13 +735,13 @@ func TestRemove(t *testing.T) { case <-time.After(time.Millisecond * 5): t.Fatal("timeout waiting for dial") } - assertPendingAddr(t, cmgr, addr) + assertPendingAddr(t, cm, addr) // Remove the persistent connection attempt while it's still pending. - if err := cmgr.Remove(connID); err != nil { + if err := cm.Remove(connID); err != nil { t.Fatalf("unexpected disconnect err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Allow the dialer to proceed with the removed persistent connection // attempt and then wait for the dialer to signal the context associated @@ -760,29 +760,29 @@ func TestRemove(t *testing.T) { case <-time.After(time.Millisecond * 5): t.Fatal("timeout waiting for cancel") } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Add a persistent connection back to the same address and wait for it to // be established. notifyDialed.Store(false) waitForPending.Store(false) notifyCanceled.Store(false) - connID, err = cmgr.AddPersistent(addr) + connID, err = cm.AddPersistent(addr) if err != nil { t.Fatalf("failed to add persistent connection: %v", err) } conn2 := assertConnReceived(t, connected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Remove the established persistent connection and wait for the disconnect // notification to ensure it is disconnected as intended. Also, ensure the // persistent connection entry is removed. connID = conn2.ID() - if err := cmgr.Remove(connID); err != nil { + if err := cm.Remove(connID); err != nil { t.Fatalf("unexpected disconnect err: %v", err) } assertConnReceived(t, disconnected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestTargetOutbound tests the target number of outbound connections @@ -794,7 +794,7 @@ func TestTargetOutbound(t *testing.T) { const targetOutbound = 10 var nextAddr atomic.Uint32 connected := make(chan *Conn) - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ TargetOutbound: targetOutbound, Dial: mockDialer, GetNewAddress: func() (*addrmgr.NetAddress, error) { @@ -805,7 +805,7 @@ func TestTargetOutbound(t *testing.T) { connected <- conn }, }) - runConnMgrAsync(t, context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cm) // Ensure only the expected number of target outbound conns are established // and no more. @@ -813,7 +813,7 @@ func TestTargetOutbound(t *testing.T) { assertConnReceived(t, connected, 0, ConnTypeOutbound) } assertNoConnReceived(t, connected) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestDoubleClose ensures closing a connection multiple times is a noop after @@ -822,7 +822,7 @@ func TestDoubleClose(t *testing.T) { t.Parallel() connected := make(chan *Conn) - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ TargetOutbound: 1, Dial: mockDialer, GetNewAddress: func() (*addrmgr.NetAddress, error) { @@ -832,11 +832,11 @@ func TestDoubleClose(t *testing.T) { connected <- conn }, }) - runConnMgrAsync(t, context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cm) // Wait for the connection to be established. conn := assertConnReceived(t, connected, 0, ConnTypeOutbound) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Override the close func to cleanly detect closes. var numClosed uint32 @@ -853,7 +853,7 @@ func TestDoubleClose(t *testing.T) { if numClosed != 1 { t.Fatal("connection closed more than once") } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestRetryPersistent tests that persistent connections are retried. @@ -862,7 +862,7 @@ func TestRetryPersistent(t *testing.T) { connected := make(chan *Conn) disconnected := make(chan *Conn) - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ RetryDuration: time.Millisecond, Dial: mockDialer, OnConnection: func(conn *Conn) { @@ -872,14 +872,14 @@ func TestRetryPersistent(t *testing.T) { disconnected <- conn }, }) - runConnMgrAsync(t, context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cm) addr := mustParseAddrPort("127.0.0.1:18555") - connID, err := cmgr.AddPersistent(addr) + connID, err := cm.AddPersistent(addr) if err != nil { t.Fatalf("failed to add persistent connection: %v", err) } - if !cmgr.IsPersistent(connID) { + if !cm.IsPersistent(connID) { t.Fatal("IsPersistent did not reported true for persistent conn") } @@ -889,16 +889,16 @@ func TestRetryPersistent(t *testing.T) { conn.Close() assertConnReceived(t, disconnected, connID, ConnTypeManual) assertConnReceived(t, connected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Remove the persistent connection, wait for it to disconnect, and ensure // it is actually removed. - if err := cmgr.Remove(connID); err != nil { + if err := cm.Remove(connID); err != nil { t.Fatalf("failed to remove persistent connection: %v", err) } assertConnReceived(t, disconnected, connID, ConnTypeManual) - assertRemovedPersistent(t, cmgr, addr) - assertConnManagerInternalState(t, cmgr) + assertRemovedPersistent(t, cm, addr) + assertConnManagerInternalState(t, cm) } // TestMaxPersistent ensures [ConnManager.AddPersistent] limits the maximum @@ -909,7 +909,7 @@ func TestMaxPersistent(t *testing.T) { connected := make(chan *Conn) disconnected := make(chan *Conn) - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Dial: mockDialer, OnConnection: func(conn *Conn) { connected <- conn @@ -918,7 +918,7 @@ func TestMaxPersistent(t *testing.T) { disconnected <- conn }, }) - runConnMgrAsync(t, context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cm) var numAddrs uint32 nextAddr := func() *addrmgr.NetAddress { @@ -932,7 +932,7 @@ func TestMaxPersistent(t *testing.T) { addrs := make([]*addrmgr.NetAddress, 0, MaxPersistent) for range MaxPersistent { addr := nextAddr() - connID, err := cmgr.AddPersistent(addr) + connID, err := cm.AddPersistent(addr) if err != nil { t.Fatalf("failed to add persistent connection %v: %v", addr, err) } @@ -941,45 +941,45 @@ func TestMaxPersistent(t *testing.T) { // Wait for the connection. assertConnReceived(t, connected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // Attempting to add more than the max allowed number of persistent conns // should be rejected. - _, err := cmgr.AddPersistent(nextAddr()) + _, err := cm.AddPersistent(nextAddr()) if !errors.Is(err, ErrMaxPersistent) { t.Fatalf("did not reject > max persistent, err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure disconnecting the persistent conn does not incorrectly decrement // the count. connID, addr := connIDs[0], addrs[0] - if err := cmgr.Disconnect(connID); err != nil { + if err := cm.Disconnect(connID); err != nil { t.Fatalf("failed to disconnect persistent conn %v: %v", addr, err) } - _, err = cmgr.AddPersistent(nextAddr()) + _, err = cm.AddPersistent(nextAddr()) if !errors.Is(err, ErrMaxPersistent) { t.Fatalf("did not reject max persistent after dc, err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Remove the first persistent connection, wait for it to disconnect, and // ensure it is actually removed. - if err := cmgr.Remove(connID); err != nil { + if err := cm.Remove(connID); err != nil { t.Fatalf("failed to remove persistent conn %v: %v", addr, err) } assertConnReceived(t, disconnected, connID, ConnTypeManual) - assertRemovedPersistent(t, cmgr, addr) - assertConnManagerInternalState(t, cmgr) + assertRemovedPersistent(t, cm, addr) + assertConnManagerInternalState(t, cm) // A new persistent conn should now be allowed. addr = nextAddr() - _, err = cmgr.AddPersistent(addr) + _, err = cm.AddPersistent(addr) if err != nil { t.Fatalf("failed to add persistent connection %v: %v", addr, err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestMaxRetryDuration tests the maximum retry duration. @@ -1007,7 +1007,7 @@ func TestMaxRetryDuration(t *testing.T) { } connected := make(chan *Conn) - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ RetryDuration: time.Millisecond, TargetOutbound: 1, Dial: timedDialer, @@ -1015,9 +1015,9 @@ func TestMaxRetryDuration(t *testing.T) { connected <- conn }, }) - runConnMgrAsync(t, context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cm) - connID, err := cmgr.AddPersistent(mustParseAddrPort("127.0.0.1:18555")) + connID, err := cm.AddPersistent(mustParseAddrPort("127.0.0.1:18555")) if err != nil { t.Fatalf("failed to add persistent connection: %v", err) } @@ -1031,7 +1031,7 @@ func TestMaxRetryDuration(t *testing.T) { }) const timeout = connTestReceiveTimeout + networkUpTimeout assertConnReceivedTimeout(t, connected, timeout, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestNetworkFailure tests that the connection manager handles a network @@ -1054,7 +1054,7 @@ func TestNetworkFailure(t *testing.T) { return nil, errors.New("network down") } var nextAddr atomic.Uint32 - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ TargetOutbound: targetOutbound, RetryDuration: retryTimeout, Dial: errDialer, @@ -1067,7 +1067,7 @@ func TestNetworkFailure(t *testing.T) { conn.RemoteAddr()) }, }) - _, shutdown, wg := runConnMgrAsync(t, context.Background(), cmgr) + _, shutdown, wg := runConnMgrAsync(t, context.Background(), cm) // Shutdown the connection manager after the max failed attempts is reached // and an additional retry duration has passed and then wait for the @@ -1114,22 +1114,22 @@ func TestMultipleFailedConns(t *testing.T) { } return nil, errors.New("network down") } - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ RetryDuration: maxRetryDuration, Dial: errDialer, }) - cmgr.maxRetryDuration = maxRetryDuration - runConnMgrAsync(t, context.Background(), cmgr) + cm.maxRetryDuration = maxRetryDuration + runConnMgrAsync(t, context.Background(), cm) // Establish several connection requests to localhost IPs. for i := range targetFailed { addr := mustParseAddrPort(fmt.Sprintf("127.0.0.%d:18555", i+1)) - _, err := cmgr.AddPersistent(addr) + _, err := cm.AddPersistent(addr) if err != nil { t.Fatalf("unexpected add err: %v", err) } } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Wait for the target number of dials and ensure they happen simultaneously // by checking it happens before the retry timeout. @@ -1138,14 +1138,14 @@ func TestMultipleFailedConns(t *testing.T) { case <-time.After(20 * time.Millisecond): t.Fatal("did not reach target number of dials before timeout") } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure that the connection manager still responds to requests while the // failed connections are still retrying. disconnected := make(chan struct{}) go func() { const badID = ^uint64(0) - cmgr.Disconnect(badID) + cm.Disconnect(badID) close(disconnected) }() select { @@ -1153,7 +1153,7 @@ func TestMultipleFailedConns(t *testing.T) { case <-time.After(20 * time.Millisecond): t.Fatal("timeout servicing connmgr requests") } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestShutdownFailedConns tests that failed connections are ignored after @@ -1168,20 +1168,20 @@ func TestShutdownFailedConns(t *testing.T) { closeOnce.Do(func() { close(dialed) }) return nil, errors.New("network down") } - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ RetryDuration: retryTimeout, Dial: waitDialer, }) - cmgr.maxRetryDuration = retryTimeout - runConnMgrAsync(t, context.Background(), cmgr) + cm.maxRetryDuration = retryTimeout + runConnMgrAsync(t, context.Background(), cm) // Add a persistent connection. addr := mustParseAddrPort("127.0.0.1:18555") - _, err := cmgr.AddPersistent(addr) + _, err := cm.AddPersistent(addr) if err != nil { t.Fatalf("unexpected error: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Shutdown the connection manager during the retry timeout after a failed // dial attempt. @@ -1208,15 +1208,15 @@ func TestRemovePendingConnection(t *testing.T) { close(canceled) return nil, errors.New("error") } - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Dial: indefiniteDialer, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) // Establish a connection request to a localhost IP. addr := mustParseAddrPort("127.0.0.1:18555") - go cmgr.Connect(ctx, addr) - assertConnManagerInternalState(t, cmgr) + go cm.Connect(ctx, addr) + assertConnManagerInternalState(t, cm) // Wait for the connection manager to attempt to dial and ensure the // connection is marked as pending while the dialer is blocked. @@ -1225,15 +1225,15 @@ func TestRemovePendingConnection(t *testing.T) { case <-time.After(time.Millisecond * 20): t.Fatal("timeout waiting for dial") } - assertPendingAddr(t, cmgr, addr) - assertConnManagerInternalState(t, cmgr) + assertPendingAddr(t, cm, addr) + assertConnManagerInternalState(t, cm) // Cancel the connection attempt while it's still pending. - connID, _ := pendingAddrConnID(cmgr, addr) - if err := cmgr.Remove(connID); err != nil { + connID, _ := pendingAddrConnID(cm, addr) + if err := cm.Remove(connID); err != nil { t.Fatalf("unexpected remove err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Wait for the dialer to signal the context associated with the dial was // canceled and ensure the internal pending state is removed. @@ -1242,10 +1242,10 @@ func TestRemovePendingConnection(t *testing.T) { case <-time.After(time.Millisecond * 20): t.Fatal("timeout waiting for cancel") } - if _, ok := pendingAddrConnID(cmgr, addr); ok { + if _, ok := pendingAddrConnID(cm, addr); ok { t.Fatalf("connection %s is still pending", addr) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestCancelIgnoreDelayedConnection tests that a canceled pending persistent @@ -1277,22 +1277,22 @@ func TestCancelIgnoreDelayedConnection(t *testing.T) { } connected := make(chan *Conn) - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Dial: failingDialer, RetryDuration: retryTimeout, OnConnection: func(conn *Conn) { connected <- conn }, }) - runConnMgrAsync(t, context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cm) // Establish a persistent connection to a localhost IP. addr := mustParseAddrPort("127.0.0.1:18555") - connID, err := cmgr.AddPersistent(addr) + connID, err := cm.AddPersistent(addr) if err != nil { t.Fatalf("unexpected error: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Wait for the retry and ensure the connection is pending. select { @@ -1300,12 +1300,12 @@ func TestCancelIgnoreDelayedConnection(t *testing.T) { case <-time.After(20 * time.Millisecond): t.Fatalf("did not get retry before timeout") } - assertPendingAddr(t, cmgr, addr) - assertConnManagerInternalState(t, cmgr) + assertPendingAddr(t, cm, addr) + assertConnManagerInternalState(t, cm) // Remove the connection and then immediately allow the next connection to // succeed. - if err := cmgr.Remove(connID); err != nil { + if err := cm.Remove(connID); err != nil { t.Fatalf("unexpected remove err: %v", err) } close(connect) @@ -1315,7 +1315,7 @@ func TestCancelIgnoreDelayedConnection(t *testing.T) { // timeout window to ensure the connection manager's backoff is allowed to // properly elapse. assertNoConnReceivedTimeout(t, connected, 5*retryTimeout) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestDialTimeout ensure [Config.Timeout] works as intended by creating a @@ -1338,16 +1338,16 @@ func TestDialTimeout(t *testing.T) { return mockDialer(ctx, network, addr) } - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Dial: timeoutDialer, DialTimeout: dialTimeout, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) // Establish a connection to a localhost IP. addr := mustParseAddrPort("127.0.0.1:18555") - go cmgr.Connect(ctx, addr) - assertConnManagerInternalState(t, cmgr) + go cm.Connect(ctx, addr) + assertConnManagerInternalState(t, cm) // Wait to receive the signal that the dialer context was cancelled, which // means the dial timeout was hit. @@ -1356,7 +1356,7 @@ func TestDialTimeout(t *testing.T) { case <-time.After(dialTimeout * 10): t.Fatal("timeout waiting for dial cancellation") } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestConnectContext ensures the [ConnManager.Connect] method works as intended @@ -1372,10 +1372,10 @@ func TestConnectContext(t *testing.T) { <-ctx.Done() return nil, ctx.Err() } - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Dial: indefiniteDialer, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) // Establish a connection request to a localhost IP with a separate context // that can be canceled. @@ -1383,7 +1383,7 @@ func TestConnectContext(t *testing.T) { connectCtx, cancelConnect := context.WithCancel(ctx) connectErr := make(chan error, 1) go func() { - _, err := cmgr.Connect(connectCtx, addr) + _, err := cm.Connect(connectCtx, addr) connectErr <- err }() @@ -1395,8 +1395,8 @@ func TestConnectContext(t *testing.T) { case <-time.After(time.Millisecond * 20): t.Fatal("timeout waiting for dial") } - assertPendingAddr(t, cmgr, addr) - assertConnManagerInternalState(t, cmgr) + assertPendingAddr(t, cm, addr) + assertConnManagerInternalState(t, cm) // Cancel the connection context, wait for the error from connect, and // ensure it is the expected error. @@ -1410,7 +1410,7 @@ func TestConnectContext(t *testing.T) { case <-time.After(10 * time.Millisecond): t.Fatal("timeout waiting for dial cancellation") } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestListeners ensures providing listeners to the connection manager along @@ -1424,14 +1424,14 @@ func TestListeners(t *testing.T) { listener1 := newMockListener("127.0.0.1:9108") listener2 := newMockListener("127.0.0.1:9208") listeners := []net.Listener{listener1, listener2} - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Listeners: listeners, OnAccept: func(conn *Conn) { receivedConns <- conn }, Dial: mockDialer, }) - runConnMgrAsync(t, context.Background(), cmgr) + runConnMgrAsync(t, context.Background(), cm) // Fake a couple of mock connections to each of the listeners. go func() { @@ -1447,7 +1447,7 @@ func TestListeners(t *testing.T) { for range expectedNumConns { assertConnReceived(t, receivedConns, 0, ConnTypeInbound) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestRejectDuplicateConns ensures duplicate addresses are rejected. This @@ -1472,7 +1472,7 @@ func TestRejectDuplicateConns(t *testing.T) { <-pending return mockDialer(ctx, network, addr) } - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Listeners: []net.Listener{listener}, OnAccept: func(conn *Conn) { inboundConns <- conn @@ -1485,108 +1485,108 @@ func TestRejectDuplicateConns(t *testing.T) { disconnected <- conn }, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) + ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) // Dial a manual connection and wait for it to become pending. addr := mustParseAddrPort("127.0.0.1:18555") - go cmgr.Connect(ctx, addr) + go cm.Connect(ctx, addr) select { case <-dialed: case <-time.After(time.Millisecond * 5): t.Fatal("did not receive pending dial before timeout") } - assertPendingAddr(t, cmgr, addr) - assertConnManagerInternalState(t, cmgr) + assertPendingAddr(t, cm, addr) + assertConnManagerInternalState(t, cm) // Duplicate connect to the pending address should be rejected. - if _, err := cmgr.Connect(ctx, addr); !errors.Is(err, ErrAlreadyPending) { + if _, err := cm.Connect(ctx, addr); !errors.Is(err, ErrAlreadyPending) { t.Fatalf("did not reject duplicate pending connection, err: %v", err) } // Inbound attempts from the pending outbound address should be rejected. go listener.Connect(addr) assertNoConnReceived(t, inboundConns) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Allow the pending connection to complete. close(pending) conn := assertConnReceived(t, connected, 0, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Duplicate connect to the established address should be rejected. - if _, err := cmgr.Connect(ctx, addr); !errors.Is(err, ErrAlreadyConnected) { + if _, err := cm.Connect(ctx, addr); !errors.Is(err, ErrAlreadyConnected) { t.Fatalf("did not reject duplicate active connection, err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Inbound attempts from the established outbound address should be // rejected. go listener.Connect(addr) assertNoConnReceived(t, inboundConns) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Close the connection and wait for the disconnect. conn.Close() assertConnReceived(t, disconnected, conn.ID(), ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Add a persistent connection back to the same address and wait for it to // connect since there are no longer any connections to the address. - connID, err := cmgr.AddPersistent(addr) + connID, err := cm.AddPersistent(addr) if err != nil { t.Fatalf("failed to add persistent connection: %v", err) } assertConnReceived(t, connected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Duplicate persistent connection attempts should be rejected. - _, err = cmgr.AddPersistent(addr) + _, err = cm.AddPersistent(addr) if !errors.Is(err, ErrDuplicatePersistent) { t.Fatalf("did not reject duplicate persistent connection, err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Manual connection attempts to persistent connection should be rejected. - _, err = cmgr.Connect(ctx, addr) + _, err = cm.Connect(ctx, addr) if !errors.Is(err, ErrDuplicatePersistent) { t.Fatalf("did not reject manual connection to persistent, err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Inbound atempts from the persistent address should be rejected. go listener.Connect(addr) assertNoConnReceived(t, inboundConns) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Remove the persistent connection, wait for it to disconnect, and ensure // it is actually removed. - if err := cmgr.Remove(connID); err != nil { + if err := cm.Remove(connID); err != nil { t.Fatalf("failed to remove persistent connection: %v", err) } assertConnReceived(t, disconnected, connID, ConnTypeManual) - assertRemovedPersistent(t, cmgr, addr) - assertConnManagerInternalState(t, cmgr) + assertRemovedPersistent(t, cm, addr) + assertConnManagerInternalState(t, cm) // Inbound connections from the same address should now succeed. go listener.Connect(addr) assertConnReceived(t, inboundConns, 0, ConnTypeInbound) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Manual connection attempts to the inbound address should be rejected. - if _, err := cmgr.Connect(ctx, addr); !errors.Is(err, ErrAlreadyConnected) { + if _, err := cm.Connect(ctx, addr); !errors.Is(err, ErrAlreadyConnected) { t.Fatalf("did not reject outbound for existing inbound conn, err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Attempts to add a persistent connection to an existing inbound should be // rejected. - _, err = cmgr.AddPersistent(addr) + _, err = cm.AddPersistent(addr) if !errors.Is(err, ErrAlreadyConnected) { t.Fatalf("did not reject persistent conn for existing inbound conn: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestMaxNormalConns ensures the connection manager limits the total number of @@ -1623,7 +1623,7 @@ func TestMaxNormalConns(t *testing.T) { var pauseTargetOutbound atomic.Bool var totalPausedAddrs atomic.Uint32 hitMaxFailedAttempts := make(chan struct{}) - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Listeners: []net.Listener{listener}, MaxNormalConns: maxNormalConns, TargetOutbound: targetOutbound, @@ -1650,8 +1650,8 @@ func TestMaxNormalConns(t *testing.T) { disconnected <- conn }, }) - cmgr.maxRetryDuration = cmgr.cfg.RetryDuration - ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) + cm.maxRetryDuration = cm.cfg.RetryDuration + ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) // Wait for the expected number of target outbound conns to be established. outbounds := make([]*Conn, 0, targetOutbound) @@ -1659,7 +1659,7 @@ func TestMaxNormalConns(t *testing.T) { conn := assertConnReceived(t, connected, 0, ConnTypeOutbound) outbounds = append(outbounds, conn) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Establish target number of inbounds to the listener and wait for them to // be established. @@ -1673,13 +1673,13 @@ func TestMaxNormalConns(t *testing.T) { conn := assertConnReceived(t, inboundConns, 0, ConnTypeInbound) inbounds = append(inbounds, conn) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Establish target number of manual connections and wait for them to be // established. go func() { for range targetManual { - go cmgr.Connect(ctx, nextAddr()) + go cm.Connect(ctx, nextAddr()) } }() manualConns := make([]*Conn, 0, targetManual+1) @@ -1687,27 +1687,27 @@ func TestMaxNormalConns(t *testing.T) { conn := assertConnReceived(t, connected, 0, ConnTypeManual) manualConns = append(manualConns, conn) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure manual connections that would exceed the max allowed normal // connections are rejected. - _, err := cmgr.Connect(ctx, nextAddr()) + _, err := cm.Connect(ctx, nextAddr()) if !errors.Is(err, ErrMaxNormalConns) { t.Fatalf("did not reject manual connection at max allowed, err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure inbound connections that would exceed the max allowed normal // connections are rejected. go listener.Connect(nextAddr()) assertNoConnReceived(t, inboundConns) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure inbound connections from whitelisted addresses are exempt from the // max allowed normal connections by establishing one while at the limit. go listener.Connect(whitelistedAddr) conn := assertConnReceived(t, inboundConns, 0, ConnTypeInbound) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure closing the whitelisted connection does not release a permit it // never acquired by asserting inbound connections that would exceed the max @@ -1716,7 +1716,7 @@ func TestMaxNormalConns(t *testing.T) { assertConnReceived(t, disconnected, conn.ID(), ConnTypeInbound) go listener.Connect(nextAddr()) assertNoConnReceived(t, inboundConns) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Pause the target outbound dials and remove one of the target outbound // connections to make room for another manual connection. Then wait for @@ -1731,31 +1731,31 @@ func TestMaxNormalConns(t *testing.T) { case <-time.After(maxFailedAttempts * connTestReceiveTimeout): t.Fatal("did not reach max failed attempts before timeout") } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Establish another manual connection to take the place of the target // outbound connection that was just closed and wait for it to be // established. - go cmgr.Connect(ctx, nextAddr()) + go cm.Connect(ctx, nextAddr()) assertConnReceived(t, connected, 0, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Unpause the target outbound dials and ensure no additional automatic // outbound connections are made despite being under the target outbound due // to max total conns. pauseTargetOutbound.Store(false) assertNoConnReceivedTimeout(t, connected, connTestNonReceiveTimeout+ - cmgr.cfg.RetryDuration) - assertConnManagerInternalState(t, cmgr) + cm.cfg.RetryDuration) + assertConnManagerInternalState(t, cm) // Ensure persistent connections are not subject to the max total normal // connections by adding one and waiting for it to be established. - connID, err := cmgr.AddPersistent(nextAddr()) + connID, err := cm.AddPersistent(nextAddr()) if err != nil { t.Fatalf("failed to add persistent connection: %v", err) } assertConnReceived(t, connected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } // TestMaxConnsPerHost ensures the connection manager limits the total number of @@ -1790,7 +1790,7 @@ func TestMaxConnsPerHost(t *testing.T) { var pauseTargetOutbound atomic.Bool var totalPausedAddrs atomic.Uint32 hitMaxFailedAttempts := make(chan struct{}) - cmgr := newTestConnManager(t, &Config{ + cm := newTestConnManager(t, &Config{ Listeners: []net.Listener{listener}, MaxNormalConns: 30, // High enough to not interfere with per-host tests. MaxConnsPerHost: maxConnsPerHost, @@ -1818,8 +1818,8 @@ func TestMaxConnsPerHost(t *testing.T) { disconnected <- conn }, }) - cmgr.maxRetryDuration = cmgr.cfg.RetryDuration - ctx, _, _ := runConnMgrAsync(t, context.Background(), cmgr) + cm.maxRetryDuration = cm.cfg.RetryDuration + ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) // Wait for the maximum allowed non-whitelisted per-host automatic outbound // conns. @@ -1828,27 +1828,27 @@ func TestMaxConnsPerHost(t *testing.T) { conn := assertConnReceived(t, connected, 0, ConnTypeOutbound) outboundConns = append(outboundConns, conn) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure non-whitelisted manual connections that would exceed the max // allowed per-host connections are rejected. - _, err := cmgr.Connect(ctx, nextSameHost()) + _, err := cm.Connect(ctx, nextSameHost()) if !errors.Is(err, ErrMaxConnsPerHost) { t.Fatalf("did not reject manual connection at per-host limit, err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure non-whitelisted inbound connections that would exceed the max // allowed per-host connections are rejected. go listener.Connect(nextSameHost()) assertNoConnReceived(t, inboundConns) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure whitelisted manual connections are allowed to exceed the per-host // limit. for range maxConnsPerHost + 1 { - go cmgr.Connect(ctx, nextSameWhitelistedHost()) + go cm.Connect(ctx, nextSameWhitelistedHost()) assertConnReceived(t, connected, 0, ConnTypeManual) } @@ -1856,16 +1856,16 @@ func TestMaxConnsPerHost(t *testing.T) { // limit. go listener.Connect(nextSameWhitelistedHost()) assertConnReceived(t, inboundConns, 0, ConnTypeInbound) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure whitelisted persistent connections are allowed to exceed the // per-host limit. - connID, err := cmgr.AddPersistent(nextSameWhitelistedHost()) + connID, err := cm.AddPersistent(nextSameWhitelistedHost()) if err != nil { t.Fatalf("failed to add persistent connection: %v", err) } assertConnReceived(t, connected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Pause the target outbound dials and remove one of the target outbound // connections to make room for another manual connection with the same @@ -1884,30 +1884,30 @@ func TestMaxConnsPerHost(t *testing.T) { // Ensure a new non-whitelisted manual connection to the same host now // succeeds. - go cmgr.Connect(ctx, nextSameHost()) + go cm.Connect(ctx, nextSameHost()) assertConnReceived(t, connected, 0, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Unpause the target outbound dials and ensure no additional automatic // outbound connections to the same host are made despite being under the // target outbound. - noConnWaitTimeout := connTestReceiveTimeout + cmgr.cfg.RetryDuration + noConnWaitTimeout := connTestReceiveTimeout + cm.cfg.RetryDuration pauseTargetOutbound.Store(false) assertNoConnReceivedTimeout(t, connected, noConnWaitTimeout) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure persistent connections for a host that already has all available // host slots used by non-persistent conns can still be added but their // connection attempts fail while there are no slots available. // // Then remove it to avoid interfering with the following tests. - connID, err = cmgr.AddPersistent(nextSameHost()) + connID, err = cm.AddPersistent(nextSameHost()) if err != nil { t.Fatalf("failed to add persistent connection: %v", err) } assertNoConnReceived(t, connected) - assertConnManagerInternalState(t, cmgr) - if err := cmgr.Remove(connID); err != nil { + assertConnManagerInternalState(t, cm) + if err := cm.Remove(connID); err != nil { t.Fatalf("unexpected remove err: %v", err) } @@ -1923,14 +1923,14 @@ func TestMaxConnsPerHost(t *testing.T) { // Ensure persistent connections can achieve the per-host limit. persistentConns := make([]*Conn, 0, maxConnsPerHost) for len(persistentConns) < maxConnsPerHost { - connID, err := cmgr.AddPersistent(nextSameHost2()) + connID, err := cm.AddPersistent(nextSameHost2()) if err != nil { t.Fatalf("failed to add persistent connection: %v", err) } conn := assertConnReceived(t, connected, connID, ConnTypeManual) persistentConns = append(persistentConns, conn) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure persistent connection reconnects are allowed at the per-host // limit. Wait for the disconnect first and then for the reconnect to be @@ -1939,17 +1939,17 @@ func TestMaxConnsPerHost(t *testing.T) { connID = persistentConn.ID() persistentConn.Close() assertConnReceived(t, disconnected, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) timeout := retryTimeout + connTestReceiveTimeout assertConnReceivedTimeout(t, connected, timeout, connID, ConnTypeManual) - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) // Ensure persistent connection registrations are limited to the max // per-host connections by attempting to add one and confirming it is // rejected. - _, err = cmgr.AddPersistent(nextSameHost2()) + _, err = cm.AddPersistent(nextSameHost2()) if !errors.Is(err, ErrMaxPersistentPerHost) { t.Fatalf("did not reject > max persistent per host, err: %v", err) } - assertConnManagerInternalState(t, cmgr) + assertConnManagerInternalState(t, cm) } From e8c8987c55d7eec65d8abce4ec568be5f8892265 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Sun, 7 Jun 2026 22:50:10 -0500 Subject: [PATCH 21/23] connmgr: Use more modern t.Context in tests. Most of the tests in this package were written when the connection manager was based the older async model and before t.Context was available. As a result, the main method used to run the connection manager in all of the tests takes a context that is always set to a new background context. This updates the method to remove the parameter and instead use the test context via t.Context. --- internal/connmgr/connmanager_test.go | 42 ++++++++++++++-------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index c3d5de3f37..f16227d3e2 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -50,10 +50,10 @@ func mustParseAddrPort(addr string) *addrmgr.NetAddress { // // It also registers a test cleanup func that waits for shutdown and asserts the // internal state of the connection manager is empty as expected. -func runConnMgrAsync(t *testing.T, ctx context.Context, cm *ConnManager) (context.Context, context.CancelFunc, *sync.WaitGroup) { +func runConnMgrAsync(t *testing.T, cm *ConnManager) (context.Context, context.CancelFunc, *sync.WaitGroup) { t.Helper() - ctx, cancel := context.WithCancel(ctx) + ctx, cancel := context.WithCancel(t.Context()) var wg sync.WaitGroup wg.Add(1) go func() { @@ -445,7 +445,7 @@ func TestConnectMode(t *testing.T) { connected <- conn }, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) + ctx, _, _ := runConnMgrAsync(t, cm) addr := mustParseAddrPort("127.0.0.1:18555") go cm.Connect(ctx, addr) @@ -495,7 +495,7 @@ func TestDisconnect(t *testing.T) { disconnected <- conn }, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) + ctx, _, _ := runConnMgrAsync(t, cm) // Attempt a connection to a localhost IP. notifyDialed.Store(true) @@ -653,7 +653,7 @@ func TestRemove(t *testing.T) { disconnected <- conn }, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) + ctx, _, _ := runConnMgrAsync(t, cm) // Ensure removing an ID that doesn't exist returns the expected error. if err := cm.Remove(^uint64(0)); !errors.Is(err, ErrNotFound) { @@ -805,7 +805,7 @@ func TestTargetOutbound(t *testing.T) { connected <- conn }, }) - runConnMgrAsync(t, context.Background(), cm) + runConnMgrAsync(t, cm) // Ensure only the expected number of target outbound conns are established // and no more. @@ -832,7 +832,7 @@ func TestDoubleClose(t *testing.T) { connected <- conn }, }) - runConnMgrAsync(t, context.Background(), cm) + runConnMgrAsync(t, cm) // Wait for the connection to be established. conn := assertConnReceived(t, connected, 0, ConnTypeOutbound) @@ -872,7 +872,7 @@ func TestRetryPersistent(t *testing.T) { disconnected <- conn }, }) - runConnMgrAsync(t, context.Background(), cm) + runConnMgrAsync(t, cm) addr := mustParseAddrPort("127.0.0.1:18555") connID, err := cm.AddPersistent(addr) @@ -918,7 +918,7 @@ func TestMaxPersistent(t *testing.T) { disconnected <- conn }, }) - runConnMgrAsync(t, context.Background(), cm) + runConnMgrAsync(t, cm) var numAddrs uint32 nextAddr := func() *addrmgr.NetAddress { @@ -1015,7 +1015,7 @@ func TestMaxRetryDuration(t *testing.T) { connected <- conn }, }) - runConnMgrAsync(t, context.Background(), cm) + runConnMgrAsync(t, cm) connID, err := cm.AddPersistent(mustParseAddrPort("127.0.0.1:18555")) if err != nil { @@ -1067,7 +1067,7 @@ func TestNetworkFailure(t *testing.T) { conn.RemoteAddr()) }, }) - _, shutdown, wg := runConnMgrAsync(t, context.Background(), cm) + _, shutdown, wg := runConnMgrAsync(t, cm) // Shutdown the connection manager after the max failed attempts is reached // and an additional retry duration has passed and then wait for the @@ -1119,7 +1119,7 @@ func TestMultipleFailedConns(t *testing.T) { Dial: errDialer, }) cm.maxRetryDuration = maxRetryDuration - runConnMgrAsync(t, context.Background(), cm) + runConnMgrAsync(t, cm) // Establish several connection requests to localhost IPs. for i := range targetFailed { @@ -1173,7 +1173,7 @@ func TestShutdownFailedConns(t *testing.T) { Dial: waitDialer, }) cm.maxRetryDuration = retryTimeout - runConnMgrAsync(t, context.Background(), cm) + runConnMgrAsync(t, cm) // Add a persistent connection. addr := mustParseAddrPort("127.0.0.1:18555") @@ -1211,7 +1211,7 @@ func TestRemovePendingConnection(t *testing.T) { cm := newTestConnManager(t, &Config{ Dial: indefiniteDialer, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) + ctx, _, _ := runConnMgrAsync(t, cm) // Establish a connection request to a localhost IP. addr := mustParseAddrPort("127.0.0.1:18555") @@ -1284,7 +1284,7 @@ func TestCancelIgnoreDelayedConnection(t *testing.T) { connected <- conn }, }) - runConnMgrAsync(t, context.Background(), cm) + runConnMgrAsync(t, cm) // Establish a persistent connection to a localhost IP. addr := mustParseAddrPort("127.0.0.1:18555") @@ -1342,7 +1342,7 @@ func TestDialTimeout(t *testing.T) { Dial: timeoutDialer, DialTimeout: dialTimeout, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) + ctx, _, _ := runConnMgrAsync(t, cm) // Establish a connection to a localhost IP. addr := mustParseAddrPort("127.0.0.1:18555") @@ -1375,7 +1375,7 @@ func TestConnectContext(t *testing.T) { cm := newTestConnManager(t, &Config{ Dial: indefiniteDialer, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) + ctx, _, _ := runConnMgrAsync(t, cm) // Establish a connection request to a localhost IP with a separate context // that can be canceled. @@ -1431,7 +1431,7 @@ func TestListeners(t *testing.T) { }, Dial: mockDialer, }) - runConnMgrAsync(t, context.Background(), cm) + runConnMgrAsync(t, cm) // Fake a couple of mock connections to each of the listeners. go func() { @@ -1485,7 +1485,7 @@ func TestRejectDuplicateConns(t *testing.T) { disconnected <- conn }, }) - ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) + ctx, _, _ := runConnMgrAsync(t, cm) // Dial a manual connection and wait for it to become pending. addr := mustParseAddrPort("127.0.0.1:18555") @@ -1651,7 +1651,7 @@ func TestMaxNormalConns(t *testing.T) { }, }) cm.maxRetryDuration = cm.cfg.RetryDuration - ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) + ctx, _, _ := runConnMgrAsync(t, cm) // Wait for the expected number of target outbound conns to be established. outbounds := make([]*Conn, 0, targetOutbound) @@ -1819,7 +1819,7 @@ func TestMaxConnsPerHost(t *testing.T) { }, }) cm.maxRetryDuration = cm.cfg.RetryDuration - ctx, _, _ := runConnMgrAsync(t, context.Background(), cm) + ctx, _, _ := runConnMgrAsync(t, cm) // Wait for the maximum allowed non-whitelisted per-host automatic outbound // conns. From 6a3fd54125003f1f90b979cab6c5d3c508ff965e Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Mon, 25 May 2026 19:24:40 -0500 Subject: [PATCH 22/23] connmgr: Only close once in double close tests. --- internal/connmgr/connmanager_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index f16227d3e2..6acd978649 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -843,7 +843,9 @@ func TestDoubleClose(t *testing.T) { origOnClose := conn.onClose conn.onClose = func() { numClosed++ - origOnClose() + if numClosed == 1 { + origOnClose() + } } // Close the connection multiple times and make sure it only happens once. From dd4c264f2448ec00bb7dc064fe857ab8f0d89625 Mon Sep 17 00:00:00 2001 From: Dave Collins Date: Mon, 25 May 2026 19:27:30 -0500 Subject: [PATCH 23/23] connmgr: Cleaner dial timeout detection test. Now that the Connect method is synchronous and returns an error, this modifies the test for detecting dial timeouts to use that error for more more accurate detection that the failure is actually the result of dial timeout as expected. --- internal/connmgr/connmanager_test.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/internal/connmgr/connmanager_test.go b/internal/connmgr/connmanager_test.go index 6acd978649..76f5c9672e 100644 --- a/internal/connmgr/connmanager_test.go +++ b/internal/connmgr/connmanager_test.go @@ -1329,12 +1329,10 @@ func TestDialTimeout(t *testing.T) { // Create a connection manager instance with a dialer that blocks for three // times the configured dial timeout before connecting. const dialTimeout = time.Millisecond * 20 - cancelled := make(chan struct{}) timeoutDialer := func(ctx context.Context, network, addr string) (net.Conn, error) { select { case <-time.After(dialTimeout * 3): case <-ctx.Done(): - close(cancelled) return nil, ctx.Err() } @@ -1346,15 +1344,21 @@ func TestDialTimeout(t *testing.T) { }) ctx, _, _ := runConnMgrAsync(t, cm) - // Establish a connection to a localhost IP. - addr := mustParseAddrPort("127.0.0.1:18555") - go cm.Connect(ctx, addr) + connectErr := make(chan error, 1) + go func() { + addr := mustParseAddrPort("127.0.0.1:18555") + _, err := cm.Connect(ctx, addr) + connectErr <- err + }() assertConnManagerInternalState(t, cm) - // Wait to receive the signal that the dialer context was cancelled, which - // means the dial timeout was hit. + // Wait for the error from connect and ensure it is the expected deadline + // exceeded (aka dial timeout) error. select { - case <-cancelled: + case err := <-connectErr: + if wantErr := context.DeadlineExceeded; !errors.Is(err, wantErr) { + t.Fatalf("unexpected connect err: got %v, want %v", err, wantErr) + } case <-time.After(dialTimeout * 10): t.Fatal("timeout waiting for dial cancellation") }