core/muxing: Flatten `StreamMuxer` interface to `poll_{inbound,outbound,address_change,close}` #2724

thomaseizinger · 2022-06-22T15:59:43Z

Description

This is the next changeset on our journey to #2722!

Instead of having a mix of poll_event, poll_outbound and poll_close, we flatten the entire interface of StreamMuxer into 4 individual functions:

poll_inbound
poll_outbound
poll_address_change
poll_close

This design is closer to the design of other async traits like AsyncRead and AsyncWrite. It also allows us to delete the StreamMuxerEvent.

Links to any relevant issues

Part of #2722.

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

thomaseizinger · 2022-06-23T12:03:47Z

Rebased to stay on top of #2710.

mxinden · 2022-06-24T05:30:09Z

Appreciate the elaborate pull request!

Still have to give this more thought.

As you mentioned in #2648, given that we don't have a muxer with backpressure on stream creation today (e.g. QUIC), we will likely not do a good job designing this abstraction at this point in time.

For now, we could get rid of poll_outbound for now, returning an ID on open_outbound, returning the actual outbound stream through the poll_event method.

Again, have to think about it some more.

thomaseizinger · 2022-06-24T06:03:23Z

For now, we could get rid of poll_outbound for now, returning an ID on open_outbound, returning the actual outbound stream through the poll_event method.

If somehow possible, I'd like to get rid of this ID because it is actually unnecessarily complicated. Without this ID, we can drop abstractions like OutboundSubstreams from #2648.

There is nothing special about a substream that requires us to do this explicit book-keeping. All we need is a mechanism for the caller to say: I want a new one.

I started with a simple poll_outbound: bool flag on the poll function but that felt like an odd API.

This variant is not yet constructed but we will need it very soon.

It is easier for clients to only call one poll function that can be configured, which substreams to open. This API change is also trying to plan ahead for muxers like QUIC which actually allow to only open substreams in one direction.

It is unused.

thomaseizinger · 2022-06-24T07:38:14Z

Clean rebase onto master, no changes other than dropping commits of previous parent PR.

thomaseizinger · 2022-06-24T08:12:01Z

I just had what I think is a really good idea!

Instead of having a StreamMuxerEvent, we can have one poll function per concern. Not only is this more consistent with the existing poll_close, this is also how traits like AsyncRead and AsyncWrite are designed.

thomaseizinger · 2022-06-24T10:23:02Z

core/CHANGELOG.md

@@ -1,13 +1,14 @@
 # 0.34.0 - unreleased

- Introduce `StreamMuxerEvent::map_inbound_stream`. See [PR 2691].


I deleted this changelog entry because it is not released yet and it no longer makes any sense if the entire type is gone now!

thomaseizinger · 2022-06-30T08:59:33Z

Instead of having a StreamMuxerEvent, we can have one poll function per concern. Not only is this more consistent with the existing poll_close, this is also how traits like AsyncRead and AsyncWrite are designed.

This is a cool idea and might even give downstream users of StreamMuxer the ability to backpressure incoming streams.

Yep that is what I thought too! Exposing individual knobs has a lot more flexibility.

I do want to try and build a wrapper around this with different trade-offs, i.e. "simpler API, less knobs".

Sorry for the delay here. I will take a more in-depth look.

No rush. I am away from my laptop for the next week or so anyway :)

muxers/yamux/src/lib.rs

thomaseizinger · 2022-07-07T05:52:49Z

swarm/src/connection.rs

            handler: wrapped_handler,
+            open_info: VecDeque::with_capacity(8),


This replaces a SmallVec from Muxing to ensure that we use the OpenInfo in a FIFO manner. A capacity of 8 at least makes only one allocation and thus, the (unbenchmarked) performance profile should be similar.

Open to better ideas.

thomaseizinger · 2022-07-07T05:53:32Z

swarm/src/connection.rs

+            if let Poll::Ready(substream) = self.muxing.poll_inbound(cx)? {
+                self.handler
+                    .inject_substream(substream, SubstreamEndpoint::Listener);
+                continue;
+            }
+
+            if let Poll::Ready(address) = self.muxing.poll_address_change(cx)? {
+                self.handler.inject_address_change(&address);
+                return Poll::Ready(Ok(Event::AddressChange(address)));
            }


I think it makes sense to flip these.

swarm/src/connection.rs

