Add addresses field for closing listeners #1485

tcharding · 2020-03-09T03:54:21Z

We would like to be able expire addresses when a listener closes.

Add an addresses field to the ListenersEvent and the ListenerClosed to hold the addresses of a listener that has just closed. When we return a ListenerClosed network event, loop over the addresses and call inject_expired_listen_address() on each one.

Fixes: #1482

tomaka · 2020-03-09T09:09:58Z

Our code isn't formatted with rustfmt, so please remove the first commit!

tcharding · 2020-03-09T21:47:22Z

Our code isn't formatted with rustfmt, so please remove the first commit!

Oh, that would explain the indentation for your macro_use! blocks (which is super nice).

Will change my editor setup to match you project and submit non-draft PR :)

Thanks @tomaka

tcharding · 2020-03-09T22:01:22Z

Our code isn't formatted with rustfmt

I see you guys use .editorconfig instead. In order to educate myself; what is the reason behind using this instead of rustfmt please?

Add an addresses field to the ListenersEvent and the ListenerClosed to hold the addresses of a listener that has just closed. When we return a ListenerClosed network event loop over the addresses and call inject_expired_listen_address on each one. Fixes: libp2p#1482

tcharding · 2020-03-09T22:04:44Z

FTR, I did not configure my editor to use .editorconfig. If CI fails (and before patching this repo again) I will invest the time to do so.

tcharding · 2020-03-10T02:58:59Z

I'm not sure how to proceed with the CI integration test failure.

tcharding · 2020-03-10T04:13:08Z

Researching further, rustfmt - is there a reason libp2p does not use rustfmt.toml to get a formatting style we like?

Discussed here: https://www.reddit.com/r/rust/comments/9jl6a9/pro_tip_if_you_use_cargo_fmtrustfmt_use_a/

mxinden · 2020-03-10T11:21:21Z

I'm not sure how to proceed with the CI integration test failure.

