(cont) fix: use floodsub events to simplify discovery

[pgte]

Continued from #176

[dirkmc]

I think this approach of explicitly ending the protocol handlers when the collaboration shuts down makes sense, it's more reliable that way.

In the collaboration ConnectionManager._resetConnections(), should there be an await for each call to GlobalConnectionManager.disconnect()? That would require modifying GlobalConnectionManager to return a Promise on hang up (which is probably a good idea)

[dirkmc]

By the way I love the branch name, it sounds like I've discovered something very important 👍

[dirkmc]

Once the connections issue is resolved we should merge in the resultant version of libp2p: https://github.com/libp2p/js-libp2p-switch/issues/288

[pgte]

Quick FYI: I'm working on checking out some test failures I'm observing on this branch (probably not related).

[dirkmc]

Ok thanks for the heads up.

I realized I need to rethink the way ring management and redials work - it's too aggressive in removing peers from the ring, and is trying too many times to reconnect to nodes that go down. I will clean it up in the next couple of hours.

[dirkmc]

I made ring removal less aggressive. There are still a couple of test failures, although they don't appear t to fail consistently

[dirkmc]

We should also make sure libp2p/js-libp2p-floodsub#63 is included because it sometimes causes the tests to fail. It was included in release 0.15.3 but so far that has not yet made it into a js-libp2p release EDIT: Actually this was a minor release so it will be pulled in with libp2p2

[pgte]

@dirkmc this is a failure I'm getting frequently in my test runs (fresh install):

[dirkmc]

Me too, it’s coming from floodsub. I’ll take a look

…

On Thu, Dec 13, 2018 at 3:29 AM Pedro Teixeira ***@***.***> wrote: @dirkmc <https://github.com/dirkmc> this is a failure I'm getting frequently in my test runs (fresh install): [image: 1__bash] <https://user-images.githubusercontent.com/47910/49925526-28160b80-feb1-11e8-94dd-264a7ce3a2ed.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#200 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKUpGQzO_-0FERnfulxJ8rrzjXAgipfks5u4g_ZgaJpZM4ZNDPi> .

[dirkmc]

Fix in libp2p/js-libp2p-floodsub#65

[dirkmc]

Ok @pgte between your fixes and my fixes the tests are now passing 🎉

The outstanding issues I found in the course of this PR are:

• the floodsub issue that you mentioned (I fixed it in fix: crash when disconnect happens during dial libp2p/js-libp2p-floodsub#65 but it looks like they haven't got to it yet
• a different floodsub bug that I introduced with an earlier PR (oops :)). Fix here: fix: allow dials even after error libp2p/js-libp2p-floodsub#66
• the connection closing issue - FIXED
• the way that GlobalConnectionManager keeps track of incoming vs outgoing connections is a little unreliable
• it seems like CI is only running browser tests

These were already issues before I started working on this PR, although it would be nice to have them as part of the PR. What do you think?

[dirkmc]

It would simplify connection management a lot if we could specify to libp2p on hang up that we only want to hang up outbound connections. I created an issue on libp2p: libp2p/js-libp2p#299

[pgte]

@dirkmc for me the tests pass consistently. What do you say about merging this into master and talking about future work on this area (discovery, membership, connection management) on other issues?

[dirkmc]

Sounds good, let’s do it!

…

On Wed, Dec 19, 2018 at 7:26 AM Pedro Teixeira ***@***.***> wrote: @dirkmc <https://github.com/dirkmc> for me the tests pass consistently. What do you say about merging this into master and talking about future work on this area (discovery, membership, connection management) on other issues? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#200 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKUpCv3ghFqaN7WSEP9CaRe1q49ResGks5u6jCEgaJpZM4ZNDPi> .