Skip to content

Commit cb21f17

Browse files
altonenaltonen
authored
transport: Fix erroneous handling of secondary connections (#149)
Previously `TransportManager` didn't correctly handle the case where local node's dialing connection was opened as a secondary connection, leaving `dial_record` as `Some` which didn't get cleaned up after the connections were closed. This resulted in subsequent dials to fail because `dial_record` was `Some`, which erroneously indicated that the remote was already being dialed, even though the dial had concluded and the connection was closed
1 parent 70819df commit cb21f17

File tree

2 files changed

+212
-33
lines changed

2 files changed

+212
-33
lines changed

Diff for: ‎src/transport/manager/mod.rs

+64-33
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,10 @@ pub(crate) mod handle;
7070
const LOG_TARGET: &str = "litep2p::transport-manager";
7171

7272
/// Score for a working address.
73-
const SCORE_DIAL_SUCCESS: i32 = 100i32;
73+
const SCORE_CONNECT_SUCCESS: i32 = 100i32;
7474

7575
/// Score for a non-working address.
76-
const SCORE_DIAL_FAILURE: i32 = -100i32;
76+
const SCORE_CONNECT_FAILURE: i32 = -100i32;
7777

7878
/// TODO:
7979
enum ConnectionEstablishedResult {
@@ -698,7 +698,7 @@ impl TransportManager {
698698
PeerState::Dialing { ref mut record } => {
699699
debug_assert_eq!(record.connection_id(), &Some(connection_id));
700700

701-
record.update_score(SCORE_DIAL_FAILURE);
701+
record.update_score(SCORE_CONNECT_FAILURE);
702702
context.addresses.insert(record.clone());
703703

704704
context.state = PeerState::Disconnected { dial_record: None };
@@ -711,7 +711,7 @@ impl TransportManager {
711711
record,
712712
dial_record: Some(mut dial_record),
713713
} => {
714-
dial_record.update_score(SCORE_DIAL_FAILURE);
714+
dial_record.update_score(SCORE_CONNECT_FAILURE);
715715
context.addresses.insert(dial_record);
716716

717717
context.state = PeerState::Connected {
@@ -730,7 +730,7 @@ impl TransportManager {
730730
"dial failed for a disconnected peer",
731731
);
732732

733-
dial_record.update_score(SCORE_DIAL_FAILURE);
733+
dial_record.update_score(SCORE_CONNECT_FAILURE);
734734
context.addresses.insert(dial_record);
735735

736736
Ok(())
@@ -914,7 +914,10 @@ impl TransportManager {
914914
let mut peers = self.peers.write();
915915
match peers.get_mut(&peer) {
916916
Some(context) => match context.state {
917-
PeerState::Connected { .. } => match context.secondary_connection {
917+
PeerState::Connected {
918+
ref mut dial_record,
919+
..
920+
} => match context.secondary_connection {
918921
Some(_) => {
919922
tracing::debug!(
920923
target: LOG_TARGET,
@@ -932,29 +935,57 @@ impl TransportManager {
932935
context.addresses.insert(AddressRecord::new(
933936
&peer,
934937
endpoint.address().clone(),
935-
SCORE_DIAL_SUCCESS,
938+
SCORE_CONNECT_SUCCESS,
936939
None,
937940
))
938941
}
939942

940943
return Ok(ConnectionEstablishedResult::Reject);
941944
}
942-
None => {
943-
tracing::debug!(
945+
None => match dial_record.take() {
946+
Some(record)
947+
if record.connection_id() == &Some(endpoint.connection_id()) =>
948+
{
949+
tracing::debug!(
950+
target: LOG_TARGET,
951+
?peer,
952+
connection_id = ?endpoint.connection_id(),
953+
address = ?endpoint.address(),
954+
"dialed connection opened as secondary connection",
955+
);
956+
957+
context.secondary_connection = Some(AddressRecord::new(
958+
&peer,
959+
endpoint.address().clone(),
960+
SCORE_CONNECT_SUCCESS,
961+
Some(endpoint.connection_id()),
962+
));
963+
}
964+
None => {
965+
tracing::debug!(
966+
target: LOG_TARGET,
967+
?peer,
968+
connection_id = ?endpoint.connection_id(),
969+
address = ?endpoint.address(),
970+
"secondary connection",
971+
);
972+
973+
context.secondary_connection = Some(AddressRecord::new(
974+
&peer,
975+
endpoint.address().clone(),
976+
SCORE_CONNECT_SUCCESS,
977+
Some(endpoint.connection_id()),
978+
));
979+
}
980+
Some(record) => tracing::warn!(
944981
target: LOG_TARGET,
945982
?peer,
946983
connection_id = ?endpoint.connection_id(),
947984
address = ?endpoint.address(),
948-
"secondary connection",
949-
);
950-
951-
context.secondary_connection = Some(AddressRecord::new(
952-
&peer,
953-
endpoint.address().clone(),
954-
SCORE_DIAL_SUCCESS,
955-
Some(endpoint.connection_id()),
956-
));
957-
}
985+
dial_record = ?record,
986+
"unknown connection opened as secondary connection, discarding",
987+
),
988+
},
958989
},
959990
PeerState::Dialing { ref record, .. } => {
960991
match record.connection_id() == &Some(endpoint.connection_id()) {
@@ -986,7 +1017,7 @@ impl TransportManager {
9861017
record: AddressRecord::new(
9871018
&peer,
9881019
endpoint.address().clone(),
989-
SCORE_DIAL_SUCCESS,
1020+
SCORE_CONNECT_SUCCESS,
9901021
Some(endpoint.connection_id()),
9911022
),
9921023
dial_record: Some(record.clone()),
@@ -1036,14 +1067,14 @@ impl TransportManager {
10361067

10371068
let record = match records.remove(endpoint.address()) {
10381069
Some(mut record) => {
1039-
record.update_score(SCORE_DIAL_SUCCESS);
1070+
record.update_score(SCORE_CONNECT_SUCCESS);
10401071
record.set_connection_id(endpoint.connection_id());
10411072
record
10421073
}
10431074
None => AddressRecord::new(
10441075
&peer,
10451076
endpoint.address().clone(),
1046-
SCORE_DIAL_SUCCESS,
1077+
SCORE_CONNECT_SUCCESS,
10471078
Some(endpoint.connection_id()),
10481079
),
10491080
};
@@ -1076,7 +1107,7 @@ impl TransportManager {
10761107
AddressRecord::new(
10771108
&peer,
10781109
endpoint.address().clone(),
1079-
SCORE_DIAL_SUCCESS,
1110+
SCORE_CONNECT_SUCCESS,
10801111
Some(endpoint.connection_id()),
10811112
),
10821113
Some(dial_record),
@@ -1086,7 +1117,7 @@ impl TransportManager {
10861117
AddressRecord::new(
10871118
&peer,
10881119
endpoint.address().clone(),
1089-
SCORE_DIAL_SUCCESS,
1120+
SCORE_CONNECT_SUCCESS,
10901121
Some(endpoint.connection_id()),
10911122
),
10921123
None,
@@ -1107,7 +1138,7 @@ impl TransportManager {
11071138
record: AddressRecord::new(
11081139
&peer,
11091140
endpoint.address().clone(),
1110-
SCORE_DIAL_SUCCESS,
1141+
SCORE_CONNECT_SUCCESS,
11111142
Some(endpoint.connection_id()),
11121143
),
11131144
dial_record: None,
@@ -1186,7 +1217,7 @@ impl TransportManager {
11861217
// all other address records back to `AddressStore`. and ask
11871218
// transport to negotiate the
11881219
let mut dial_record = records.remove(&address).expect("address to exist");
1189-
dial_record.update_score(SCORE_DIAL_SUCCESS);
1220+
dial_record.update_score(SCORE_CONNECT_SUCCESS);
11901221

11911222
// negotiate the connection
11921223
match self
@@ -1298,7 +1329,7 @@ impl TransportManager {
12981329

12991330
if transports.is_empty() {
13001331
for (_, mut record) in records {
1301-
record.update_score(SCORE_DIAL_FAILURE);
1332+
record.update_score(SCORE_CONNECT_FAILURE);
13021333
context.addresses.insert(record);
13031334
}
13041335

@@ -2259,7 +2290,7 @@ mod tests {
22592290
} => {
22602291
let seconary_connection = context.secondary_connection.as_ref().unwrap();
22612292
assert_eq!(seconary_connection.address(), &address2);
2262-
assert_eq!(seconary_connection.score(), SCORE_DIAL_SUCCESS);
2293+
assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS);
22632294
}
22642295
state => panic!("invalid state: {state:?}"),
22652296
}
@@ -2282,7 +2313,7 @@ mod tests {
22822313
} => {
22832314
let seconary_connection = peer.secondary_connection.as_ref().unwrap();
22842315
assert_eq!(seconary_connection.address(), &address2);
2285-
assert_eq!(seconary_connection.score(), SCORE_DIAL_SUCCESS);
2316+
assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS);
22862317
assert!(peer.addresses.contains(&address3));
22872318
}
22882319
state => panic!("invalid state: {state:?}"),
@@ -2365,7 +2396,7 @@ mod tests {
23652396
} => {
23662397
let seconary_connection = context.secondary_connection.as_ref().unwrap();
23672398
assert_eq!(seconary_connection.address(), &address2);
2368-
assert_eq!(seconary_connection.score(), SCORE_DIAL_SUCCESS);
2399+
assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS);
23692400
}
23702401
state => panic!("invalid state: {state:?}"),
23712402
}
@@ -2467,7 +2498,7 @@ mod tests {
24672498
} => {
24682499
let seconary_connection = context.secondary_connection.as_ref().unwrap();
24692500
assert_eq!(seconary_connection.address(), &address2);
2470-
assert_eq!(seconary_connection.score(), SCORE_DIAL_SUCCESS);
2501+
assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS);
24712502
}
24722503
state => panic!("invalid state: {state:?}"),
24732504
}
@@ -2579,7 +2610,7 @@ mod tests {
25792610
} => {
25802611
let seconary_connection = context.secondary_connection.as_ref().unwrap();
25812612
assert_eq!(seconary_connection.address(), &address2);
2582-
assert_eq!(seconary_connection.score(), SCORE_DIAL_SUCCESS);
2613+
assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS);
25832614
}
25842615
state => panic!("invalid state: {state:?}"),
25852616
}
@@ -2616,7 +2647,7 @@ mod tests {
26162647
} => {
26172648
let seconary_connection = context.secondary_connection.as_ref().unwrap();
26182649
assert_eq!(seconary_connection.address(), &address2);
2619-
assert_eq!(seconary_connection.score(), SCORE_DIAL_SUCCESS);
2650+
assert_eq!(seconary_connection.score(), SCORE_CONNECT_SUCCESS);
26202651
}
26212652
state => panic!("invalid state: {state:?}"),
26222653
}

Diff for: ‎tests/connection/mod.rs

+148
Original file line numberDiff line numberDiff line change
@@ -1352,3 +1352,151 @@ async fn unspecified_listen_address_websocket() {
13521352
}
13531353
}
13541354
}
1355+
1356+
#[tokio::test]
1357+
async fn simultaneous_dial_then_redial_tcp() {
1358+
simultaneous_dial_then_redial(
1359+
Transport::Tcp(TcpConfig {
1360+
reuse_port: false,
1361+
..Default::default()
1362+
}),
1363+
Transport::Tcp(TcpConfig {
1364+
reuse_port: false,
1365+
..Default::default()
1366+
}),
1367+
)
1368+
.await
1369+
}
1370+
1371+
#[tokio::test]
1372+
async fn simultaneous_dial_then_redial_websocket() {
1373+
simultaneous_dial_then_redial(
1374+
Transport::WebSocket(WebSocketConfig {
1375+
reuse_port: false,
1376+
..Default::default()
1377+
}),
1378+
Transport::WebSocket(WebSocketConfig {
1379+
reuse_port: false,
1380+
..Default::default()
1381+
}),
1382+
)
1383+
.await
1384+
}
1385+
1386+
#[tokio::test]
1387+
async fn simultaneous_dial_then_redial_quic() {
1388+
simultaneous_dial_then_redial(
1389+
Transport::Quic(Default::default()),
1390+
Transport::Quic(Default::default()),
1391+
)
1392+
.await
1393+
}
1394+
1395+
async fn simultaneous_dial_then_redial(transport1: Transport, transport2: Transport) {
1396+
let _ = tracing_subscriber::fmt()
1397+
.with_env_filter(tracing_subscriber::EnvFilter::from_default_env())
1398+
.try_init();
1399+
1400+
let (ping_config1, _ping_event_stream1) = PingConfig::default();
1401+
let config1 = ConfigBuilder::new()
1402+
.with_keypair(Keypair::generate())
1403+
.with_libp2p_ping(ping_config1);
1404+
1405+
let config1 = match transport1 {
1406+
Transport::Tcp(config) => config1.with_tcp(config),
1407+
Transport::Quic(config) => config1.with_quic(config),
1408+
Transport::WebSocket(config) => config1.with_websocket(config),
1409+
}
1410+
.build();
1411+
1412+
let (ping_config2, _ping_event_stream2) = PingConfig::default();
1413+
let config2 = ConfigBuilder::new()
1414+
.with_keypair(Keypair::generate())
1415+
.with_libp2p_ping(ping_config2);
1416+
1417+
let config2 = match transport2 {
1418+
Transport::Tcp(config) => config2.with_tcp(config),
1419+
Transport::Quic(config) => config2.with_quic(config),
1420+
Transport::WebSocket(config) => config2.with_websocket(config),
1421+
}
1422+
.build();
1423+
1424+
let mut litep2p1 = Litep2p::new(config1).unwrap();
1425+
let mut litep2p2 = Litep2p::new(config2).unwrap();
1426+
let peer1 = *litep2p1.local_peer_id();
1427+
let peer2 = *litep2p2.local_peer_id();
1428+
1429+
litep2p1.add_known_address(peer2, litep2p2.listen_addresses().cloned());
1430+
litep2p2.add_known_address(peer1, litep2p1.listen_addresses().cloned());
1431+
1432+
let (_, _) = tokio::join!(litep2p1.dial(&peer2), litep2p2.dial(&peer1));
1433+
1434+
let mut peer1_open = false;
1435+
let mut peer2_open = false;
1436+
1437+
while !peer1_open || !peer2_open {
1438+
tokio::select! {
1439+
event = litep2p1.next_event() => match event.unwrap() {
1440+
Litep2pEvent::ConnectionEstablished { .. } => {
1441+
peer1_open = true;
1442+
}
1443+
_ => {},
1444+
},
1445+
event = litep2p2.next_event() => match event.unwrap() {
1446+
Litep2pEvent::ConnectionEstablished { .. } => {
1447+
peer2_open = true;
1448+
}
1449+
_ => {},
1450+
},
1451+
}
1452+
}
1453+
1454+
let mut peer1_close = false;
1455+
let mut peer2_close = false;
1456+
1457+
while !peer1_close || !peer2_close {
1458+
tokio::select! {
1459+
event = litep2p1.next_event() => match event.unwrap() {
1460+
Litep2pEvent::ConnectionClosed { .. } => {
1461+
peer1_close = true;
1462+
}
1463+
_ => {},
1464+
},
1465+
event = litep2p2.next_event() => match event.unwrap() {
1466+
Litep2pEvent::ConnectionClosed { .. } => {
1467+
peer2_close = true;
1468+
}
1469+
_ => {},
1470+
},
1471+
}
1472+
}
1473+
1474+
let (_, _) = tokio::join!(litep2p1.dial(&peer2), litep2p2.dial(&peer1));
1475+
1476+
let future = async move {
1477+
let mut peer1_open = false;
1478+
let mut peer2_open = false;
1479+
1480+
while !peer1_open || !peer2_open {
1481+
tokio::select! {
1482+
event = litep2p1.next_event() => match event.unwrap() {
1483+
Litep2pEvent::ConnectionEstablished { .. } => {
1484+
peer1_open = true;
1485+
}
1486+
_ => {},
1487+
},
1488+
event = litep2p2.next_event() => match event.unwrap() {
1489+
Litep2pEvent::ConnectionEstablished { .. } => {
1490+
peer2_open = true;
1491+
}
1492+
_ => {},
1493+
},
1494+
}
1495+
}
1496+
};
1497+
1498+
match tokio::time::timeout(std::time::Duration::from_secs(10), future).await {
1499+
Err(_) => panic!("failed to open notification stream"),
1500+
_ => {}
1501+
}
1502+
}

0 commit comments

Comments
 (0)