Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(swarm): allow NetworkBehaviours to create and remove listeners #3292

Merged
merged 50 commits into from
Jun 9, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
4f3b233
feat/networkbehaviour: Added ListenOn and RemoveListener
dariusc93 Dec 30, 2022
d7cc720
Merge branch 'master' into networkbehaviour-listen
dariusc93 Mar 4, 2023
14d0c49
Merge branch 'master' into networkbehaviour-listen
dariusc93 Mar 7, 2023
518295d
Merge branch 'master' into networkbehaviour-listen
dariusc93 Mar 9, 2023
f9f50f8
Merge branch 'networkbehaviour-listen' of github.com:dariusc93/rust-l…
dariusc93 May 3, 2023
ed11db4
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 3, 2023
eaff6f0
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 4, 2023
d104545
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 14, 2023
1892fd8
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 14, 2023
ee4cf30
chore: Use ListenerId in `ToSwarm::ListenOn`
dariusc93 May 14, 2023
a5331cb
Update swarm-derive/src/lib.rs
dariusc93 May 14, 2023
ced5bb0
Update swarm/src/behaviour.rs
dariusc93 May 14, 2023
9d2db40
Update swarm/src/behaviour.rs
dariusc93 May 14, 2023
b26f4fc
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 14, 2023
84895ef
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 15, 2023
ad8856e
chore: Send ListenerError to behaviour
dariusc93 May 15, 2023
df1a19c
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 15, 2023
ec128ac
Update swarm/src/behaviour.rs
thomaseizinger May 15, 2023
97be0c6
chore: Update CHANGELOG.md
dariusc93 May 15, 2023
8cbf386
chore: Update comments
dariusc93 May 15, 2023
6c163a6
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 16, 2023
b326e32
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 23, 2023
0b0f04e
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 24, 2023
ae1089f
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 25, 2023
590f198
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 25, 2023
97ec46d
chore: Return listener error to behaviour
dariusc93 May 25, 2023
48523bf
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 25, 2023
fa64bca
chore: emit event when calling ToSwarm::ListenOn
dariusc93 May 25, 2023
4c9b192
Merge branch 'networkbehaviour-listen' of github.com:dariusc93/rust-l…
dariusc93 May 25, 2023
124a150
chore: Add Swarm::add_listener
dariusc93 May 26, 2023
1f6a798
chore: Add test
dariusc93 May 27, 2023
15fd6e4
chore: Rename test function
dariusc93 May 27, 2023
c8968e0
chore: Formatted code
dariusc93 May 27, 2023
1e40992
chore: Updated test
dariusc93 May 27, 2023
82a9898
Merge branch 'master' into networkbehaviour-listen
dariusc93 May 31, 2023
4d2946d
Merge branch 'master' into networkbehaviour-listen
dariusc93 Jun 2, 2023
d5e36f5
Merge branch 'master' into networkbehaviour-listen
dariusc93 Jun 5, 2023
3e09dc0
Merge branch 'master' into networkbehaviour-listen
dariusc93 Jun 6, 2023
2666615
Merge branch 'master' into networkbehaviour-listen
dariusc93 Jun 6, 2023
95e61ab
chore: Update test
dariusc93 Jun 6, 2023
f880ba2
chore: Move code into add_listener
dariusc93 Jun 6, 2023
7068f12
fix: Move listen on into add_listener
dariusc93 Jun 6, 2023
0bd41ab
chore: Added ListenOpts
dariusc93 Jun 7, 2023
f4c196f
chore: Formatted code
dariusc93 Jun 7, 2023
bc789b2
chore: re-export ListenOpts
dariusc93 Jun 7, 2023
5ea58d1
chore: Updated code
dariusc93 Jun 7, 2023
603933c
chore: Added comment
dariusc93 Jun 7, 2023
08d7659
chore: Formatted code
dariusc93 Jun 7, 2023
92bf3aa
Merge branch 'master' into networkbehaviour-listen
dariusc93 Jun 7, 2023
64d0ad4
Merge branch 'master' into networkbehaviour-listen
dariusc93 Jun 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions swarm-derive/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -699,12 +699,19 @@ fn build_struct(ast: &DeriveInput, data_struct: &DataStruct) -> TokenStream {
}
};


