feat(transport): allow `ListenerId` to be user-controlled by dariusc93 · Pull Request #3567 · libp2p/rust-libp2p

dariusc93 · 2023-03-07T19:42:49Z

Description

Transport::listen_on is an asynchronous operation. It returns immediately but the actual process of establishing a listening socket happens as part of Transport::poll which will return one or more TransportEvents related to a particular listen_on call.

Currently, listen_on returns a ListenerId which allows the user of the Transport interface to correlate the events with a particular listen_on call. This "user" is the Swarm runtime. Currently, a user of libp2p establishes a new listening socket by talking to the Swarm::listen_on interface and it is not possible to do the same thing via the NetworkBehaviour trait.

Within the NetworkBehaviour trait, we emit commands to the Swarm like ToSwarm::Dial. These commands don't have a "return value" like a synchronous function does and thus, if we were to add a ToSwarm::ListenOn command, it could not receive the ListenerId from the Transport.

To fix this and to be consistent with our coding guidelines we change the interface of Transport::listen_on to require the user to pass in a ListenerId. This will allow us to construct a command in a NetworkBehaviour that remembers this ID which enables precise tracking of which events containing a ListenerId correlate which a particular listen_on command.

This is especially important in the context of listening on wildcard addresses like 0.0.0.0 because we end up binding to multiple network interfaces and thus emit multiple events for a single listen_on call.

Notes

Links to any relevant issues

Open Questions

Should Transport::listen_on return a ListenerId? This PR doesnt have it return it since its redundant now, but not sure if anybody else feel the same way.

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 · 2023-03-09T08:34:11Z

Can you run rustfmt? I'll have a look at the code over the next days.

It is a breaking change so we'll likely hold off a bit on merging that to reduce the rate of breaking changes.

thomaseizinger · 2023-03-09T08:35:30Z

Should Transport::listen_on return a ListenerId? This PR doesnt have it return it since its redundant now, but not sure if anybody else feel the same way.

Agree with that.

thomaseizinger

I am in favor of this but it is a breaking change so we can't merge it immediately.

transports/tcp/src/provider/async_io.rs

thomaseizinger · 2023-03-12T21:39:18Z

Test failure should go away once you update to latest master.

thomaseizinger

I'd like to move away from random ListenerIds and instead use the same pattern we used for ConnectionIds: a static atomic counter.

transports/quic/src/lib.rs

dariusc93 · 2023-05-04T20:42:39Z

core/src/transport.rs

 pub use self::optional::OptionalTransport;
 pub use self::upgrade::Upgrade;

+static NEXT_LISTENER_ID: AtomicU64 = AtomicU64::new(1);


Should we change this to AtomicUsize and use usize in ListenerId instead?

I don't think it really matters, happy to leave it as a u64. What do you think?

I think we can leave it

Adding to the above the argument of portability.

PowerPC and MIPS platforms with 32-bit pointers do not have AtomicU64 or AtomicI64 types.

https://doc.rust-lang.org/std/sync/atomic/index.html#portability

Thus slight preference for AtomicUsize from my end. Also consistent with ConnectionId.

core/src/transport.rs

thomaseizinger · 2023-05-05T10:18:42Z

Looking forward to this! I updated the PR description to reflect why we are doing this. Once the remaining comments are resolved, I'd also like @mxinden to have a look at this although I do think we are in agreement here.

…`ListenerId::default`

thomaseizinger

LGTM, thanks for the effort!

I would like @mxinden to also review this.

transports/websocket/src/framed.rs

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

mergify · 2023-05-08T03:42:29Z

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

mxinden

Minor comments. This is good to go from my end. Thank you for the work @dariusc93!

core/CHANGELOG.md

mxinden · 2023-05-14T08:20:09Z

core/src/transport.rs

 pub use self::optional::OptionalTransport;
 pub use self::upgrade::Upgrade;

+static NEXT_LISTENER_ID: AtomicU64 = AtomicU64::new(1);


Adding to the above the argument of portability.

PowerPC and MIPS platforms with 32-bit pointers do not have AtomicU64 or AtomicI64 types.

https://doc.rust-lang.org/std/sync/atomic/index.html#portability

Thus slight preference for AtomicUsize from my end. Also consistent with ConnectionId.

Co-authored-by: Max Inden <mail@max-inden.de>

core/CHANGELOG.md

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

refactor(transport): Provide ListenerId in Transport::listen_on

db8dbec

dariusc93 mentioned this pull request Mar 7, 2023

feat(swarm): allow NetworkBehaviours to create and remove listeners #3292

Merged

4 tasks

dariusc93 and others added 6 commits March 7, 2023 14:57

fix: Fix test

