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

Connections & upgrading #168

Merged
merged 19 commits into from
Jun 24, 2019
Merged

Connections & upgrading #168

merged 19 commits into from
Jun 24, 2019

Conversation

yusefnapora
Copy link
Contributor

There's still a ways to go on this, but so far it covers the basic protocol negotiation & connection upgrade process.

One thing I'm not sure is correct is the roles of dialer and listener in the negotiation process. I've written it out with both sides sending the initial multistream header simultaneously, followed by the dialing peer proposing and the listener either accepting or rejecting. Does it work the other way as well, with the listening peer proposing protocols?

@JustMaier
Copy link

Hey @yusefnapora, I just wanted to say thanks for your efforts on this. As someone trying to grapple what exactly is happening in the connection lifecycle, this is super helpful and this documentation is much needed.

@yusefnapora
Copy link
Contributor Author

Thanks for the kind words @JustMaier! I pushed a few more paragraphs just now, so hopefully that's helpful as well. Let me know if anything is hard to parse or understand :)

priority mechanism for selecting which connections are the most "expendable"
when you're near the limit.

Resource allocation, measurement and enforcement policies are all an active area

Choose a reason for hiding this comment

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

I'm interested in this discussion, where can I find this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a proposal for go-libp2p here: libp2p/go-libp2p#635 - it's not clear yet whether a cross-platform spec is needed, although we may want one if we start exposing metrics about resource consumption, so everything uses the same units.

@yusefnapora yusefnapora marked this pull request as ready for review June 11, 2019 15:21
@yusefnapora yusefnapora requested review from raulk, Stebalien, vyzo, bigs and a user June 11, 2019 15:22
@yusefnapora
Copy link
Contributor Author

I think this is ready to review - I ought to have flipped the draft switch earlier, but got a bit blindsided by moving to NC.

I requested reviews from a few people, but anyone interested is welcome to review, of course. Let me know if you want to be in the Interest Group - otherwise I'll start volunteering some people this week 😄


Even during a single invocation of an application, you're likely to benefit from
an in-memory metadata storage facility, which will allow you to cache addresses
for connection resumption. Desining a storage interface which can be backed by

Choose a reason for hiding this comment

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

Small typo here Desining

I also realized that we're actually using the IETF QUIC standard, but I
was linking to the Google / Chromium spec, so I updated the link to
point to the latest draft.
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

This is getting a good shape @yusefnapora 💪

Left some minor feedback

connections/README.md Outdated Show resolved Hide resolved
connections/README.md Outdated Show resolved Hide resolved
connections/README.md Outdated Show resolved Hide resolved
connections/README.md Show resolved Hide resolved
Copy link
Contributor

@bigs bigs left a comment

Choose a reason for hiding this comment

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

@yusefnapora this is incredible! a few tiny stylistic edits, but i won't block this from going through.

new connection or a new stream multiplexed over an existing connection.

Next, both peers will send the multistream protocol id to establish that they
want to use multistream-select. Note that both sides may send the initial
Copy link
Contributor

Choose a reason for hiding this comment

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

-"Note that b"
+B

with `"na"`.

When registering protocol handlers, it's possible to use a custom predicate or
"match function", which will receive incoming protocol ids and return `true` if
Copy link
Contributor

Choose a reason for hiding this comment

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

return a boolean communicating whether or not the protocol is supporte

the number of open connections. Doing so while still maintaining robust
performance and connectivity will likely require implementing some kind of
priority mechanism for selecting which connections are the most "expendable"
when you're near the limit.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe note that protocols based on connectionless protocols (aka QUIC via UDP) can be suspended and resumed with much lower overhead

| ClosedStream | A stream has closed |
| Listen | We've started listening on a new address |
| ListenClose | We've stopped listening on an address |

Copy link
Contributor

Choose a reason for hiding this comment

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

in the future, we're eager to explore a services style model to allow subsystems to communicate. maybe doesn't need to be in this draft directly, but something to think about!


While the event bus refactoring is specific to go-libp2p, a future spec may
standardize event types used to communicate information across key libp2p
subsystems, and may possibly require libp2p implementations to provide an
Copy link
Member

Choose a reason for hiding this comment

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

Fwiw, that's something that rust-libp2p would definitely never implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good to know 😄

I honestly don't know if there's value in specifying the event system, since it feels like an implementation detail. The reason I added this note is that there's some ongoing work now (in go-libp2p) to dynamically change the DHT behavior in response to events from another protocol (identify). It seems likely that other kinds of "loosely coupled" interactions are likely to emerge in go-libp2p once the event bus is in place. So really I just want something to refer to when writing about those kinds of interactions, e.g. "the DHT may do X in response to Y events from component Z".

I think actually spec'ing the mechanism for event delivery is probably too much, and maybe even referring to "events" at all is too implementation-specific.

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM

Great work @yusefnapora 👌

@yusefnapora
Copy link
Contributor Author

Thanks for the feedback and kind words everyone!

@yusefnapora yusefnapora merged commit ae6a8b7 into master Jun 24, 2019
@yusefnapora yusefnapora deleted the feat/conn-handshake branch June 24, 2019 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants