Add connection type param to hangUp()

[dirkmc]

Fixes libp2p/js-libp2p#299

Note: Requires libp2p/js-libp2p-circuit#41 for tests to pass

[jacobheun]

@dirkmc I reran CI so the latest libp2p-circuit would get pulled in. Looks like there are some failures, can you take a look at those?

[dirkmc]

Thanks Jacob, I’m in Nicaragua this week but I can take a look when I’m back next Wednesday

…

On Wed, Jan 2, 2019 at 2:37 PM Jacob Heun ***@***.***> wrote: @dirkmc <https://github.com/dirkmc> I reran CI so the latest libp2p-circuit would get pulled in. Looks like there are some failures, can you take a look at those? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#298 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKUpMI5GDvrv9jR-xVAGv0OvagKEEaYks5u_Rh5gaJpZM4ZfCzh> .

[dirkmc]

@jacobheun I made the tests more consistent so it looks like everything is passing now

[jacobheun]

One request, this is a useful function that should probably get used in some of the other test suites (not in this PR). Can we move this to a new utils index file in the test folder?

Other than that, this looks great!

[dirkmc]

I saw there is already a test/utils.js file so I put it in there, let me know if that works

[jacobheun]

I ran this against the libp2p test suites and encountered a couple test failures with the circuit relay tests there. I am taking a look at those to see what's going on.

[dirkmc]

@jacobheun did you have any luck finding why the libp2p tests are failing?

[jacobheun]

I haven't had a chance to look at in too much depth. I'm a bit unavailable this week, but will be able to look at it next week if I can't get to it later this week.

[dirkmc]

Great, thank you

[jacobheun]

Okay, I had some time today to look into the issue. The failure with the circuit relay tests in the js-libp2p repo is due to the change in dialer:

let connection = _switch.connection.getOne(b58Id) // old
let connection = _switch.connection.getOne(b58Id, ConnectionFSM.ConnectionType.Outgoing) // new

Two nodes dial to the relay, both will have outgoing connections. When nodeA attempts to dial to nodeB over the relay the connection from nodeA -> Relay is fine. When the Relay attempts to connect to nodeB, it has an issue because it doesn't use the existing connection to nodeB, since that connection is Incoming for the relay. If the relay is passive, it won't attempt dialing so the request fails. An active relay should dial, but it looks like there is a bug there that I will create a separate issue for.

Ideally, I think it would be better to not have the concept of incoming vs outgoing and instead use a goodbye protocol to manage scenarios like that mentioned in libp2p/js-libp2p#299 (comment). As a workaround, we had previously discussed using both as a type. Perhaps an alternative would be to remove type and start using a tags attribute. Tagging is eventually going to be needed by Connection Manager, so this could be the starting point for that. When you go to hang up "outgoing" connections you could check the tags. If it also has incoming, just remove the outgoing tag, otherwise hangup.

I need to think about this a bit more.

[dirkmc]

hmm yes it's more complicated that I had imagined :)
Let me know once you've thought it through what makes most sense in the wider context of libp2p, it seems like there may be some bigger structural changes to be made. I'm happy to help out with writing tests and implementation.

[daviddias]

@jacobheun with the refactor, these need to be remade or closed. I'll let you do the review

[jacobheun]

Closing this as tagging will be needed for this and it will likely change the structure of this significantly.