From 3a84c141787d2206db1ace365fa1b19082d9f00a Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Wed, 11 Jan 2023 16:13:16 +1100 Subject: [PATCH 1/6] Use single quotes to avoid running commands --- .github/workflows/ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9ce4fc5fdea..a25fdf3c4e0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -299,10 +299,10 @@ jobs: - name: Check PR title length run: | - title="${{ github.event.pull_request.title }}" + title='${{ github.event.pull_request.title }}' title_length=${#title} if [ $title_length -gt 72 ] then echo "PR title is too long (greater than 72 characters)" exit 1 - fi \ No newline at end of file + fi From 458aa533d9489aa374658bdba808f45446d4925d Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Fri, 13 Jan 2023 11:47:28 +1100 Subject: [PATCH 2/6] Use intermediary variable --- .github/workflows/ci.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a25fdf3c4e0..c8725c7a332 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -298,9 +298,10 @@ jobs: require_scope: false - name: Check PR title length + env: + TITLE: ${{ github.event.pull_request.title }} run: | - title='${{ github.event.pull_request.title }}' - title_length=${#title} + title_length=${#TITLE} if [ $title_length -gt 72 ] then echo "PR title is too long (greater than 72 characters)" From d0d8b774536831db7a508a871d473a5702f86393 Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 16 Jan 2023 12:30:54 +1100 Subject: [PATCH 3/6] Make connection IDs globally unique --- core/src/connection.rs | 21 ++++++--------------- protocols/gossipsub/src/behaviour/tests.rs | 22 +++++++++++----------- protocols/kad/src/behaviour/test.rs | 2 +- swarm/src/connection/pool.rs | 15 ++------------- 4 files changed, 20 insertions(+), 40 deletions(-) diff --git a/core/src/connection.rs b/core/src/connection.rs index 91008408fe2..39a85750909 100644 --- a/core/src/connection.rs +++ b/core/src/connection.rs @@ -19,27 +19,18 @@ // DEALINGS IN THE SOFTWARE. use crate::multiaddr::{Multiaddr, Protocol}; +use std::sync::atomic::{AtomicUsize, Ordering}; + +static NEXT_CONNECTION_ID: AtomicUsize = AtomicUsize::new(0); /// Connection identifier. #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] pub struct ConnectionId(usize); impl ConnectionId { - /// Creates a `ConnectionId` from a non-negative integer. - /// - /// This is primarily useful for creating connection IDs - /// in test environments. There is in general no guarantee - /// that all connection IDs are based on non-negative integers. - pub fn new(id: usize) -> Self { - Self(id) - } -} - -impl std::ops::Add for ConnectionId { - type Output = Self; - - fn add(self, other: usize) -> Self { - Self(self.0 + other) + /// Returns the next available [`ConnectionId`]. + pub fn next() -> Self { + Self(NEXT_CONNECTION_ID.fetch_add(1, Ordering::SeqCst)) } } diff --git a/protocols/gossipsub/src/behaviour/tests.rs b/protocols/gossipsub/src/behaviour/tests.rs index 26af832e05f..7aa9adccbbc 100644 --- a/protocols/gossipsub/src/behaviour/tests.rs +++ b/protocols/gossipsub/src/behaviour/tests.rs @@ -225,13 +225,13 @@ where gs.on_swarm_event(FromSwarm::ConnectionEstablished(ConnectionEstablished { peer_id: peer, - connection_id: ConnectionId::new(0), + connection_id: ConnectionId::next(), endpoint: &endpoint, failed_addresses: &[], other_established: 0, // first connection })); if let Some(kind) = kind { - gs.on_connection_handler_event(peer, ConnectionId::new(1), HandlerEvent::PeerKind(kind)); + gs.on_connection_handler_event(peer, ConnectionId::next(), HandlerEvent::PeerKind(kind)); } if explicit { gs.add_explicit_peer(&peer); @@ -579,7 +579,7 @@ fn test_join() { // inform the behaviour of a new peer gs.on_swarm_event(FromSwarm::ConnectionEstablished(ConnectionEstablished { peer_id: random_peer, - connection_id: ConnectionId::new(1), + connection_id: ConnectionId::next(), endpoint: &ConnectedPoint::Dialer { address: "/ip4/127.0.0.1".parse::().unwrap(), role_override: Endpoint::Dialer, @@ -959,7 +959,7 @@ fn test_get_random_peers() { *p, PeerConnections { kind: PeerKind::Gossipsubv1_1, - connections: vec![ConnectionId::new(1)], + connections: vec![ConnectionId::next()], }, ) }) @@ -3009,7 +3009,7 @@ fn test_ignore_rpc_from_peers_below_graylist_threshold() { //receive from p1 gs.on_connection_handler_event( p1, - ConnectionId::new(0), + ConnectionId::next(), HandlerEvent::Message { rpc: GossipsubRpc { messages: vec![raw_message1], @@ -3035,7 +3035,7 @@ fn test_ignore_rpc_from_peers_below_graylist_threshold() { //receive from p2 gs.on_connection_handler_event( p2, - ConnectionId::new(0), + ConnectionId::next(), HandlerEvent::Message { rpc: GossipsubRpc { messages: vec![raw_message3], @@ -3647,7 +3647,7 @@ fn test_scoring_p4_invalid_signature() { gs.on_connection_handler_event( peers[0], - ConnectionId::new(0), + ConnectionId::next(), HandlerEvent::Message { rpc: GossipsubRpc { messages: vec![], @@ -4131,7 +4131,7 @@ fn test_scoring_p6() { for id in others.iter().take(3) { gs.on_swarm_event(FromSwarm::ConnectionEstablished(ConnectionEstablished { peer_id: *id, - connection_id: ConnectionId::new(0), + connection_id: ConnectionId::next(), endpoint: &ConnectedPoint::Dialer { address: addr.clone(), role_override: Endpoint::Dialer, @@ -4152,7 +4152,7 @@ fn test_scoring_p6() { for peer in peers.iter().take(3) { gs.on_swarm_event(FromSwarm::ConnectionEstablished(ConnectionEstablished { peer_id: *peer, - connection_id: ConnectionId::new(0), + connection_id: ConnectionId::next(), endpoint: &ConnectedPoint::Dialer { address: addr2.clone(), role_override: Endpoint::Dialer, @@ -4182,7 +4182,7 @@ fn test_scoring_p6() { //two times same ip doesn't count twice gs.on_swarm_event(FromSwarm::ConnectionEstablished(ConnectionEstablished { peer_id: peers[0], - connection_id: ConnectionId::new(0), + connection_id: ConnectionId::next(), endpoint: &ConnectedPoint::Dialer { address: addr, role_override: Endpoint::Dialer, @@ -5194,7 +5194,7 @@ fn test_subscribe_and_graft_with_negative_score() { let (mut gs2, _, _) = inject_nodes1().create_network(); - let connection_id = ConnectionId::new(0); + let connection_id = ConnectionId::next(); let topic = Topic::new("test"); diff --git a/protocols/kad/src/behaviour/test.rs b/protocols/kad/src/behaviour/test.rs index bf3bd7cf17a..7c93e456404 100644 --- a/protocols/kad/src/behaviour/test.rs +++ b/protocols/kad/src/behaviour/test.rs @@ -1294,7 +1294,7 @@ fn network_behaviour_on_address_change() { let local_peer_id = PeerId::random(); let remote_peer_id = PeerId::random(); - let connection_id = ConnectionId::new(1); + let connection_id = ConnectionId::next(); let old_address: Multiaddr = Protocol::Memory(1).into(); let new_address: Multiaddr = Protocol::Memory(2).into(); diff --git a/swarm/src/connection/pool.rs b/swarm/src/connection/pool.rs index 30dff859d51..cf2392ab4d3 100644 --- a/swarm/src/connection/pool.rs +++ b/swarm/src/connection/pool.rs @@ -100,9 +100,6 @@ where /// The pending connections that are currently being negotiated. pending: HashMap>, - /// Next available identifier for a new connection / task. - next_connection_id: ConnectionId, - /// Size of the task command buffer (per task). task_command_buffer_size: usize, @@ -326,7 +323,6 @@ where counters: ConnectionCounters::new(limits), established: Default::default(), pending: Default::default(), - next_connection_id: ConnectionId::new(0), task_command_buffer_size: config.task_command_buffer_size, dial_concurrency_factor: config.dial_concurrency_factor, substream_upgrade_protocol_override: config.substream_upgrade_protocol_override, @@ -412,13 +408,6 @@ where self.established.keys() } - fn next_connection_id(&mut self) -> ConnectionId { - let connection_id = self.next_connection_id; - self.next_connection_id = self.next_connection_id + 1; - - connection_id - } - fn spawn(&mut self, task: BoxFuture<'static, ()>) { self.executor.spawn(task) } @@ -458,7 +447,7 @@ where dial_concurrency_factor_override.unwrap_or(self.dial_concurrency_factor), ); - let connection_id = self.next_connection_id(); + let connection_id = ConnectionId::next(); let (abort_notifier, abort_receiver) = oneshot::channel(); @@ -508,7 +497,7 @@ where return Err((limit, handler)); } - let connection_id = self.next_connection_id(); + let connection_id = ConnectionId::next(); let (abort_notifier, abort_receiver) = oneshot::channel(); From 11e5f74b33efb207a8dc82999a36bf6d50f47d1a Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 16 Jan 2023 12:33:23 +1100 Subject: [PATCH 4/6] Deprecate old new function --- core/src/connection.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/core/src/connection.rs b/core/src/connection.rs index 39a85750909..3949921c3e1 100644 --- a/core/src/connection.rs +++ b/core/src/connection.rs @@ -28,6 +28,16 @@ static NEXT_CONNECTION_ID: AtomicUsize = AtomicUsize::new(0); pub struct ConnectionId(usize); impl ConnectionId { + /// Creates a `ConnectionId` from a non-negative integer. + /// + /// This is primarily useful for creating connection IDs + /// in test environments. There is in general no guarantee + /// that all connection IDs are based on non-negative integers. + #[deprecated(since = "0.39.0", note = "Use `ConnectionId::next` instead.")] + pub fn new(id: usize) -> Self { + Self(id) + } + /// Returns the next available [`ConnectionId`]. pub fn next() -> Self { Self(NEXT_CONNECTION_ID.fetch_add(1, Ordering::SeqCst)) From 47cf01ff7bcfd4f7d9a31d30690021fe2b61ce3b Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Mon, 16 Jan 2023 12:34:39 +1100 Subject: [PATCH 5/6] Add changelog entry --- core/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/CHANGELOG.md b/core/CHANGELOG.md index a7e24f5d616..18a45cccfaf 100644 --- a/core/CHANGELOG.md +++ b/core/CHANGELOG.md @@ -1,3 +1,9 @@ +# 0.39.0 [unreleased] + +- Deprecate `ConnectionId::new` in favor of `ConnectionId::next`. See [PR 3327]. + +[PR 3327]: https://github.com/libp2p/rust-libp2p/pull/3327 + # 0.38.0 - Remove deprecated functions `StreamMuxerExt::next_{inbound,outbound}`. See [PR 3031]. From 1beec8cb3755932f26e02434aa712cb74ae2467f Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Tue, 17 Jan 2023 15:05:09 +1100 Subject: [PATCH 6/6] Remove ordering functionality from `ConnectionId` --- core/src/connection.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/connection.rs b/core/src/connection.rs index 3949921c3e1..3496848aa2e 100644 --- a/core/src/connection.rs +++ b/core/src/connection.rs @@ -24,7 +24,7 @@ use std::sync::atomic::{AtomicUsize, Ordering}; static NEXT_CONNECTION_ID: AtomicUsize = AtomicUsize::new(0); /// Connection identifier. -#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)] pub struct ConnectionId(usize); impl ConnectionId {