This is an automated email from the ASF dual-hosted git repository.
amichair pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/aries-rsa.git
The following commit(s) were added to refs/heads/master by this push:
new 31e2f603 ARIES-2239 Fix race condition when TcpConnectionManager
resolves concurrent connections
31e2f603 is described below
commit 31e2f603bb6da48798c3d1edb573e6455cdc7312
Author: Amichai Rothman <[email protected]>
AuthorDate: Wed Jun 17 13:11:13 2026 +0300
ARIES-2239 Fix race condition when TcpConnectionManager resolves concurrent
connections
---
.../rsa/discovery/tcp/TcpConnectionManager.java | 62 +++++++++++++++++-----
1 file changed, 48 insertions(+), 14 deletions(-)
diff --git
a/discovery/tcp/src/main/java/org/apache/aries/rsa/discovery/tcp/TcpConnectionManager.java
b/discovery/tcp/src/main/java/org/apache/aries/rsa/discovery/tcp/TcpConnectionManager.java
index 4962a16c..399e2ed3 100644
---
a/discovery/tcp/src/main/java/org/apache/aries/rsa/discovery/tcp/TcpConnectionManager.java
+++
b/discovery/tcp/src/main/java/org/apache/aries/rsa/discovery/tcp/TcpConnectionManager.java
@@ -189,9 +189,10 @@ public class TcpConnectionManager implements
EndpointEventListener {
LOG.debug("received message {} on connection {}", message, conn);
if (message instanceof HandshakeMessage) {
HandshakeMessage h = (HandshakeMessage) message;
+ String peerUuid = h.getUuid();
// update the peer data
conn.setPeerAddress(h.getAddress());
- conn.setPeerUuid(h.getUuid());
+ conn.setPeerUuid(peerUuid);
// if gossip is enabled, try adding all of this peer's peers,
// as well as the peer itself (in case it found us via its
// own gossip, and we haven't met before)
@@ -200,21 +201,54 @@ public class TcpConnectionManager implements
EndpointEventListener {
addPeers(Collections.singleton(h.getAddress()));
}
// if we already have another connection with this peer (e.g.
reverse direction)
- // then we keep the old one (which is already in use) and close
the new one
- boolean existing = connectionsByUuid.putIfAbsent(h.getUuid(),
conn) != null;
+ // then we use the outbound connection from the peer with the
higher uuid -
+ // this way both peers agree on which connection to use in case of
a race.
+ // Once the decision is made about which connection to keep and
which to close,
+ // there is still the question of which side performs the closing:
+ // there is a possible race condition where one peer receives the
handshake
+ // and closes the connection before it even had a chance to send
its handshake -
+ // so the other peer will never know that the connection is
intentionally closed
+ // and not simply experiencing connection errors. If it is the
outbound side, it
+ // will keep retrying to connect. to solve this, only the outbound
originator is the
+ // one that closes the connection. The inbound side just avoids
using it until then.
+ boolean existing = connectionsByUuid.putIfAbsent(peerUuid, conn)
!= null;
if (existing) {
- // both sides can check if a connection between them already
exists, but
- // there is a possible race condition where one peer receives
the handshake
- // and closes the connection before it even had a chance to
send its handshake -
- // so the other peer will never know the connection is
intentionally closed
- // and not experiencing connection errors. If it is the
outbound side, it will
- // keep retrying to connect. to solve this, only the outbound
peer is the one
- // that closes the connection. The inbound side just doesn't
use it until then.
- if (conn.isOutbound()) {
- LOG.debug("connection already exists with peer {}, closing
{}", h.getUuid(), conn);
- conn.close();
+ boolean winner = localUuid.compareTo(peerUuid) > 0; // we have
higher uuid
+ TcpConnection prev = null;
+ if (winner) {
+ if (conn.isOutbound()) {
+ // new connection is our winner outbound -
+ // use it instead of existing (and loser peer will
close their outbound)
+ prev = connectionsByUuid.put(peerUuid, conn);
+ LOG.debug("connection already exists with peer {},
switching from {} to {}",
+ peerUuid, prev, conn);
+ } else {
+ // new connection is the loser's outbound (our
inbound) -
+ // leave the existing winner outbound as-is
+ LOG.debug("connection already exists with peer {},
ignoring {}",
+ peerUuid, conn);
+ }
+ } else {
+ if (conn.isOutbound()) {
+ // new connection is our loser outbound -
+ // close it and leave the existing winner outbound
as-is
+ LOG.debug("connection already exists with peer {},
ignoring {} and closing it",
+ peerUuid, conn);
+ conn.close();
+ } else {
+ // new connection is the winner's outbound (our
inbound) -
+ // use it instead of existing and close our outbound
+ prev = connectionsByUuid.put(peerUuid, conn);
+ LOG.debug("connection already exists with peer {},
switching from {} to {} and closing",
+ peerUuid, prev, conn);
+ prev.close();
+ }
+ }
+ // if we didn't swap connections, we already sent the known
endpoints
+ // on the existing connection so no need to send them again
+ if (prev == null) {
+ return;
}
- return;
}
// send all of our known local endpoints to the new peer
localEndpointManager.getEndpoints().forEach(