3b4c37a

fix: Additional fixes

b35c50f

fix: Additional fixes

de5bd32

fix: Remove ListenerId from Result

e55f4ed

fix: Additional fixes

90446c2

Merge branch 'master' into listen_on_with_id

a651083

dariusc93 and others added 2 commits March 9, 2023 08:15

chore: Formatted code

cc2b41c

Merge branch 'master' into listen_on_with_id

5178876

thomaseizinger reviewed Mar 12, 2023

View reviewed changes

transports/tcp/src/provider/async_io.rs Outdated Show resolved Hide resolved

thomaseizinger added this to the v0.52.0 milestone Mar 12, 2023

dariusc93 added 2 commits March 12, 2023 18:05

Merge branch 'master' into listen_on_with_id

117cf08

chore: Update comments

c755ec0

This comment was marked as resolved.

Sign in to view

dariusc93 added 2 commits May 3, 2023 18:58

Merge branch 'master' into listen_on_with_id

2657ee9

chore: Formatted code

e55144f

thomaseizinger reviewed May 4, 2023

View reviewed changes

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

dariusc93 added 5 commits May 4, 2023 16:35

chore: Update example

9d32aee

chore: Use ListenerId instead of Default

861b7cc

chore: Use atomicu64 instead of random numbers

579e002

Merge branch 'master' into listen_on_with_id

f17fe54

chore: format code

0842eed

dariusc93 commented May 4, 2023

View reviewed changes

chore: Import ListenerId

6acd5ac

thomaseizinger reviewed May 5, 2023

View reviewed changes

core/src/transport.rs Show resolved Hide resolved

thomaseizinger changed the title ~~refactor(transport): Provide ListenerId in Transport::listen_on~~ feat(transport): allow ListenerId to be user-controlled May 5, 2023

dariusc93 added 5 commits May 5, 2023 20:34

Merge branch 'master' into listen_on_with_id

188fb15

refactor: Add ListenerId::next and deprecate ListenerId::new and …

450a2d5

…`ListenerId::default`

chore: formatted code

562d3b5

chore: Use ListenerId::next instead of default

5d5df5c

chore: Use ListenerId::next

1e43396

thomaseizinger marked this pull request as ready for review May 6, 2023 07:26

thomaseizinger previously approved these changes May 6, 2023

View reviewed changes

transports/websocket/src/framed.rs Outdated Show resolved Hide resolved

Update transports/websocket/src/framed.rs

114c271

Co-authored-by: Thomas Eizinger <thomas@eizinger.io>

dariusc93 dismissed thomaseizinger’s stale review via 114c271 May 6, 2023 16:11

chore: Update CHANGELOG.md

07a7071

dariusc93 and others added 4 commits May 7, 2023 23:47

Merge branch 'master' into listen_on_with_id

cf10cc6

Merge branch 'master' into listen_on_with_id

3d65a69

Merge branch 'master' into listen_on_with_id

a3fc4c3

fix: Correct merged code

ffe19ec

mxinden previously approved these changes May 14, 2023

View reviewed changes

dariusc93 and others added 3 commits May 14, 2023 04:34

Update core/CHANGELOG.md

3199300

Co-authored-by: Max Inden <mail@max-inden.de>

Update core/CHANGELOG.md

7d2a295

Co-authored-by: Max Inden <mail@max-inden.de>

chore: Use AtomicUsize

b1ebf65

mxinden reviewed May 14, 2023

View reviewed changes

core/CHANGELOG.md Outdated Show resolved Hide resolved

Update core/CHANGELOG.md

08ceb02

mxinden added the send-it label May 14, 2023

thomaseizinger approved these changes May 14, 2023

View reviewed changes

mergify bot merged commit 5b32c8a into libp2p:master May 14, 2023

mxinden mentioned this pull request May 15, 2023

refactor(quic): rewrite quic using quinn #3454

Merged

13 tasks

Conversation

dariusc93 commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes

Links to any relevant issues

Open Questions

Change checklist

Uh oh!

thomaseizinger commented Mar 9, 2023

Uh oh!

thomaseizinger commented Mar 9, 2023

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomaseizinger commented Mar 12, 2023

Uh oh!

This comment was marked as resolved.

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dariusc93 May 4, 2023

Choose a reason for hiding this comment

Uh oh!

thomaseizinger May 5, 2023

Choose a reason for hiding this comment

Uh oh!

dariusc93 May 6, 2023

Choose a reason for hiding this comment

Uh oh!

mxinden May 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thomaseizinger commented May 5, 2023

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mergify bot commented May 8, 2023

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mxinden May 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dariusc93 commented Mar 7, 2023 •

edited

Loading