thomaseizinger · 2022-07-07T05:56:24Z

Instead of having a StreamMuxerEvent, we can have one poll function per concern. Not only is this more consistent with the existing poll_close, this is also how traits like AsyncRead and AsyncWrite are designed.

This is a cool idea and might even give downstream users of StreamMuxer the ability to backpressure incoming streams. Sorry for the delay here. I will take a more in-depth look.

Did you get a chance to look at this @mxinden ?

mxinden · 2022-07-07T11:44:26Z

Instead of having a StreamMuxerEvent, we can have one poll function per concern. Not only is this more consistent with the existing poll_close, this is also how traits like AsyncRead and AsyncWrite are designed.

This is a cool idea and might even give downstream users of StreamMuxer the ability to backpressure incoming streams. Sorry for the delay here. I will take a more in-depth look.

Did you get a chance to look at this @mxinden ?

I think we should do this change 👍 As far as I can tell, the QUIC implementation would be easy to adapt:

https://github.com/libp2p/rust-libp2p/pull/2289/files#diff-d59efbc1f8dc6ed1521dd36e906216f4ff4ade561dca9b50beea1da354240cf7

Can we prevent users (not implementors) forgetting to call one of the poll_* methods. Would #[must_use] prevent that? If not, I guess given that libp2p-swarm is the main user, I think it is fine.

I suggest we wait for #2289 and #2622 to adapt to the latest StreamMuxer changes before merging here. That way we can better predict the impact on these implementations. @thomaseizinger what do you think?

//CC @elenaf9 and @kpp for #2289 and //CC @melekes for #2622

thomaseizinger · 2022-07-07T11:49:35Z

Can we prevent users (not implementors) forgetting to call one of the poll_* methods. Would #[must_use] prevent that? If not, I guess given that libp2p-swarm is the main user, I think it is fine.

I don't think it is possible. We might be able to provide a wrapper struct but I see libp2p-core as a set of fundamental building blocks that optimizes for flexibility and thus, reading docs and proper use of the APIs is essential IMO.

thomaseizinger · 2022-07-07T11:51:06Z

I suggest we wait for #2289 and #2622 to adapt to the latest StreamMuxer changes before merging here. That way we can better predict the impact on these implementations. @thomaseizinger what do you think?

Hmm, I'd prefer to merge this early to be honest. Happy to wait for an assessment from @kpp, @elenaf9 and @melekes whether merging first would make things easier.

melekes · 2022-07-12T10:12:56Z

How does the new interface translates when it comes to OutboundSubstream

    type OutboundSubstream = BoxFuture<'static, Result<Arc<DetachedDataChannel>, Self::Error>>;

In webrtc transport, I create a future in open_substream and poll it in poll_outbound. Where am I supposed to create a future now? Note the code I'm using is async.

thomaseizinger · 2022-07-12T12:48:59Z

The new interface provides a poll_outbound function which should eventually return a Substream.

If your implementation is based on async, you'd probably want to have an Option<BoxFuture> which you fill with a new one when it is empty or poll the inner one when it is present.

thomaseizinger · 2022-07-12T12:50:14Z

Currently, this interface still relies on internal mutability but I am planning to make it Pin<&mut Self> in a follow-up PR.

mxinden · 2022-07-14T04:20:07Z

I suggest we wait for #2289 and #2622 to adapt to the latest StreamMuxer changes before merging here. That way we can better predict the impact on these implementations. @thomaseizinger what do you think?

Hmm, I'd prefer to merge this early to be honest. Happy to wait for an assessment from @kpp, @elenaf9 and @melekes whether merging first would make things easier.

Fine by me as well. Mind resolving the conflicts @thomaseizinger?

thomaseizinger · 2022-07-15T12:52:48Z

Conflicts resolved. Please check if I updated the changelog and manifest versions correctly @mxinden !

thomaseizinger · 2022-07-15T13:20:54Z

@mxinden @elenaf9 This is ready for a review again.

mxinden

Thanks for the thorough work! Great to have you here.

mxinden · 2022-07-18T03:13:42Z

swarm/src/connection.rs

