Rework swarm and allow interrupting a dial by tomaka · Pull Request #366 · libp2p/rust-libp2p

tomaka · 2018-07-24T14:31:25Z

Removes several Arcs, but most notably allows interrupting current dialing attempts.
This change will also later allow more control, like removing listeners.

tomaka · 2018-07-24T15:52:31Z

Added another commit that should fix #318

tomaka · 2018-07-24T16:17:57Z

Pushed another commit that should fix #320

tomaka · 2018-07-24T16:36:20Z

Added a fix for #332

Signed-off-by: Pierre Krieger <pierre.krieger1708@gmail.com>

gnunicorn

Thanks!

There are a few things that are unclear to me and I'd like to have addressed.

gnunicorn · 2018-07-30T09:39:28Z

core/src/swarm.rs

+    /// dialing fails or the handler has been called with the resulting future.
+    ///
+    /// The returned future is filled with the output of `then`.
+    pub(crate) fn dial_then<Du, F>(&self, multiaddr: Multiaddr, transport: Du, then: F)


I don't understand this change. With dial_then returning a future, can't the same be achieved through dial_then(..).then(|v| v) rather than doing that complex fn-management here. Am I missing something?

The future returned by dial and dial_then doesn't get automatically processed as part of the swarm_future.

Before this change, if you call unique_connec.dial() (or get_or_dial()) and drop the Future returned by dial(), then the unique_connec will not update its state. By putting this logic as part of the swarm_future, we know that it's going to be the case.

Is this not generally useful? Why pub(crate)?

It's true that it's useful, but in my opinion this is a very weird API and I'm not sure we want to expose it.

gnunicorn · 2018-07-30T09:42:57Z

core/src/swarm.rs

+                    let _ = tx.send(then(val));
+                });
+                let mut then = Box::new(move |val: Result<(), IoError>| {
+                    if let Some(then) = then.take() {


Maybe use .expect here to reduce complexity? There is not reasonable explanation the else should ever be reached, or?

gnunicorn · 2018-07-30T10:29:10Z

core/src/swarm.rs

-                Err((err, _)) => {
-                    debug!("Error in listener: {:?}", err);
-                    break
+        for n in (0 .. shared.listeners.len()).rev() {


There is a lot of complexity and nuance hidden in these loops: taking each listener, starting from the last so we can efficiently remove and push items onto the vector, sometimes dropping the listener or replacing it with the remaining, while shuffling the order (although I am not sure this is intended or just a side-effect).

I had to read it three times, and wrote some code to understand what was happening before realizing all nuance and things that happen here.

This making these loops quite hard to understand and fragile to future changes. Is there any way we can reduce the complexity of these loops or at least add inline comments to make sure then next reader doesn't accidentally break it?

gnunicorn · 2018-07-30T10:32:18Z

core/src/transport/interruptible.rs

+}
+
+impl<T> Interruptible<T> {
+    /// Internal function that builds a `Interruptible`.


nitpick: "an"

gnunicorn · 2018-07-30T11:58:32Z

core/src/unique.rs

+            });
+
+        let dial_fut = dial_fut
+            .map_err(|_| IoError::new(IoErrorKind::Other, "multiaddress not supported"))


how are we sure the only reason this failed is because of multi-addr support-problems? Either explain via comment or make a more concrete matching of the error to not cover up internal errors by replacing the message indiscriminately, please.

Yes, that's the design of the dial() and dial_then() methods. They can only Err if the multiaddress is not supported. Any other error is in the produced future.

See #367

twittner · 2018-08-07T13:48:25Z

core/src/unique.rs

+                        task_local! {
+                            static TASK_ID: usize = NEXT_TASK_ID.fetch_add(1, Ordering::Relaxed)
+                        }
+                        tasks_waiting.insert(TASK_ID.with(|&k| k), task::current());


Clever. A pity that tasks do not come with an ID out of the box.

@twittner they do. Unfortunately, it isn't publicly available :( .

twittner · 2018-08-07T13:51:04Z

core/src/swarm.rs

    }
 }

+// TODO: stronger typing


What does this refer to?

All the boxes within Shared :-/

twittner · 2018-08-07T14:10:28Z

core/src/unique.rs

            .map_err(|(err, _)| err)
            .then(move |val| {
-                *inner.lock() = UniqueConnecInner::Empty;
+                drop(cleaner);      // Make sure that `cleaner` gets called there.


Since cleaner is moved here, would it not be sufficient to declare cleaner = Arc::downgrade(&self.inner); above and put the Cleaner::drop body in here? Why a custom Drop impl instead?

In case the user drops the future.
then is only reached if you execute the future, but not if you just destroy it.

Rework swarm and allow interrupting a dial

e95e66d

ghost assigned tomaka Jul 24, 2018

ghost added the in progress label Jul 24, 2018

Improve the UniqueConnec situation

3bd195c

tomaka added 2 commits July 24, 2018 18:04

Codestyle and remove UniqueConnec::get

37c9bf6

Rename get_or_dial() to dial() and add dial_if_empty()

31d61e6

tomaka added 2 commits July 24, 2018 18:28

Clean the UniqueConnec is the future is dropped

9c489db

Rename set_until to tie_or_stop and add tie_or_passthrough

e17db2a

tomaka added 9 commits July 24, 2018 18:39

Minor doc fix

14fe1e8

Fix example

0336fa7

Signed-off-by: Pierre Krieger <pierre.krieger1708@gmail.com>

Merge remote-tracking branch 'upstream/master' into swarm-interrupt-dial

5a75c61

Add some tests

34aa93b

More tests, and a fix

1bca1d1

Fix memory leak with tasks registration

94d6b2a

Some more tests and bugfix

5b604fa

Fix test

2d742d4

Interrupt dialing when a UniqueConnec is dropped or cleared

66fbc0c

gnunicorn suggested changes Jul 30, 2018

View reviewed changes

gnunicorn added the pr-needs-work label Jul 30, 2018

tomaka mentioned this pull request Aug 3, 2018

Fixes to the mplex implementation #360

Merged

tomaka added 3 commits August 3, 2018 15:54

Fix concerns

ed1ef76

Merge branch 'master' into swarm-interrupt-dial

ef47018

Merge branch 'master' into swarm-interrupt-dial

e62d85f

gnunicorn added pr-needs-review and removed pr-needs-work labels Aug 6, 2018

twittner approved these changes Aug 7, 2018

View reviewed changes

gnunicorn approved these changes Aug 8, 2018

View reviewed changes

gnunicorn added pr-good-waiting-for-ci and removed pr-needs-review labels Aug 8, 2018

Merge branch 'master' into swarm-interrupt-dial

cc9360b

ghost assigned gnunicorn Aug 8, 2018

gnunicorn merged commit ea881e3 into libp2p:master Aug 8, 2018

ghost removed the in progress label Aug 8, 2018

gnunicorn removed pr-good-waiting-for-ci labels Aug 8, 2018

tomaka deleted the swarm-interrupt-dial branch August 8, 2018 12:13

Conversation

tomaka commented Jul 24, 2018

Uh oh!

tomaka commented Jul 24, 2018

Uh oh!

tomaka commented Jul 24, 2018

Uh oh!

tomaka commented Jul 24, 2018

Uh oh!

gnunicorn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomaka Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomaka Aug 3, 2018 •

edited

Loading