feat: move Identify I/O from NetworkBehaviour to ConnectionHandler #3208

jxs · 2022-12-07T13:47:42Z

Description

Addresses #2885

Notes

Following the discussion on #2885 this was the simplest way I found to move the I/O to Handler.
Per commit review is suggested.

Links to any relevant issues

Open Questions

Change checklist

I have performed a self-review of my own code
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works
A changelog entry has been made in the appropriate crates

instead of responding pending replies from NetworkBehaviour, send them back to ConnectionHandler. ConnectionHandler for now just receives them, it's implementation of the responding will come next.

reply to the Identification requests from the ConnectionHandler.

thomaseizinger

Thanks!

It would be nice if we changed a few more things:

Initialise the ConnectionHandler with PeerId and observed address directly
Don't send the substream back and forth
Upon receiving a new substream, send an event to the behaviour asking for more information like protocols, upon answer, grab the next substream in line and send it back

I think this would be an overall easier design because it clearly shows, what information is needed from the behaviour that can't be accessed by the handler.

We can even pass stuff like the public key and the agent version directly to the handler upon initialisation.

thomaseizinger · 2022-12-07T20:53:17Z

protocols/identify/src/behaviour.rs

-        io: Pin<Box<dyn Future<Output = Result<(), UpgradeError>> + Send>>,
-    },
+struct Request {
+    peer: PeerId,


The handler could learn this via IntoConnectionHandler. It will never change across its lifespan.

thomaseizinger · 2022-12-07T20:53:34Z

protocols/identify/src/behaviour.rs

-    },
+struct Request {
+    peer: PeerId,
+    io: ReplySubstream<NegotiatedSubstream>,


It would be nice to not pass this back and forth.

Yeah agreed thanks, updated!

thomaseizinger · 2022-12-07T20:54:49Z

protocols/identify/src/behaviour.rs

+struct Request {
+    peer: PeerId,
+    io: ReplySubstream<NegotiatedSubstream>,
+    observed: Multiaddr,


Same here, a Handler can learn this via IntoConnectionHandler and it won't change¹ across its lifetime.

Footnotes

AddressChange is not emitted at the moment but once it will, it is directly given to the handler. ↩

but we only get it on connection_established which seems to be called after new_handler.

You have to implement IntoConnectionHandler additionally on a struct that you return from new_handler, said struct can then capture the ConnectedPoint and initialise the ConnectionHandler with it.

thanks Thomas, updated!

to the behaviour, instead keep track of the info as it doesn't change.

jxs

Thanks for the fast review and guidance Thomas!

I think this would be an overall easier design because it clearly shows, what information is needed from the behaviour that can't be accessed by the handler.

We can even pass stuff like the public key and the agent version directly to the handler upon initialisation.

the downside of that is that information comes from multiple different places instead of coming only from a single Info struct. I updated to code to no longer send back and forth the substream, if you still feel that we should pass first the PeerId on constructor and then all the other details when requested, to prepare for the changes discussed on #2885 no worries I'll update the PR :)

jxs · 2022-12-08T18:04:18Z

protocols/identify/src/handler.rs

@@ -255,6 +302,46 @@ impl ConnectionHandler for Handler {
            }
        }

+        // Check for pending replies to send.
+        if let Some(ref info) = self.info {


NB: I thought let_chains had landed already been stabilized, but it was reverted.

Really? Did you try with Rust 1.65?

yeah, compiler points me to rust-lang/rust#53667, see rust-lang/rust#53667 (comment)

jxs · 2022-12-08T18:04:51Z

protocols/identify/src/behaviour.rs

-    },
+struct Request {
+    peer: PeerId,
+    io: ReplySubstream<NegotiatedSubstream>,


Yeah agreed thanks, updated!

jxs · 2022-12-08T18:13:33Z

protocols/identify/src/behaviour.rs

+struct Request {
+    peer: PeerId,
+    io: ReplySubstream<NegotiatedSubstream>,
+    observed: Multiaddr,


but we only get it on connection_established which seems to be called after new_handler.

thomaseizinger · 2022-12-08T20:10:47Z

Thanks for the fast review and guidance Thomas!

I think this would be an overall easier design because it clearly shows, what information is needed from the behaviour that can't be accessed by the handler.

We can even pass stuff like the public key and the agent version directly to the handler upon initialisation.

the downside of that is that information comes from multiple different places instead of coming only from a single Info struct.

Information already comes from different places! I think it is cleaner to not hide this fact but explicitly show, what information we request from the behaviour.

all the other details when requested

Everything that doesn't change over the lifetime of the handler should come in via the constructor in my opinion. This accurately models in the type system, what data is known when and how it flows through the system! In other words, only the supported protocols and external addresses should come in via the event.

provided on new_handler constructor Everything that doesn't change over the lifetime.

…remove-io-behaviour

thomaseizinger

Thanks! A few more ideas!

thomaseizinger · 2022-12-10T22:18:26Z

protocols/identify/src/behaviour.rs

-    /// Pending replies to send.
-    pending_replies: VecDeque<Reply>,
+    /// Information requests from the handlers to be fullfiled.
+    requests: VecDeque<PeerId>,


I think you'll need to track the connection ID here so you can respond to the correct handler!

thomaseizinger · 2022-12-10T22:20:04Z

protocols/identify/src/behaviour.rs

+            };
+            return Poll::Ready(NetworkBehaviourAction::NotifyHandler {
+                peer_id: peer,
+                handler: NotifyHandler::Any,


Here is the curlpit! This will be buggy with > 1 connection per peer.

Thanks Thomas, addressed.

thomaseizinger · 2022-12-10T22:26:19Z

protocols/identify/src/handler.rs

+    /// Information provided by the `Behaviour` upon requesting.
+    behaviour_info: Option<BehaviourInfo>,


Instead of tracking this here, I think it would be cleaner to:

Store pending substreams in a queue

Request a new BehaviourInfo every time a new substream comes in

Construct a Sending every time you receive a BehaviourInfo from the behaviour by popping the oldest substream from the queue in (1)

That will work off substreams in a FIFO fashion and feed them the most up-to-date information.

Alternatively, we could initialise this in the constructor as well. I think we can assume that new_handler in the behaviour only gets called after it got polled at least once!

Instead of tracking this here, I think it would be cleaner to:

Store pending substreams in a queue

Request a new BehaviourInfo every time a new substream comes in

Construct a Sending every time you receive a BehaviourInfo from the behaviour by popping the oldest substream from the queue in (1)

That will work off substreams in a FIFO fashion and feed them the most up-to-date information.

Thanks, makes sense to me. But used two queues instead of one with different states for a less confusing management of them ptal Thomas.

thomaseizinger · 2022-12-10T23:24:22Z

protocols/identify/src/handler.rs

+
+/// Information provided by the `Behaviour` upon requesting.
+#[derive(Debug)]
+pub struct BehaviourInfo {


Not the best name but should do for now I guess :)

instead of caching the behaviour info.

thomaseizinger

Thanks! This looks good to me now, one more idea but happy to merge anytime!

protocols/identify/src/handler.rs

thomaseizinger · 2022-12-11T21:08:44Z

protocols/identify/src/handler.rs

+    /// Identifying information of the local node that is pushed to a remote.
+    Push(Info),
+    /// Identifying information requested from this node.
+    Identify(BehaviourInfo),


How different are these two really? Isn't exactly the same, just with different semantics?

thanks for recalling this again

large size difference between variants --> protocols/identify/src/handler.rs:174:1 | 174 | / pub enum InEvent { 175 | | /// Identifying information of the local node that is pushed to a remote. 176 | | Push(Info), | | ---------- the largest variant contains at least 304 bytes 177 | | /// Identifying information requested from this node. 178 | | Identify(BehaviourInfo), | | ----------------------- the second-largest variant contains at least 48 bytes 179 | | } | |_^ the entire enum is at least 304 bytes | = note: `-D clippy::large-enum-variant` implied by `-D warnings` = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant help: consider boxing the large fields to reduce the total size of the enum | 176 | Push(Box<Info>), | ~~~~~~~~~

I wasn't talking about the size difference actually.

Semantically, even for an identify push, all the handler needs to know are the updated protocols and listen addresses, which is represented as the BehaviourInfo struct. Can we model it as the same type?

ah right! So I followed along and merge Behaviour's pending_push into requests for better balance when fullfiling them. Previously Push requests would always have priority over Identify ptal Thomas.

thomaseizinger · 2022-12-11T21:11:01Z

protocols/identify/src/handler.rs

@@ -82,6 +114,12 @@ pub struct Handler {
        >; 4],
    >,

+    /// Streams awaiting `BehaviourInfo` to then send identify requests.
+    reply_streams: VecDeque<ReplySubstream<NegotiatedSubstream>>,


Is the abstraction of a ReplySubstream useful here? Would it be easier to just buffer the streams themselves?

Sorry Thomas, not following. What is the alternative? But the send method is implemented for ReplySubstream.

The alternative is inlining send to wherever it is called which I'd guess is only one place?

We don't really do this wrapping of a substream anywhere else in the codebase so it would be nice to align it here.

Constructing a Framed is what we typically do.

ah, right! Thanks! I was able to remove ReplySubstream but send is still being used in more places.
We can go further and deprecate Info and just use the protobuf Identify, but probably do that on a subsequent PR?

ah, right! Thanks! I was able to remove ReplySubstream but send is still being used in more places.
We can go further and deprecate Info and just use the protobuf Identify, but probably do that on a subsequent PR?

Nah, I think that is fine. We usually don't use the protobuf struct directly which is good.

into a single struct with the corresponding Substream protocol.

to better balance incoming requests and fullfil them by order.

mergify · 2022-12-12T20:59:27Z

This pull request has merge conflicts. Could you please resolve them @jxs? 🙏

thomaseizinger

Very nice, this actually went beyond what I thought could be simplified :)

…remove-io-behaviour

mxinden

Great simplifications along the main goal of the pull request. Thanks.

Needs a changelog entry. Otherwise good to go from my end.

…remove-io-behaviour

jxs · 2022-12-13T18:45:02Z

Updated with CHANGELOG.md entry

mxinden

🙏

…ibp2p#3208) Addresses libp2p#2885

Identify: rename pending IdentifyHandler references on doc.

cff7c4a

jxs force-pushed the identify-remove-io-behaviour branch from 0ad547f to 4cf8814 Compare December 7, 2022 15:14

jxs added 2 commits December 7, 2022 15:30

Identify: start moving I/O from NetworkBehaviour,

d703ba6

instead of responding pending replies from NetworkBehaviour, send them back to ConnectionHandler. ConnectionHandler for now just receives them, it's implementation of the responding will come next.

identify: Move I/O from Networkehaviour,

3db1ec1

reply to the Identification requests from the ConnectionHandler.

jxs force-pushed the identify-remove-io-behaviour branch from 4cf8814 to 3db1ec1 Compare December 7, 2022 15:31

thomaseizinger reviewed Dec 7, 2022

View reviewed changes

jxs added 2 commits December 8, 2022 18:00

review: Don't bubble the Substream from the handler

e01c02e

to the behaviour, instead keep track of the info as it doesn't change.

review: improve Request doc wording.

11a9254

jxs commented Dec 8, 2022

View reviewed changes

jxs added 2 commits December 9, 2022 20:34

review: separate Info details provided by the behaviour,

57013bb

provided on new_handler constructor Everything that doesn't change over the lifetime.

Merge branch 'master' of github.com:libp2p/rust-libp2p into identify-…

a84178f

…remove-io-behaviour

thomaseizinger reviewed Dec 10, 2022

View reviewed changes

review: handler, request info on each new substream,

0db0c52

instead of caching the behaviour info.

jxs force-pushed the identify-remove-io-behaviour branch from 02c1783 to 0db0c52 Compare December 11, 2022 19:09

thomaseizinger approved these changes Dec 11, 2022

View reviewed changes

jxs added 5 commits December 12, 2022 00:39

review: use FuturesUnordered for pending replies.

aa0d81a

review: deprecate ReplySubstream.

fa05a2e

review: rename Protocol and ProtocolPush to Identify and Push.

7aab6a6

review: merge BehaviorInfo and InEvent,

963f11d

into a single struct with the corresponding Substream protocol.

review: merge Behaviour pending_push into requests,

8e88bf6

to better balance incoming requests and fullfil them by order.

thomaseizinger approved these changes Dec 12, 2022

View reviewed changes

jxs added 3 commits December 12, 2022 21:31

Merge branch 'master' of github.com:libp2p/rust-libp2p into identify-…

c0afa8f

…remove-io-behaviour

review: fix doc typos.

60470d2

Merge branch 'master' of github.com:libp2p/rust-libp2p into identify-…

b484572

…remove-io-behaviour

mxinden reviewed Dec 13, 2022

View reviewed changes

jxs added 2 commits December 13, 2022 18:43

review: add CHANGELOG.md entry

d457cbd

Merge branch 'master' of github.com:libp2p/rust-libp2p into identify-…

bbe124d

…remove-io-behaviour

mxinden approved these changes Dec 13, 2022

View reviewed changes

mxinden added the send-it label Dec 13, 2022

mergify bot merged commit f80c714 into libp2p:master Dec 13, 2022

mxinden mentioned this pull request Dec 13, 2022

protocols/identify: Move I/O to ConnectionHandler #2885

Closed

jxs added a commit to jxs/rust-libp2p that referenced this pull request Dec 14, 2022

feat: move Identify I/O from NetworkBehaviour to ConnectionHandler (l…

e6e4005

…ibp2p#3208) Addresses libp2p#2885

		/// Information provided by the `Behaviour` upon requesting.
		behaviour_info: Option<BehaviourInfo>,

feat: move Identify I/O from NetworkBehaviour to ConnectionHandler #3208

feat: move Identify I/O from NetworkBehaviour to ConnectionHandler #3208

Conversation

jxs commented Dec 7, 2022 • edited Loading

Description

Notes

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jxs left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Dec 8, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Dec 12, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

jxs commented Dec 13, 2022

mxinden left a comment

Choose a reason for hiding this comment

jxs commented Dec 7, 2022 •

edited

Loading

thomaseizinger left a comment •

edited

Loading

jxs left a comment •

edited

Loading

thomaseizinger Dec 12, 2022 •

edited

Loading