-                Poll::Ready(SubstreamEvent::AddressChange(address)) => {
-                    self.handler.inject_address_change(&address);
-                    return Poll::Ready(Ok(Event::AddressChange(address)));
+                    continue; // Go back to the top, handler can potentially make progress again.


Side note, I like this style of:

When one made progress

but one does not return

start from the top via continue as other sub components might be able to make progress now.

I do too although it is a pattern I had to get used to and it is not trivial to understand when you get started with async Rust.

Follow up on libp2p#2724. Given that libp2p-core is bumped to v0.35.0, libp2p-tcp needs to be bumped as well.

Follow up on #2724. Given that libp2p-core is bumped to v0.35.0, libp2p-tcp needs to be bumped as well.

Discussed in libp2p#2722.

…nd,address_change,close}` (libp2p#2724) Instead of having a mix of `poll_event`, `poll_outbound` and `poll_close`, we flatten the entire interface of `StreamMuxer` into 4 individual functions: - `poll_inbound` - `poll_outbound` - `poll_address_change` - `poll_close` This design is closer to the design of other async traits like `AsyncRead` and `AsyncWrite`. It also allows us to delete the `StreamMuxerEvent`.

thomaseizinger changed the title ~~Remove dedicated "outbound" functions from StreamMuxer.~~ core/muxing: Remove dedicated "outbound" functions from StreamMuxer Jun 22, 2022

thomaseizinger mentioned this pull request Jun 22, 2022

Re-design the StreamMuxer trait #2722

Closed

24 tasks

thomaseizinger force-pushed the refactor/remove-io-error-bound branch from c2106db to a28a7c5 Compare June 23, 2022 11:53

thomaseizinger force-pushed the refactor/remove-poll-outbound branch from cd4d01a to a2c64ae Compare June 23, 2022 12:03

Base automatically changed from refactor/remove-io-error-bound to master June 24, 2022 06:26

thomaseizinger added 8 commits June 24, 2022 09:37

Introduce StreamMuxerEvent::OutboundSubstream

fffa5ff

This variant is not yet constructed but we will need it very soon.

Introduce StreamMuxerEvent::map_outbound_substream

e8630d7

Remove OutboundStreamId

66dc071

It is unused.

Inline Muxing type into Connection

f155e6b

Remove obvious comment on how poll functions work

03861fc

Fix benchmarks

ed8f0b7

Fix intra-doc link

3c3bed3

thomaseizinger force-pushed the refactor/remove-poll-outbound branch from a2c64ae to 3c3bed3 Compare June 24, 2022 07:37

Fix documentation of StreamMuxer

90b7ad6

thomaseizinger requested review from mxinden and elenaf9 June 24, 2022 07:42

Minor import and comment cleanup

33c9491

thomaseizinger marked this pull request as ready for review June 24, 2022 07:48

thomaseizinger added 2 commits June 24, 2022 10:05

Remove StreamMuxerEvent in favor of individual poll functions

546603c

Fix mplex tests and benches

68b4c0b

thomaseizinger added 2 commits June 24, 2022 12:16

Remove mentions of deleted event

4a24307

Update changelog

e742b8e

thomaseizinger commented Jun 24, 2022

View reviewed changes

Remove now unused bitflags dependency

2348a4c

thomaseizinger commented Jul 7, 2022

View reviewed changes

thomaseizinger added 2 commits July 15, 2022 14:33

Merge branch 'master' into refactor/remove-poll-outbound

f5cba3d

Update changelog and version numbers

3143a63

thomaseizinger added 6 commits July 15, 2022 14:53

Add unreleased label

bea8318

Add docs to StreamMuxer

8bbdbfa

Fix changelog of yamux and mplex

9409250

Explicitly return result

4f7bd45

Add docs for poll loop in Connection

f20fd74

Fixup a few more changelogs and manifests

3f776ef

mxinden approved these changes Jul 18, 2022

View reviewed changes

mxinden merged commit 1a553db into master Jul 18, 2022

mxinden added a commit to mxinden/rust-libp2p that referenced this pull request Jul 19, 2022

transports/tcp: Bump to v0.35.0

fa9c284

Follow up on libp2p#2724. Given that libp2p-core is bumped to v0.35.0, libp2p-tcp needs to be bumped as well.

mxinden mentioned this pull request Jul 19, 2022

transports/tcp: Bump to v0.35.0 #2760

Merged

mxinden added a commit that referenced this pull request Jul 19, 2022

transports/tcp: Bump to v0.35.0 (#2760)

66c2319

Follow up on #2724. Given that libp2p-core is bumped to v0.35.0, libp2p-tcp needs to be bumped as well.

thomaseizinger deleted the refactor/remove-poll-outbound branch August 2, 2022 10:16

elenaf9 added a commit to elenaf9/rust-libp2p that referenced this pull request Aug 3, 2022

transports/quic: adapt QuicMuxer to libp2p#2724

57840a3

Discussed in libp2p#2722.

elenaf9 mentioned this pull request Aug 3, 2022

transports/quic: Adapt QuicMuxer to upstream StreamMuxer changes elenaf9/rust-libp2p#6

Closed

mxinden mentioned this pull request Aug 8, 2022

core/muxing: Generalise StreamMuxer::poll_address_change to poll #2797

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/muxing: Flatten `StreamMuxer` interface to `poll_{inbound,outbound,address_change,close}` #2724

core/muxing: Flatten `StreamMuxer` interface to `poll_{inbound,outbound,address_change,close}` #2724

thomaseizinger commented Jun 22, 2022 •

edited

thomaseizinger commented Jun 23, 2022

mxinden commented Jun 24, 2022 •

edited

thomaseizinger commented Jun 24, 2022

thomaseizinger commented Jun 24, 2022 •

edited

thomaseizinger commented Jun 24, 2022

thomaseizinger Jun 24, 2022

thomaseizinger commented Jun 30, 2022

thomaseizinger Jul 7, 2022

thomaseizinger Jul 7, 2022

thomaseizinger commented Jul 7, 2022

mxinden commented Jul 7, 2022

thomaseizinger commented Jul 7, 2022 •

edited

thomaseizinger commented Jul 7, 2022

melekes commented Jul 12, 2022

thomaseizinger commented Jul 12, 2022

thomaseizinger commented Jul 12, 2022

mxinden commented Jul 14, 2022

thomaseizinger commented Jul 15, 2022

thomaseizinger commented Jul 15, 2022

mxinden left a comment

mxinden Jul 18, 2022 •

edited

thomaseizinger Jul 18, 2022 •

edited

		@@ -1,13 +1,14 @@
		# 0.34.0 - unreleased

		- Introduce `StreamMuxerEvent::map_inbound_stream`. See [PR 2691].

		handler: wrapped_handler,
		open_info: VecDeque::with_capacity(8),

core/muxing: Flatten StreamMuxer interface to poll_{inbound,outbound,address_change,close} #2724

core/muxing: Flatten StreamMuxer interface to poll_{inbound,outbound,address_change,close} #2724

Conversation

thomaseizinger commented Jun 22, 2022 • edited

Description

Links to any relevant issues

Open Questions

Change checklist

thomaseizinger commented Jun 23, 2022

mxinden commented Jun 24, 2022 • edited

thomaseizinger commented Jun 24, 2022

thomaseizinger commented Jun 24, 2022 • edited

thomaseizinger commented Jun 24, 2022

thomaseizinger Jun 24, 2022

Choose a reason for hiding this comment

thomaseizinger commented Jun 30, 2022

thomaseizinger Jul 7, 2022

Choose a reason for hiding this comment

thomaseizinger Jul 7, 2022

Choose a reason for hiding this comment

thomaseizinger commented Jul 7, 2022

mxinden commented Jul 7, 2022

thomaseizinger commented Jul 7, 2022 • edited

thomaseizinger commented Jul 7, 2022

melekes commented Jul 12, 2022

thomaseizinger commented Jul 12, 2022

thomaseizinger commented Jul 12, 2022

mxinden commented Jul 14, 2022

thomaseizinger commented Jul 15, 2022

thomaseizinger commented Jul 15, 2022

mxinden left a comment

Choose a reason for hiding this comment

mxinden Jul 18, 2022 • edited

Choose a reason for hiding this comment

thomaseizinger Jul 18, 2022 • edited

Choose a reason for hiding this comment

core/muxing: Flatten `StreamMuxer` interface to `poll_{inbound,outbound,address_change,close}` #2724

core/muxing: Flatten `StreamMuxer` interface to `poll_{inbound,outbound,address_change,close}` #2724

thomaseizinger commented Jun 22, 2022 •

edited

mxinden commented Jun 24, 2022 •

edited

thomaseizinger commented Jun 24, 2022 •

edited

thomaseizinger commented Jul 7, 2022 •

edited

mxinden Jul 18, 2022 •

edited

thomaseizinger Jul 18, 2022 •

edited