refactor(tcp): use SelectAll for driving listener streams #3361

vnermolaev · 2023-01-20T11:04:32Z

Description

The PR optimizes polling of the listeners in the TCP transport by using futures::SelectAll instead of storing them in a queue and polling manually.

Resolves #2781.

Notes

I observed that the following branches are never triggered

rust-libp2p/transports/tcp/src/lib.rs

Lines 600 to 611 in d82c2a1

    
           Poll::Ready(None) => { 
        
               return Poll::Ready(TransportEvent::ListenerClosed { 
        
                   listener_id: listener.listener_id, 
        
                   reason: Ok(()), 
        
               }); 
        
           } 
        
           Poll::Ready(Some(Err(err))) => { 
        
               return Poll::Ready(TransportEvent::ListenerClosed { 
        
                   listener_id: listener.listener_id, 
        
                   reason: Err(err), 
        
               }); 
        
           }

This is because the underlying ListenStream always returns Poll::Ready(Some(Ok(..))) and neither Poll::Ready(Some(Err(...))) (in-line documentation says such errors are not fatal), nor Poll::Ready(None).

I ended up following the approach found in

https://github.com/vnermolaev/rust-libp2p/blob/acf84113cff5843234ba6ea20c79b079e9d1195c/protocols/relay/src/priv_client/transport.rs#L232-L235

Please advise. Is this the desired behaviour? Otherwise, what approach should I follow? @elenaf9

Links to any relevant issues

#2781

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

jxs

Hi, and thanks for tackling this! Some notes left

transports/tcp/src/lib.rs

protocols/relay/CHANGELOG.md

transports/tcp/src/lib.rs

thomaseizinger

A few comments, otherwise LGTM.

Clean work, thank you!

thomaseizinger · 2023-01-22T22:09:01Z

transports/tcp/src/lib.rs

+        if let Some(event) = me.queued_events.pop_front() {
+            return Poll::Ready(Some(event));
+        }
+
+        if me.is_closed {
+            // Terminate the stream if the listener closed and all remaining events have been reported.
+            return Poll::Ready(None);
+        }


We now have two return paths here that don't register their own waker. You'll need to store a waker in the listener before you return Poll::Pending at the bottom.

I lack expertise on this particular issue. Instead I have aligned the impl for the tcp transport with the impl for the quic transport, they are pretty similar.

@elenaf9 Did you consider this case when implementing QUIC? I think we may have a potential unwoken task here:

Assume 1 active listener

Assume the listener had no work and returned Pending

We call close on the listener

Without an explicit waker, I believe the close will only happen once the listener gets polled again due to another event

Well, it appears this PR can't be merged before this question is cleared out.

We can move forward here by just adding the waker. Worst case, we have an unnecessary wake-up.

Make a new field inside the listener, holding an Option<Waker>

Call it close_listener_waker

Set it before you return Poll::Pending at the bottom

Use take to remove the listener within close and if it is present, call wake on it

@jxs @mxinden perhaps you want to take a look at this too.

In my eyes, the suggestion above by @thomaseizinger is the way to go.

Good catch @thomaseizinger. Yes you are right, we need the explicit waker here.

transports/tcp/src/lib.rs

protocols/relay/CHANGELOG.md

transports/tcp/src/lib.rs

mxinden · 2023-01-26T12:06:37Z

transports/tcp/src/lib.rs

+        if let Some(event) = me.queued_events.pop_front() {
+            return Poll::Ready(Some(event));
+        }
+
+        if me.is_closed {
+            // Terminate the stream if the listener closed and all remaining events have been reported.
+            return Poll::Ready(None);
+        }


In my eyes, the suggestion above by @thomaseizinger is the way to go.

vnermolaev · 2023-01-27T12:32:04Z

@mxinden @thomaseizinger
I have applied the requested changes, i.e.,

We can move forward here by just adding the waker. Worst case, we have an unnecessary wake-up.

* Make a new field inside the listener, holding an `Option<Waker>`

* Call it `close_listener_waker`

* Set it before you return `Poll::Pending` at the bottom

* Use `take` to remove the listener within `close` and if it is present, call `wake` on it

However,

