libp2p / js-libp2p-websocket-star

libp2p-webrtc-star without webrtc. Just plain socket.io.
https://libp2p.io
39 stars 15 forks source link

closing the listener drops underlying connections #49

Closed pgte closed 6 years ago

pgte commented 6 years ago

When closing a listener, all the underlying connections are dropped.

Instead, this should behave like webrtc-star, where the peer connections are maintained. (in Webrtc-star, the severed connection is to the discovery service, effectively preventing from knowing new peers.)

mkg20001 commented 6 years ago

The connections are going through the server, to which the connection gets closed. If the intent is to not discover any more peers then the user should just call ws-star.discovery.stop(cb) instead.

pgte commented 6 years ago

I think that the purpose of stopping a listener is to say "don't accept any more connections", and not "do that AND close all the existing connections".

More context: this discussion.

mkg20001 commented 6 years ago

But if the listener doesn't disconnect on close, when should it do it then?

pgte commented 6 years ago

Well, libp2p-switch keeps track of all the connections, so what's stopping the user or a connection-manager to kill any (or all) connections?

For instance, this is the current behaviour of the websocket-star transport, where listener.close() does not kill connections to other peers.

The behaviour would be analogous of this in the node.js world would be the server.close() (without needing to pass a callback).

mkg20001 commented 6 years ago

The behaviour would be analogous of this in the node.js world would be the server.close() (without needing to pass a callback).

So the connection to the relay should be closed only if 1. all connections are disconnected and 2. the listener is closed?

pgte commented 6 years ago

Yes, I suppose that's how it should be implemented... Thoughts?

mkg20001 commented 6 years ago

@pgte Implemented in #53

jacobheun commented 6 years ago

This is available via #53 in 0.9.0.