dariusc93 marked this conversation as resolved.
Show resolved Hide resolved
quote!{
match #trait_to_impl::poll(&mut self.#field, cx, poll_params) {
#generate_event_match_arm
std::task::Poll::Ready(#network_behaviour_action::Dial { opts }) => {
return std::task::Poll::Ready(#network_behaviour_action::Dial { opts });
}
std::task::Poll::Ready(#network_behaviour_action::ListenOn { id, address }) => {
return std::task::Poll::Ready(#network_behaviour_action::ListenOn { id, address });
}
std::task::Poll::Ready(#network_behaviour_action::RemoveListener { id }) => {
return std::task::Poll::Ready(#network_behaviour_action::RemoveListener { id });
}
std::task::Poll::Ready(#network_behaviour_action::NotifyHandler { peer_id, handler, event }) => {
return std::task::Poll::Ready(#network_behaviour_action::NotifyHandler {
peer_id,
Expand Down
10 changes: 10 additions & 0 deletions swarm/src/behaviour.rs
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,12 @@ pub enum ToSwarm<TOutEvent, TInEvent> {
/// This allows a [`NetworkBehaviour`] to identify a connection that resulted out of its own dial request.
Dial { opts: DialOpts },

/// Instructs the `Swarm` to listen on the provided address
dariusc93 marked this conversation as resolved.
Show resolved Hide resolved
ListenOn { id: ListenerId, address: Multiaddr },

/// Instructs the `Swarm` to remove the listener
dariusc93 marked this conversation as resolved.
Show resolved Hide resolved
RemoveListener { id: ListenerId },

/// Instructs the `Swarm` to send an event to the handler dedicated to a
/// connection with a peer.
///
Expand Down Expand Up @@ -345,6 +351,8 @@ impl<TOutEvent, TInEventOld> ToSwarm<TOutEvent, TInEventOld> {
match self {
ToSwarm::GenerateEvent(e) => ToSwarm::GenerateEvent(e),
ToSwarm::Dial { opts } => ToSwarm::Dial { opts },
ToSwarm::ListenOn { id, address } => ToSwarm::ListenOn { id, address },
ToSwarm::RemoveListener { id } => ToSwarm::RemoveListener { id },
ToSwarm::NotifyHandler {
peer_id,
handler,
Expand Down Expand Up @@ -374,6 +382,8 @@ impl<TOutEvent, THandlerIn> ToSwarm<TOutEvent, THandlerIn> {
match self {
ToSwarm::GenerateEvent(e) => ToSwarm::GenerateEvent(f(e)),
ToSwarm::Dial { opts } => ToSwarm::Dial { opts },
ToSwarm::ListenOn { id, address } => ToSwarm::ListenOn { id, address },
ToSwarm::RemoveListener { id } => ToSwarm::RemoveListener { id },
ToSwarm::NotifyHandler {
peer_id,
handler,
Expand Down
6 changes: 6 additions & 0 deletions swarm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1121,6 +1121,12 @@ where
}
}
}
ToSwarm::ListenOn { id, address } => {
self.transport.listen_on(id, address).ok()?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this fails, the NetworkBehaviour will never learn about it. I think we need to capture this error and report it back to the NetworkBehaviour as well as issue a ListenerError.

The documentation on ListenerError talks about "non-fatal" errors but I think adding another variant is even more confusing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this back during the initial PR, but I assumed it would be alright since ToSwarm::Dial ignores the error. We could do this instead

Suggested change
self.transport.listen_on(id, address).ok()?;
if let Err(e) = self.transport.listen_on(id, address) {
self.behaviour.on_swarm_event(FromSwarm::ListenerError(
behaviour::ListenerError {
listener_id: id,
err: &e,
},
));
}

So it would notify the behaviour of the error instead of ignoring it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this back during the initial PR, but I assumed it would be alright since ToSwarm::Dial ignores the error.

Are we? Probably also something that should be fixed then :)

We could do this instead

Suggested change
self.transport.listen_on(id, address).ok()?;
if let Err(e) = self.transport.listen_on(id, address) {
self.behaviour.on_swarm_event(FromSwarm::ListenerError(
behaviour::ListenerError {
listener_id: id,
err: &e,
},
));
}

So it would notify the behaviour of the error instead of ignoring it.

Yep, that looks good to me!

Copy link
Member Author

@dariusc93 dariusc93 May 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this back during the initial PR, but I assumed it would be alright since ToSwarm::Dial ignores the error.

Are we? Probably also something that should be fixed then :)

Looking at Swarm::dial, it does send the error to the behaviour internally before returning the error itself so it doesnt look to be necessary

EDIT: Overlooked the beginning, but it looks like it doesnt send anything for dial_opts.get_or_parse_peer_id().map_err(DialError::InvalidPeerId), but not sure if this would need to be sent to the behaviour. If it does we could do this internally there in the function by doing

let peer_id = match dial_opts
            .get_or_parse_peer_id()
            .map_err(DialError::InvalidPeerId)
        {
            Ok(peer_id) => peer_id,
            Err(e) => {
                self.behaviour
                    .on_swarm_event(FromSwarm::DialFailure(DialFailure {
                        peer_id: None,
                        error: &e,
                        connection_id,
                    }));
                return Err(e);
            }
        };

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That error will go away once we have multiformats/rust-multiaddr#73.

}
ToSwarm::RemoveListener { id } => {
self.remove_listener(id);
}
ToSwarm::NotifyHandler {
peer_id,
handler,
Expand Down