From 67d20d4af51d0705b6ae3374612dddf848dbb59f Mon Sep 17 00:00:00 2001
From: zzz <zzz@i2pmail.org>
Date: Sat, 17 Dec 2022 08:43:25 -0500
Subject: [PATCH] Transport: Change _currentAddresses from COWAL to standard
ArrayList and lock
COWAL caused problems because replaceAddress() was removing the wrong address,
since RouterAddress.equals() does not compare caps, so empty '4' and '6' addresses would be equal.
So we could temporarily end up with multiple v4/v6 addresses.
COWAL wasn't really buying us anything anyway.
Fix removeAddress() that called the listener even if no change.
---
.../i2p/router/transport/TransportImpl.java | 85 ++++++++++++-------
1 file changed, 53 insertions(+), 32 deletions(-)
diff --git a/router/java/src/net/i2p/router/transport/TransportImpl.java b/router/java/src/net/i2p/router/transport/TransportImpl.java
index 9e43a79eb3..afab743d9f 100644
--- a/router/java/src/net/i2p/router/transport/TransportImpl.java
+++ b/router/java/src/net/i2p/router/transport/TransportImpl.java
@@ -25,7 +25,6 @@ import java.util.Map;
import java.util.Set;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
-import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.ConcurrentHashMap;
import net.i2p.data.DataHelper;
@@ -55,7 +54,7 @@ import net.i2p.util.VersionComparator;
public abstract class TransportImpl implements Transport {
private final Log _log;
private TransportEventListener _listener;
- protected final List<RouterAddress> _currentAddresses;
+ private final List<RouterAddress> _currentAddresses;
// Only used by NTCP. SSU does not use. See send() below.
private final BlockingQueue<OutNetMessage> _sendPool;
protected final RouterContext _context;
@@ -112,7 +111,7 @@ public abstract class TransportImpl implements Transport {
//_context.statManager().createRateStat("transport.sendProcessingTime." + getStyle(), "Time to process and send a message (ms)", "Transport", new long[] { 60*1000l });
_context.statManager().createRateStat("transport.expiredOnQueueLifetime", "How long a message that expires on our outbound queue is processed", "Transport", new long[] { 60*1000l, 10*60*1000l, 60*60*1000l, 24*60*60*1000l } );
- _currentAddresses = new CopyOnWriteArrayList<RouterAddress>();
+ _currentAddresses = new ArrayList<RouterAddress>(3);
if (getStyle().equals("NTCP"))
_sendPool = new ArrayBlockingQueue<OutNetMessage>(8);
else
@@ -550,11 +549,13 @@ public abstract class TransportImpl implements Transport {
/**
* What addresses are we currently listening to?
* Replaces getCurrentAddress()
- * @return all addresses, non-null
+ * @return a copy of all addresses, non-null
* @since IPv6
*/
public List<RouterAddress> getCurrentAddresses() {
- return _currentAddresses;
+ synchronized(_currentAddresses) {
+ return new ArrayList<RouterAddress>(_currentAddresses);
+ }
}
/**
@@ -568,9 +569,11 @@ public abstract class TransportImpl implements Transport {
* @since IPv6
*/
public RouterAddress getCurrentAddress(boolean ipv6) {
- for (RouterAddress ra : _currentAddresses) {
- if (ipv6 == TransportUtil.isIPv6(ra))
- return ra;
+ synchronized(_currentAddresses) {
+ for (RouterAddress ra : _currentAddresses) {
+ if (ipv6 == TransportUtil.isIPv6(ra))
+ return ra;
+ }
}
return null;
}
@@ -580,17 +583,21 @@ public abstract class TransportImpl implements Transport {
* @since IPv6
*/
public boolean hasCurrentAddress() {
- return !_currentAddresses.isEmpty();
+ synchronized(_currentAddresses) {
+ return !_currentAddresses.isEmpty();
+ }
}
/**
* Ask the transport to update its address based on current information and return it
* Transports should override.
- * @return all addresses, non-null
+ * @return a copy of all addresses, non-null
* @since 0.7.12
*/
public List<RouterAddress> updateAddress() {
- return _currentAddresses;
+ synchronized(_currentAddresses) {
+ return new ArrayList<RouterAddress>(_currentAddresses);
+ }
}
/**
@@ -603,21 +610,27 @@ public abstract class TransportImpl implements Transport {
* @param address null to remove all
*/
protected void replaceAddress(RouterAddress address) {
- if (_log.shouldLog(Log.WARN))
- _log.warn("Replacing address with " + address, new Exception());
- if (address == null) {
- _currentAddresses.clear();
- } else {
- boolean isIPv6 = TransportUtil.isIPv6(address);
- for (RouterAddress ra : _currentAddresses) {
- if (isIPv6 == TransportUtil.isIPv6(ra))
- // COWAL
- _currentAddresses.remove(ra);
+ boolean isIPv6 = TransportUtil.isIPv6(address);
+ if (_log.shouldWarn())
+ _log.warn("Replacing IPv" + (isIPv6 ? '6' : '4') + " address with " + address, new Exception());
+ int sz;
+ synchronized(_currentAddresses) {
+ if (address == null) {
+ _currentAddresses.clear();
+ sz = 0;
+ } else {
+ for (Iterator<RouterAddress> iter = _currentAddresses.iterator(); iter.hasNext(); ) {
+ RouterAddress ra = iter.next();
+ if (isIPv6 == TransportUtil.isIPv6(ra)) {
+ iter.remove();
+ }
+ }
+ _currentAddresses.add(address);
+ sz = _currentAddresses.size();
}
- _currentAddresses.add(address);
}
- if (_log.shouldLog(Log.WARN))
- _log.warn(getStyle() + " now has " + _currentAddresses.size() + " addresses");
+ if (_log.shouldWarn())
+ _log.warn(getStyle() + " now has " + sz + " addresses");
if (_listener != null)
_listener.transportAddressChanged();
}
@@ -633,11 +646,15 @@ public abstract class TransportImpl implements Transport {
protected void removeAddress(RouterAddress address) {
if (_log.shouldWarn())
_log.warn("Removing address " + address, new Exception());
- boolean changed = _currentAddresses.remove(address);
- changed = true;
+ boolean changed;
+ int sz;
+ synchronized(_currentAddresses) {
+ changed = _currentAddresses.remove(address);
+ sz = _currentAddresses.size();
+ }
if (changed) {
if (_log.shouldWarn())
- _log.warn(getStyle() + " now has " + _currentAddresses.size() + " addresses");
+ _log.warn(getStyle() + " now has " + sz + " addresses");
if (_listener != null)
_listener.transportAddressChanged();
} else {
@@ -658,16 +675,20 @@ public abstract class TransportImpl implements Transport {
if (_log.shouldWarn())
_log.warn("Removing addresses, ipv6? " + ipv6, new Exception());
boolean changed = false;
- for (RouterAddress ra : _currentAddresses) {
- if (ipv6 == TransportUtil.isIPv6(ra)) {
- // COWAL
- if (_currentAddresses.remove(ra))
+ int sz;
+ synchronized(_currentAddresses) {
+ for (Iterator<RouterAddress> iter = _currentAddresses.iterator(); iter.hasNext(); ) {
+ RouterAddress ra = iter.next();
+ if (ipv6 == TransportUtil.isIPv6(ra)) {
+ iter.remove();
changed = true;
+ }
}
+ sz = _currentAddresses.size();
}
if (changed) {
if (_log.shouldWarn())
- _log.warn(getStyle() + " now has " + _currentAddresses.size() + " addresses");
+ _log.warn(getStyle() + " now has " + sz + " addresses");
if (_listener != null)
_listener.transportAddressChanged();
} else {
--
GitLab