As far as I can tell this is unrelated to your patch set. (See #1323.)

In order to educate myself; what is the reason behind using this instead of rustfmt please?

You might find some answers in #1164 and the resources linked within.

mxinden

This looks good to me, but I would like @tomaka or some other maintainer to take another look.

mxinden · 2020-03-10T11:30:18Z

Retriggered the tests. All green now.

tomaka · 2020-03-10T13:12:14Z

core/src/connection/listeners.rs

@@ -148,6 +148,8 @@ where
    Closed {
        /// The ID of the listener that closed.
        listener_id: ListenerId,
+        /// The addresses that the listener was listening on.
+        addresses: SmallVec<[Multiaddr; 4]>,


This is a nit, but I'd rather go with Vec in order to not expose a third-party dependency in our API.
Alternatively, a custom iterator type that wraps around smallvec::IntoIter would work as well.

No worries, will fix and re-spin. Thanks.

tomaka · 2020-03-10T13:13:37Z

core/src/connection/listeners.rs

@@ -283,12 +285,14 @@ where
                Poll::Ready(None) => {
                    return Poll::Ready(ListenersEvent::Closed {
                        listener_id: *listener_project.id,
+                        addresses: listener.addresses.clone(),


Since we're destroying the listener, you don't need to clone.

Suggested change

addresses: listener.addresses.clone(),

addresses: mem::replace(listener.addresses, SmallVec::new()),

(or, for Vec, listener.addresses.drain(..).collect())

I went with a Vec and used the SmallVec method to_vec() for the conversion.

tomaka · 2020-03-10T13:13:48Z

core/src/network/event.rs

@@ -55,6 +56,8 @@ where
    ListenerClosed {
        /// The listener ID that closed.
        listener_id: ListenerId,
+        /// The addresses that the listener was listening on.
+        addresses: SmallVec<[Multiaddr; 4]>,


Same remark

In order to not expose a third party dependency in our API use a `Vec` type for the addresses list instead of a `SmallVec`.

tcharding · 2020-03-10T21:52:55Z

Whats the rust-libp2p projects protocol on squashing/rebasing of PRs? Would you prefer the second commit (added after review) to be squashed into the first or left as it is? Thanks for the reviews.

mxinden · 2020-03-11T08:01:53Z

Whats the rust-libp2p projects protocol on squashing/rebasing of PRs? Would you prefer the second commit (added after review) to be squashed into the first or left as it is? Thanks for the reviews.

@tcharding no action required from your side. We use Github's Squash and merge feature.

tomaka · 2020-03-11T08:19:52Z

core/src/connection/listeners.rs

@@ -283,12 +285,14 @@ where
                Poll::Ready(None) => {
                    return Poll::Ready(ListenersEvent::Closed {
                        listener_id: *listener_project.id,
+                        addresses: listener.addresses.to_vec(),


It's still cloning the content of the container, which is what I was trying to avoid with my suggestion.

Suggested change

addresses: listener.addresses.to_vec(),

addresses: listener.addresses.drain(..).collect(),

Its hard to get good help :) I did try your suggestion but it didn't seem to work. Will explore further. Thanks

I've had a bit more of a play with it. Please correct me if I'm wrong but it seems we cannot get a mutable reference to listener (needed if we want to drain addresses) because we have already taken a mutable reference. We would need the following code to be possible (I think, open to correction):

/// Provides an API similar to `Stream`, except that it cannot end. pub fn poll(mut self: Pin<&mut Self>, cx: &mut Context) -> Poll<ListenersEvent<TTrans>> { let mut remaining = self.listeners.len(); while let Some(mut listener) = self.listeners.pop_back() { let mut listener_project = listener.as_mut().project(); let mut addresses = listener.as_mut().addresses;

This is clearly not possible. Am I missing something, is there another way to do this?

Ah sorry, you just have to use listener_project

Suggested change

addresses: listener.addresses.to_vec(),

addresses: listener_project.addresses.drain(..).collect(),

Cool, thanks. I learned (a little bit) about pin_project, seems I still have a lot to learn about pinning :) Thanks for your patient revies @tomaka. One final thing, just to make sure, are you happy with the remaining usage of SmallVec in connection/listener.rs introduced in this PR?

Closed { /// The ID of the listener that closed. listener_id: ListenerId, + /// The addresses that the listener was listening on. + addresses: SmallVec<[Multiaddr; 4]>,

And, since I'm new and its good to start on the right foot; can you please confirm that the typical pre-PR-push tasks you would expect to pass are only:

cargo build

cargo test

(i.e., no clippy, no e2e tests etc)

Thanks,
Tobin.

are you happy with the remaining usage of SmallVec in connection/listener.rs introduced in this PR?

Ah! No, I missed that. Please turn that into a Vec as well.

There are more tests than cargo test, but running cargo test locally is good enough before opening a PR.

Done, I think we are getting dangerously close to a mergeable PR :)

Wait. Please revert that last change. Having SmallVec in a private field is good.
(you mentioned "introduced in this PR", but your PR didn't touch this.)

Oh boy, you are right. I hope you meant 'revert' figuratively, I removed last patch and force pushed. Thanks for your patience.

core/src/connection/listeners.rs

We would like to avoid clones where possible for efficiency reasons. When returning a `ListenersEvent::Closed` we are already consuming the listener (by way of a pin projection). We can therefore use a consuming iterator instead of cloning. Use `drain(..).collect()` instead of clone to consume the addresses when returning a `ListenersEvent::Closed`.

tomaka

Should be good for merge after that

tomaka · 2020-03-19T09:22:41Z

swarm/src/lib.rs

+                    for addr in addresses.iter() {
+                        this.behaviour.inject_expired_listen_addr(addr);
+                    }


One last change: could you move this above inject_listener_closed?
The listener and its addresses technically expire at the same time, but since here we have to pick an order, it makes more sense that the addresses expire before.

No worries, done!

The listener and its addresses technically expire at the same time, but since here we have to pick an order, it makes more sense that the addresses expire first.

This reverts commit 7bf5266

tcharding mentioned this pull request Mar 9, 2020

Expire listening addresses if a listener is closed #1482

Closed

tcharding force-pushed the add-addresses branch from 95dde72 to f154c81 Compare March 9, 2020 22:02

tcharding marked this pull request as ready for review March 9, 2020 22:03

mxinden approved these changes Mar 10, 2020

View reviewed changes

tomaka requested changes Mar 10, 2020

View reviewed changes

Use Vec instead of SmallVec

7e347a0

In order to not expose a third party dependency in our API use a `Vec` type for the addresses list instead of a `SmallVec`.

tomaka requested changes Mar 11, 2020

View reviewed changes

tcharding force-pushed the add-addresses branch from 92d8612 to 2867fe8 Compare March 16, 2020 21:08

tomaka approved these changes Mar 19, 2020

View reviewed changes

tcharding and others added 2 commits March 20, 2020 08:42

Expire addresses before listener

cd53105

The listener and its addresses technically expire at the same time, but since here we have to pick an order, it makes more sense that the addresses expire first.

Merge branch 'master' into add-addresses

f268456

tomaka merged commit 7bf5266 into libp2p:master Mar 23, 2020

tcharding deleted the add-addresses branch March 23, 2020 20:47

folex added a commit to fluencelabs/rust-libp2p that referenced this pull request Mar 30, 2020

Revert "Add addresses field for closing listeners (libp2p#1485)"

3c183a2

This reverts commit 7bf5266

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add addresses field for closing listeners #1485

Add addresses field for closing listeners #1485

tcharding commented Mar 9, 2020 •

edited

Loading

tomaka commented Mar 9, 2020

tcharding commented Mar 9, 2020 •

edited

Loading

tcharding commented Mar 9, 2020

tcharding commented Mar 9, 2020

tcharding commented Mar 10, 2020

tcharding commented Mar 10, 2020

mxinden commented Mar 10, 2020

mxinden left a comment

mxinden commented Mar 10, 2020

tomaka Mar 10, 2020

tcharding Mar 10, 2020

tomaka Mar 10, 2020

tcharding Mar 10, 2020

tomaka Mar 10, 2020

tcharding commented Mar 10, 2020

mxinden commented Mar 11, 2020

tomaka Mar 11, 2020

tcharding Mar 11, 2020

tcharding Mar 11, 2020 •

edited

Loading

tomaka Mar 12, 2020

tcharding Mar 12, 2020

tomaka Mar 13, 2020

tcharding Mar 15, 2020

tomaka Mar 16, 2020

tcharding Mar 16, 2020

tomaka left a comment

tomaka Mar 19, 2020

tcharding Mar 19, 2020

	addresses: listener.addresses.clone(),
	addresses: mem::replace(listener.addresses, SmallVec::new()),

	addresses: listener.addresses.to_vec(),
	addresses: listener.addresses.drain(..).collect(),

	addresses: listener.addresses.to_vec(),
	addresses: listener_project.addresses.drain(..).collect(),

Add addresses field for closing listeners #1485

Add addresses field for closing listeners #1485

Conversation

tcharding commented Mar 9, 2020 • edited Loading

tomaka commented Mar 9, 2020

tcharding commented Mar 9, 2020 • edited Loading

tcharding commented Mar 9, 2020

tcharding commented Mar 9, 2020

tcharding commented Mar 10, 2020

tcharding commented Mar 10, 2020

mxinden commented Mar 10, 2020

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Mar 10, 2020

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

tcharding commented Mar 10, 2020

mxinden commented Mar 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding Mar 11, 2020 • edited Loading

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

tomaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Mar 9, 2020 •

edited

Loading

tcharding commented Mar 9, 2020 •

edited

Loading

tcharding Mar 11, 2020 •

edited

Loading