diff --git a/addrmgr/addrmanager.go b/addrmgr/addrmanager.go index fa8f27bcae..1cfbbc5ca4 100644 --- a/addrmgr/addrmanager.go +++ b/addrmgr/addrmanager.go @@ -176,7 +176,7 @@ func (a *AddrManager) updateAddress(netAddr, srcAddr *wire.NetAddress) { // TODO: only update addresses periodically. // Update the last seen time and services. // note that to prevent causing excess garbage on getaddr - // messages the netaddresses in addrmaanger are *immutable*, + // messages the netaddresses in addrmanager are *immutable*, // if we need to change them then we replace the pointer with a // new copy so that we don't have to copy every na for getaddr. if netAddr.Timestamp.After(ka.na.Timestamp) || @@ -186,7 +186,7 @@ func (a *AddrManager) updateAddress(netAddr, srcAddr *wire.NetAddress) { naCopy := *ka.na naCopy.Timestamp = netAddr.Timestamp naCopy.AddService(netAddr.Services) - ka.na = &naCopy + ka.na = &naCopy // warning: na field of KnownAddress is mutable } // If already in tried, we have nothing to do here. @@ -773,11 +773,50 @@ func NetAddressKey(na *wire.NetAddress) string { // random one from the possible addresses with preference given to ones that // have not been used recently and should not pick 'close' addresses // consecutively. +// +// NOTE: The KnownAddress returned by GetAddress is not safely usable while the +// AddrManager is in use by another goroutine since the fields of KnownAddress +// are mutable. Is this method practical outside of tests that use a toy +// AddrManager? func (a *AddrManager) GetAddress() *KnownAddress { - // Protect concurrent access. a.mtx.Lock() defer a.mtx.Unlock() + return a.getAddress() +} + +// GetAddressCopy is like GetAddress, except that this returns a copy of a +// stored KnownAddress, so is safe for concurrent access. +func (a *AddrManager) GetAddressCopy() *KnownAddress { + a.mtx.Lock() + defer a.mtx.Unlock() + ka := a.getAddress() + if ka == nil { + return nil + } + // While the NetAddress in ka.na is immutable, the na field of KnownAddress + // is not, so we access it under the address manager's lock. See + // updateAddress, SetServices, Connected, and Good, where various + // KnownAddress fields are modified that are also accessible by KnownAddress + // methods without a synchronization mechanism. + kaCopy := *ka + return &kaCopy +} + +// GetNetAddress returns a single address that should be routable and its last +// connection attempt time. It picks a random one from the possible addresses +// with preference given to ones that have not been used recently and should not +// pick 'close' addresses consecutively. +func (a *AddrManager) GetNetAddress() (*wire.NetAddress, time.Time) { + a.mtx.Lock() + defer a.mtx.Unlock() + ka := a.getAddress() + if ka == nil { + return nil, time.Time{} + } + return ka.na, ka.lastattempt +} +func (a *AddrManager) getAddress() *KnownAddress { if a.numAddresses() == 0 { return nil } diff --git a/server.go b/server.go index 746c48dde3..a2bb9ea38b 100644 --- a/server.go +++ b/server.go @@ -2843,8 +2843,8 @@ func newServer(listenAddrs, agentBlacklist, agentWhitelist []string, if !cfg.SimNet && len(cfg.ConnectPeers) == 0 { newAddressFunc = func() (net.Addr, error) { for tries := 0; tries < 100; tries++ { - addr := s.addrManager.GetAddress() - if addr == nil { + netAddr, lastAttempt := s.addrManager.GetNetAddress() + if netAddr == nil { break } @@ -2854,27 +2854,27 @@ func newServer(listenAddrs, agentBlacklist, agentWhitelist []string, // in the same group so that we are not connecting // to the same network segment at the expense of // others. - key := addrmgr.GroupKey(addr.NetAddress()) + key := addrmgr.GroupKey(netAddr) if s.OutboundGroupCount(key) != 0 { continue } // only allow recent nodes (10mins) after we failed 30 // times - if tries < 30 && time.Since(addr.LastAttempt()) < 10*time.Minute { + if tries < 30 && time.Since(lastAttempt) < 10*time.Minute { continue } // allow nondefault ports after 50 failed tries. - if tries < 50 && fmt.Sprintf("%d", addr.NetAddress().Port) != + if tries < 50 && fmt.Sprintf("%d", netAddr.Port) != activeNetParams.DefaultPort { continue } // Mark an attempt for the valid address. - s.addrManager.Attempt(addr.NetAddress()) + s.addrManager.Attempt(netAddr) - addrString := addrmgr.NetAddressKey(addr.NetAddress()) + addrString := addrmgr.NetAddressKey(netAddr) return addrStringToNetAddr(addrString) }