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.

Sign up for GitHub

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

Jump to bottom

Add the possibility to assume that multistream-select will succeed #659

Closed
tomaka opened this issue Nov 19, 2018 · 5 comments
Closed

Add the possibility to assume that multistream-select will succeed #659

tomaka opened this issue Nov 19, 2018 · 5 comments
Assignees
@romanb @tomaka
Labels
difficulty:moderate priority:important The changes needed are critical for libp2p, or are blocking another project

Comments

@tomaka
Copy link
Member

tomaka commented Nov 19, 2018 •
edited
Loading

If the connection upgrade only accepts one protocol, we can optimize the multistream-select process by assuming that it works, and closing the connection/substream altogether if later the remote returns "na".

I would suggest modifying dialer_select_proto so that if the size_hint() of the protocols list returns a maximum of 1, immediately succeed after sending the data without waiting for the remote to confirm, and wraps around the returned stream.

The behaviour should also be precisely documented.
@tomaka tomaka added priority:important The changes needed are critical for libp2p, or are blocking another project difficulty:moderate labels Nov 19, 2018
@tomaka tomaka mentioned this issue Nov 20, 2018
Closed
@ghost ghost assigned tomaka Nov 20, 2018
@ghost ghost added the in progress label Nov 20, 2018
@tomaka tomaka mentioned this issue Jan 10, 2019
Closed
@tomaka
Copy link
Member Author

tomaka commented Jan 29, 2019

This should be integrated in such a way that multistream-select 2.0 is easy to plug in later.

@tomaka
Copy link
Member Author

tomaka commented Mar 13, 2019

I think that multistream-select should simplfy add a Negotiated<T> wrapper around the successfully negotiated substreams (even on the listening side), so that we can do whatever we want underneath.

@tomaka tomaka mentioned this issue Mar 13, 2019
Merged
@tomaka tomaka assigned tomaka and mattrutherford and unassigned tomaka Mar 21, 2019
@tomaka
Copy link
Member Author

tomaka commented Apr 3, 2019

Note that shutdown should check the outcome of the negotiation and produce an error if it failed, otherwise failed negotiations would be undetected for write-only substreams.

@tomaka tomaka assigned tomaka and unassigned mattrutherford May 14, 2019
@tomaka
Copy link
Member Author

tomaka commented May 14, 2019

This is actually harder than expected to do properly, because the code of multistream-select is from a period of time where I didn't really know how tokio/futures worked and thus isn't that great.

I would actually suggest a rewrite for multistream-select 2.0.

This was referenced May 14, 2019
Closed
Closed
@romanb romanb self-assigned this Jul 12, 2019
romanb pushed a commit to romanb/rust-libp2p that referenced this issue Jul 16, 2019
Remove tokio-codec dependency from multistream-select.
2d1ebff 
In preparation for the eventual switch from tokio to std futures.

Includes some initial cleanup in preparation for further work
in the context of libp2p#659.
romanb pushed a commit to romanb/rust-libp2p that referenced this issue Jul 16, 2019
Remove tokio-codec dependency from multistream-select.
0036f04 
In preparation for the eventual switch from tokio to std futures.

Includes some initial refactoring in preparation for further work
in the context of libp2p#659.
romanb pushed a commit to romanb/rust-libp2p that referenced this issue Jul 16, 2019
Remove tokio-codec dependency from multistream-select.
f5ce0b1 
In preparation for the eventual switch from tokio to std futures.

Includes some initial refactoring in preparation for further work
in the context of libp2p#659.
romanb pushed a commit to romanb/rust-libp2p that referenced this issue Jul 16, 2019
Remove tokio-codec dependency from multistream-select.
0aecda2 
In preparation for the eventual switch from tokio to std futures.

Includes some initial refactoring in preparation for further work
in the context of libp2p#659.
@romanb romanb mentioned this issue Jul 16, 2019
Merged
romanb pushed a commit to romanb/rust-libp2p that referenced this issue Jul 17, 2019
Remove tokio-codec dependency from multistream-select.
538741a 
In preparation for the eventual switch from tokio to std futures.

Includes some initial refactoring in preparation for further work
in the context of libp2p#659.
romanb pushed a commit to romanb/rust-libp2p that referenced this issue Jul 25, 2019
Remove tokio-codec dependency from multistream-select.
e44d40f 
In preparation for the eventual switch from tokio to std futures.