now, there are linting errors coming from parts other than the TCP transport. Should they be addressed separately?
Also the following lint

error: lint `clippy::derive_hash_xor_eq` has been renamed to `clippy::derived_hash_with_manual_eq`
Error:   --> muxers/mplex/src/codec.rs:68:14
   |
68 |     #![allow(clippy::derive_hash_xor_eq)]
   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use the new name: `clippy::derived_hash_with_manual_eq`
   |
   = note: `-D renamed-and-removed-lints` implied by `-D warnings`

if applied will also break the build stating that such a lint is not known... can someone confirm?
2. The issue with sending the last event on closing and thus setting up a waker, I assume, also exists in other transports as it was the case and this one.

mergify · 2023-01-27T17:35:45Z

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

elenaf9

Thank you for tackling this @vnermolaev!

👍 LGTM

now, there are linting errors coming from parts other than the TCP transport. Should they be addressed separately?

They are already addressed in #3389. Would you mind reverting ab06ff8?

The issue with sending the last event on closing and thus setting up a waker, I assume, also exists in other transports as it was the case and this one.

Yes you are right. Would you be interested in doing patches to fix them there as well (in separate PRs; namely for libp2p-quic and libp2p-webrtc)?

transports/tcp/src/lib.rs

elenaf9 · 2023-01-27T18:11:37Z

transports/tcp/src/lib.rs

+        if let Some(event) = me.queued_events.pop_front() {
+            return Poll::Ready(Some(event));
+        }
+
+        if me.is_closed {
+            // Terminate the stream if the listener closed and all remaining events have been reported.
+            return Poll::Ready(None);
+        }


Good catch @thomaseizinger. Yes you are right, we need the explicit waker here.

…t for ListenStream.

vnermolaev · 2023-01-27T18:51:20Z

now, there are linting errors coming from parts other than the TCP transport. Should they be addressed separately?

They are already addressed in #3389. Would you mind reverting ab06ff8?

@elenaf9 I have rebased on top of the latest master, excluding the mentioned commit, clippy still fails on sources outside the TCP transport, i.e., swarm, see the failing check.

elenaf9 · 2023-01-27T19:51:05Z

now, there are linting errors coming from parts other than the TCP transport. Should they be addressed separately?

They are already addressed in #3389. Would you mind reverting ab06ff8?

@elenaf9 I have rebased on top of the latest master, excluding the mentioned commit, clippy still fails on sources outside the TCP transport, i.e., swarm, see the failing check.

Yes, #3389 is not merged yet. The failing check however is only the clippy beta one, which is not required for merging the PR. Ideally #3389 will be merged soon, but it shouldn't block this PR, i.e. we can also merge without.

vnermolaev · 2023-01-27T20:57:21Z

Yes, #3389 is not merged yet. The failing check however is only the clippy beta one, which is not required for merging the PR. Ideally #3389 will be merged soon, but it shouldn't block this PR, i.e. we can also merge without.

then I think it is ready to be merged :)

Yes you are right. Would you be interested in doing patches to fix them there as well (in separate PRs; namely for libp2p-quic and libp2p-webrtc)?

Yes, I am interested.

thomaseizinger · 2023-01-29T03:03:33Z

transports/tcp/CHANGELOG.md

@@ -17,9 +17,12 @@

 - Update `rust-version` to reflect the actual MSRV: 1.60.0. See [PR 3090].

