feat: direct messages #175
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
js-peer/src/lib/direct-message.ts
Outdated
| stream?.abort(e) | ||
| throw e | ||
| } finally { | ||
| await stream?.close() |
There was a problem hiding this comment.
I'm curious what the tradeoffs of opening and closing a stream for every message are. Not a blocker, but useful for learning.
There was a problem hiding this comment.
You’re incurring a small amount of overhead - there will be a round trip for opening a stream, two for multistream select (e.g. negotiating a protocol) and then one for closing the stream afterwards.
This may not be disastrous depending on the frequency of message sending and your network latency to the remote peer, it’s application-specific.
There was a problem hiding this comment.
Another factor is that opening/closing streams can be useful if the remote has a strict limit on the number of incoming protocol streams.
There was a problem hiding this comment.
You’re incurring a small amount of overhead - there will be a round trip for opening a stream, two for multistream select (e.g. negotiating a protocol) and then one for closing the stream afterwards.
If the connection is open already, and const stream = await conn.newStream(DIRECT_MESSAGE_PROTOCOL, { negotiateFully: false }) is called (line 106), will it entail two roundtrips for multistream select?
There was a problem hiding this comment.
Ah yeah - negotiateFully: false will just send the mss header, the line break, and the protocol name in one go, assume success, and carry on so yeah, it doesn't wait to read anything. It'll still wait for the message to be passed to the network layer but that's pretty fast.
There was a problem hiding this comment.
My thought on this was that opening/closing a stream would have a relatively low overhead compared to opening a connection - it also simplifies the code a bit by having one less thing to track / cleanup.
I'm not sure how the connection manager handles connections but would assume that it wouldn't close connections with an open stream, thus best to close the stream when possible.
There was a problem hiding this comment.
would assume that it wouldn't close connections with an open stream
I believe that's correct
thus best to close the stream when possible.
mm, not sure I understood what you mean. Do you mean that you rely on another open stream, i.e.the gossipsub stream that remains open to the peer for the public chat?
There was a problem hiding this comment.
would assume that it wouldn't close connections with an open stream
I believe that's correct
thus best to close the stream when possible.
mm, not sure I understood what you mean. Do you mean that you rely on another open stream, i.e.the gossipsub stream that remains open to the peer for the public chat?
I mean so that it frees the stream resource, so that the connection manager can choose to close the DM connection if needed. Guess I should look at the connection manager code, might be assuming too much here.
|
I made a couple of small suggestions in as I'm going through the PR and testing dozyio#6 (which I couldn't push directly here) |
js-peer/src/components/peer.tsx
Outdated
| leaveTo="transform opacity-0 scale-95" | ||
| > | ||
| <MenuItems className="absolute left-0 z-10 mt-2 w-48 origin-top-right rounded-md bg-white shadow-lg ring-1 ring-black ring-opacity-5 focus:outline-none"> | ||
| {!identified && |
There was a problem hiding this comment.
How were you able to test this?
There was a problem hiding this comment.
How were you able to test this?
I was having an issues connect to the Go relay node (hit a limit on relay connections) and was only able to connect to the rust peer. Gossiping still worked so could see the messages and could test unidentified peers that way, although there is a slight issue.
A peer doesn't add its peer record to the DHT when only using delegated routing so there isn't a way to resolve the multiaddr from the peer id (this was when the rust peer didn't have gossip peer discovery). I haven't thought of a way around this as yet / maybe a non-issue.
There was a problem hiding this comment.
Great stuff! I left a bunch of comments and small suggestions in a separate PR.
From a UX perspective, my suggestion would be to remove the peer menu to reduce the number of clicks. For peer not supporting DMs, this could be a hover tooltip.
Thanks @dozyio
* chore: small simplification * chore: remove from field which can be derived --------- Co-authored-by: Daniel N <2color@users.noreply.github.com>
Thanks for the PR - merged... was going to add a bit more functionality with the menu, but can do that on a separate pr. Will remove for now |
|
@dozyio Is this ready for another review? |
resolved
Good idea - will sort something out next week |
|
@2color can we merge this? would like to do the js-libp2p 2.0 upgrade next week if possible |
|
Let's go! |
Hopefully a bit simpler than the previous PR