Includes some initial refactoring in preparation for further work
in the context of libp2p#659.
romanb pushed a commit to romanb/rust-libp2p that referenced this issue Jul 25, 2019
Remove tokio-codec dependency from multistream-select.
e94e21b 
In preparation for the eventual switch from tokio to std futures.

Includes some initial refactoring in preparation for further work
in the context of libp2p#659.
This was referenced Jul 26, 2019
Closed
Merged
romanb added a commit that referenced this issue Jul 29, 2019
@romanb
Remove tokio-codec dependency from multistream-select. (#1203)
2fd9411 
* Remove tokio-codec dependency from multistream-select.

In preparation for the eventual switch from tokio to std futures.

Includes some initial refactoring in preparation for further work
in the context of #659.

* Reduce default buffer sizes.

* Allow more than one frame to be buffered for sending.

* Doc tweaks.

* Remove superfluous (duplicated) Message types.
romanb added a commit that referenced this issue Aug 12, 2019
@romanb
[multistream-select] Reduce roundtrips in protocol negotiation. (#1212)
589d280 
* Remove tokio-codec dependency from multistream-select.

In preparation for the eventual switch from tokio to std futures.

Includes some initial refactoring in preparation for further work
in the context of #659.

* Reduce default buffer sizes.

* Allow more than one frame to be buffered for sending.

* Doc tweaks.

* Remove superfluous (duplicated) Message types.

* Reduce roundtrips in multistream-select negotiation.

1. Enable 0-RTT: If the dialer only supports a single protocol, it can send
   protocol data (e.g. the actual application request) together with
   the multistream-select header and protocol proposal. Similarly,
   if the listener supports a proposed protocol, it can send protocol
   data (e.g. the actual application response) together with the
   multistream-select header and protocol confirmation.

2. In general, the dialer "settles on" an expected protocol as soon
   as it runs out of alternatives. Furthermore, both dialer and listener
   do not immediately flush the final protocol confirmation, allowing it
   to be sent together with application protocol data. Attempts to read
   from the negotiated I/O stream implicitly flushes any pending data.

3. A clean / graceful shutdown of an I/O stream always completes protocol
   negotiation.

The publich API of multistream-select changed slightly, requiring both
AsyncRead and AsyncWrite bounds for async reading and writing due to
the implicit buffering and "lazy" negotiation. The error types have
also been changed, but they were not previously fully exported.

Includes some general refactoring with simplifications and some more tests,
e.g. there was an edge case relating to a possible ambiguity when parsing
multistream-select protocol messages.

* Further missing commentary.

* Remove unused test dependency.

* Adjust commentary.

* Cleanup NegotiatedComplete::poll()

* Fix deflate protocol tests.

* Stabilise network_simult test.

The test implicitly relied on "slow" connection establishment
in order to have a sufficient probability of passing.
With the removal of roundtrips in multistream-select, it is now
more likely that within the up to 50ms duration between swarm1
and swarm2 dialing, the connection is already established, causing
the expectation of step == 1 to fail when receiving a Connected event,
since the step may then still be 0.

This commit aims to avoid these spurious errors by detecting runs
during which a connection is established "too quickly", repeating
the test run.

It still seems theoretically possible that, if connections are always
established "too quickly", the test runs forever. However, given that
the delta between swarm1 and swarm2 dialing is 0-50ms and that the
TCP transport is used, that seems probabilistically unlikely.
Nevertheless, the purpose of the artificial dialing delay between
swarm1 and swarm2 should be re-evaluated and possibly at least
the maximum delay further reduced.

* Complete negotiation between upgrades in libp2p-core.

While multistream-select, as a standalone library and providing
an API at the granularity of a single negotiation, supports
lazy negotiation (and in particular 0-RTT negotiation), in the
context of libp2p-core where any number of negotiations are
composed generically within the concept of composable "upgrades",
it is necessary to wait for protocol negotiation between upgrades
to complete.

* Clarify docs. Simplify listener upgrades.

Since reading from a Negotiated I/O stream implicitly flushes any pending
negotiation data, there is no pitfall involved in not waiting for completion.
@romanb
Copy link
Contributor

romanb commented Aug 22, 2019

This has been done in #1212 and is supported by the multistream-select crate, though libp2p-core cannot fully make use of such optimistic negotiation to avoid pitfalls (see the discussion on the PR). I suggest to eventually create a new issue for the implementation of multi(stream)select-2.0, once that leaves that design phase (see libp2p/specs#205).

@romanb romanb closed this as completed Aug 22, 2019
santos227 pushed a commit to santos227/rustlib that referenced this issue Jun 20, 2022
@romanb
Remove tokio-codec dependency from multistream-select. (#1203)
d066455 
* Remove tokio-codec dependency from multistream-select.

In preparation for the eventual switch from tokio to std futures.

Includes some initial refactoring in preparation for further work
in the context of libp2p/rust-libp2p#659.

* Reduce default buffer sizes.

* Allow more than one frame to be buffered for sending.

* Doc tweaks.

* Remove superfluous (duplicated) Message types.
santos227 pushed a commit to santos227/rustlib that referenced this issue Jun 20, 2022
@romanb
[multistream-select] Reduce roundtrips in protocol negotiation. (#1212)
ed783ec 
* Remove tokio-codec dependency from multistream-select.

In preparation for the eventual switch from tokio to std futures.

Includes some initial refactoring in preparation for further work
in the context of libp2p/rust-libp2p#659.

* Reduce default buffer sizes.

* Allow more than one frame to be buffered for sending.

* Doc tweaks.

* Remove superfluous (duplicated) Message types.

* Reduce roundtrips in multistream-select negotiation.

1. Enable 0-RTT: If the dialer only supports a single protocol, it can send
   protocol data (e.g. the actual application request) together with
   the multistream-select header and protocol proposal. Similarly,
   if the listener supports a proposed protocol, it can send protocol
   data (e.g. the actual application response) together with the
   multistream-select header and protocol confirmation.

2. In general, the dialer "settles on" an expected protocol as soon
   as it runs out of alternatives. Furthermore, both dialer and listener
   do not immediately flush the final protocol confirmation, allowing it
   to be sent together with application protocol data. Attempts to read
   from the negotiated I/O stream implicitly flushes any pending data.

3. A clean / graceful shutdown of an I/O stream always completes protocol
   negotiation.

The publich API of multistream-select changed slightly, requiring both
AsyncRead and AsyncWrite bounds for async reading and writing due to
the implicit buffering and "lazy" negotiation. The error types have
also been changed, but they were not previously fully exported.

Includes some general refactoring with simplifications and some more tests,
e.g. there was an edge case relating to a possible ambiguity when parsing
multistream-select protocol messages.

* Further missing commentary.

* Remove unused test dependency.

* Adjust commentary.

* Cleanup NegotiatedComplete::poll()

* Fix deflate protocol tests.

* Stabilise network_simult test.

The test implicitly relied on "slow" connection establishment
in order to have a sufficient probability of passing.
With the removal of roundtrips in multistream-select, it is now
more likely that within the up to 50ms duration between swarm1
and swarm2 dialing, the connection is already established, causing
the expectation of step == 1 to fail when receiving a Connected event,
since the step may then still be 0.

This commit aims to avoid these spurious errors by detecting runs
during which a connection is established "too quickly", repeating
the test run.

It still seems theoretically possible that, if connections are always
established "too quickly", the test runs forever. However, given that
the delta between swarm1 and swarm2 dialing is 0-50ms and that the
TCP transport is used, that seems probabilistically unlikely.
Nevertheless, the purpose of the artificial dialing delay between
swarm1 and swarm2 should be re-evaluated and possibly at least
the maximum delay further reduced.

* Complete negotiation between upgrades in libp2p-core.

While multistream-select, as a standalone library and providing
an API at the granularity of a single negotiation, supports
lazy negotiation (and in particular 0-RTT negotiation), in the
context of libp2p-core where any number of negotiations are
composed generically within the concept of composable "upgrades",
it is necessary to wait for protocol negotiation between upgrades
to complete.

* Clarify docs. Simplify listener upgrades.

Since reading from a Negotiated I/O stream implicitly flushes any pending
negotiation data, there is no pitfall involved in not waiting for completion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@romanb romanb

@tomaka tomaka

Labels
difficulty:moderate priority:important The changes needed are critical for libp2p, or are blocking another project
Projects
None yet
Milestone
No milestone
3 participants
@romanb @tomaka @mattrutherford