+- Optimize listeners polling behaviour in the tcp transport, fixing [2781](https://github.com/libp2p/rust-libp2p/issues/2781). See [PR 3361].


Do we need this? It is mostly a refactoring that is not user facing.

Could mention the bug fix but otherwise I'd suggest not adding a changelog entry.

transports/tcp/src/lib.rs

Co-authored-by: Elena Frank <[email protected]>

vnermolaev · 2023-01-29T17:46:42Z

All right, all suggestions/requests are satisfied.
Unfortunately, some CI checks fail, not triggered by changes in this PR.

Checking libp2p-request-response v0.24.0 (/home/runner/work/rust-libp2p/rust-libp2p/protocols/request-response)
error[E0369]: cannot multiply `&&EdwardsBasepointTable` by `&Scalar`
   --> /home/runner/.cargo/registry/src/g.yxqyang.asia-1ecc6299db9ec823/snow-0.9.0/src/resolvers/default.rs:132:47
    |
132 |         let point = (&ED25519_BASEPOINT_TABLE * &self.privkey).to_montgomery();
    |                      ------------------------ ^ ------------- &Scalar
    |                      |
    |                      &&EdwardsBasepointTable
    |
help: `*` can be used on `&EdwardsBasepointTable` if you dereference the left-hand side
    |
132 |         let point = (*&ED25519_BASEPOINT_TABLE * &self.privkey).to_montgomery();
    |                      +

and clippy-beta as before.

thomaseizinger · 2023-01-30T00:02:26Z

See mcginty/snow#146 for the build failure.

thomaseizinger · 2023-01-30T00:36:25Z

See mcginty/snow#146 for the build failure.

I am afraid we cannot fix this on our end. We'd have to insert cargo update statements in our CI pipeline which doesn't help our users either. Let's see how responsive the snow maintainers are.

vnermolaev · 2023-01-30T08:23:15Z

Do I understand correctly that this PR is on hold for now?

thomaseizinger · 2023-01-30T10:31:53Z

Do I understand correctly that this PR is on hold for now?

Yes, we can't merge PRs at the moment due to the breakage in snow.

I have a proposal up to track Cargo.lock in Git which allows us to pin an earlier version of curve25519-dalek: #3399

Approvals have been dismissed because the PR was updated after the send-it label was applied.

vnermolaev changed the title ~~transports/tcp: use futures::stream::SelectAll for driving listener streams~~ refactor(transports/tcp: use futures::stream::SelectAll for driving listener streams Jan 20, 2023

vnermolaev changed the title ~~refactor(transports/tcp: use futures::stream::SelectAll for driving listener streams~~ refactor(transports/tcp): use futures::stream::SelectAll for driving listener streams Jan 20, 2023

vnermolaev force-pushed the feature/select-all branch from acef5d2 to acf8411 Compare January 20, 2023 13:58

vnermolaev changed the title ~~refactor(transports/tcp): use futures::stream::SelectAll for driving listener streams~~ refactor(tcp): use SelectAll for driving listener streams Jan 20, 2023

vnermolaev force-pushed the feature/select-all branch from acf8411 to 7491aef Compare January 20, 2023 14:17

vnermolaev marked this pull request as ready for review January 20, 2023 15:14

jxs reviewed Jan 20, 2023

View reviewed changes

transports/tcp/src/lib.rs Outdated Show resolved Hide resolved

protocols/relay/CHANGELOG.md Outdated Show resolved Hide resolved

protocols/relay/CHANGELOG.md Outdated Show resolved Hide resolved

transports/tcp/src/lib.rs Show resolved Hide resolved

thomaseizinger linked an issue Jan 20, 2023 that may be closed by this pull request

transports/tcp: use futures::stream::SelectAll for driving listener streams #2781

Closed

vnermolaev force-pushed the feature/select-all branch from 8880b24 to e054223 Compare January 22, 2023 14:45

thomaseizinger reviewed Jan 22, 2023

View reviewed changes

jxs reviewed Jan 23, 2023

View reviewed changes

transports/tcp/src/lib.rs Outdated Show resolved Hide resolved

vnermolaev force-pushed the feature/select-all branch 3 times, most recently from c1eba3d to 8980a45 Compare January 24, 2023 10:56

mxinden reviewed Jan 26, 2023

View reviewed changes

vnermolaev force-pushed the feature/select-all branch 4 times, most recently from 44b83d9 to ab06ff8 Compare January 27, 2023 12:26

elenaf9 reviewed Jan 27, 2023

View reviewed changes

Victor Ermolaev added 6 commits January 27, 2023 19:33

Make initial shift from using TcpListenerEvent to using TransportEven…

66b41f8

…t for ListenStream.

Complete the shift.

3f4b2e8

Amend changelog.

d889b2d

Address PR comments.

8421e4b

Update polling behaviour.

d8fb545

Add explicit waker to deliver the last event.

df3fa05

vnermolaev force-pushed the feature/select-all branch from ab06ff8 to df3fa05 Compare January 27, 2023 18:35

thomaseizinger reviewed Jan 29, 2023

View reviewed changes

Victor Ermolaev and others added 2 commits January 29, 2023 18:17

Address PR comments.

538afa3

Apply suggestions from code review

4b080d2

Co-authored-by: Elena Frank <[email protected]>

vnermolaev requested a review from thomaseizinger January 29, 2023 17:47

vnermolaev mentioned this pull request Jan 30, 2023

Potentially unwoken tasks in quic and web-rtc #3400

Closed

thomaseizinger previously approved these changes Jan 30, 2023

View reviewed changes

thomaseizinger added the send-it label Jan 30, 2023

Merge branch 'master' into feature/select-all

5188479

thomaseizinger approved these changes Jan 30, 2023

View reviewed changes

Merge branch 'master' into feature/select-all

ff0e502

mergify bot merged commit c15e651 into libp2p:master Jan 30, 2023

vnermolaev deleted the feature/select-all branch January 31, 2023 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(tcp): use SelectAll for driving listener streams #3361

refactor(tcp): use SelectAll for driving listener streams #3361

vnermolaev commented Jan 20, 2023 •

edited by thomaseizinger

Loading

jxs left a comment

thomaseizinger left a comment

thomaseizinger Jan 22, 2023

vnermolaev Jan 23, 2023

thomaseizinger Jan 23, 2023

vnermolaev Jan 25, 2023

thomaseizinger Jan 25, 2023

thomaseizinger Jan 25, 2023

mxinden Jan 26, 2023

elenaf9 Jan 27, 2023

mxinden Jan 26, 2023

vnermolaev commented Jan 27, 2023 •

edited

Loading

mergify bot commented Jan 27, 2023

elenaf9 left a comment •

edited

Loading

elenaf9 Jan 27, 2023

vnermolaev commented Jan 27, 2023 •

edited

Loading

elenaf9 commented Jan 27, 2023 •

edited

Loading

vnermolaev commented Jan 27, 2023

thomaseizinger Jan 29, 2023

vnermolaev Jan 29, 2023

vnermolaev commented Jan 29, 2023

thomaseizinger commented Jan 30, 2023

thomaseizinger commented Jan 30, 2023

vnermolaev commented Jan 30, 2023

thomaseizinger commented Jan 30, 2023 •

edited

Loading

	Poll::Ready(None) => {
	return Poll::Ready(TransportEvent::ListenerClosed {
	listener_id: listener.listener_id,
	reason: Ok(()),
	});
	}
	Poll::Ready(Some(Err(err))) => {
	return Poll::Ready(TransportEvent::ListenerClosed {
	listener_id: listener.listener_id,
	reason: Err(err),
	});
	}

		@@ -17,9 +17,12 @@

		- Update `rust-version` to reflect the actual MSRV: 1.60.0. See [PR 3090].

		- Optimize listeners polling behaviour in the tcp transport, fixing [2781](https://github.com/libp2p/rust-libp2p/issues/2781). See [PR 3361].

refactor(tcp): use SelectAll for driving listener streams #3361

refactor(tcp): use SelectAll for driving listener streams #3361

Conversation

vnermolaev commented Jan 20, 2023 • edited by thomaseizinger Loading

Description

Notes

Links to any relevant issues

Change checklist

jxs left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnermolaev commented Jan 27, 2023 • edited Loading

mergify bot commented Jan 27, 2023

elenaf9 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnermolaev commented Jan 27, 2023 • edited Loading

elenaf9 commented Jan 27, 2023 • edited Loading

vnermolaev commented Jan 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnermolaev commented Jan 29, 2023

thomaseizinger commented Jan 30, 2023

thomaseizinger commented Jan 30, 2023

vnermolaev commented Jan 30, 2023

thomaseizinger commented Jan 30, 2023 • edited Loading

vnermolaev commented Jan 20, 2023 •

edited by thomaseizinger

Loading

vnermolaev commented Jan 27, 2023 •

edited

Loading

elenaf9 left a comment •

edited

Loading

vnermolaev commented Jan 27, 2023 •

edited

Loading

elenaf9 commented Jan 27, 2023 •

edited

Loading

thomaseizinger commented Jan 30, 2023 •

edited

Loading