Skip to content
This repository has been archived by the owner on Apr 24, 2023. It is now read-only.

fix: handle reconnection to signaling server #112

Merged
merged 3 commits into from
May 19, 2017

Conversation

pgte
Copy link
Contributor

@pgte pgte commented May 17, 2017

If a connection to the signalling server dies and then recovers, Socket.io emits the connect event again.
It happens that the peer was ignoring it. This fixes it.

@RichardLitt RichardLitt added the status/in-progress In progress label May 17, 2017
@5310
Copy link

5310 commented May 17, 2017

In my related PR [109] I wanted the peers to reconnect to each other if a peer disconnects from the signalling server by re-sending the 'intro' messages any time the wss connection to the signalling server is reestablished.

This seems like a far more general solution that covers both peer disconnections and signalling-server disconnections (and doesn't seem to trigger Coveralls:) Have you noticed any noticeable performance issues with larger swarms?

src/index.js Outdated
listener.io.emit('ss-join', ma.toString())
listener.io.on('ws-handshake', incommingDial)
listener.io.on('ws-peer', this._peerDiscovered)
listener.emit('listening')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be an issue for libp2p-swarm because it shouldn't emit listening a second time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@daviddias
Copy link
Member

@pgte can you also add tests for this?

listener.io.on('ws-handshake', incommingDial)
listener.io.on('ws-peer', this._peerDiscovered)

listener.io.on('connect', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reconnect is still a connect, IMO needs to go through the same handshake as a connect. Why treat it differently?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense now with the latest change, thanks!

@pgte
Copy link
Contributor Author

pgte commented May 17, 2017

@diasdavid any clue on how to test this? I would like to start and stop the server, but it's buried inside a gulp file. Any idea?

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the testing, just create a Node.js test with the file extension of .node.js and aegir will only run it in Node.js, this way you can rely in your own SigServer

listener.io.on('ws-handshake', incommingDial)
listener.io.on('ws-peer', this._peerDiscovered)

listener.io.on('connect', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense now with the latest change, thanks!

Copy link
Member

@daviddias daviddias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @pgte :)

@daviddias daviddias merged commit 9eaf7df into master May 19, 2017
@daviddias daviddias deleted the fix/ss-server-reconnect branch May 19, 2017 11:45
@daviddias daviddias removed the status/in-progress In progress label May 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants