A few BUGFIXES and new tests to cover those scenarios + multiple signalling servers! #89
Conversation
michaelfakhri
changed the title
|
Everything's green! |
| @@ -60,7 +58,11 @@ class WebRTCStar { | |||
| callback = callback ? once(callback) : noop | |||
|
|
|||
| const intentId = (~~(Math.random() * 1e9)).toString(36) + Date.now() | |||
| const sioClient = this.listenersRefs[Object.keys(this.listenersRefs)[0]].io | |||
| const keys = Object.keys(this.listenersRefs) | |||
| .filter((key) => cleanUrlSIO(ma) === cleanUrlSIO(multiaddr(key))) | |||
There was a problem hiding this comment.
Indentation here looks a tad weird, might reconsider making it more readable?
|
I plan to review this PR carefully this week. Apologies for the time waiting ETOOMANYPRS :D |
| sigS2 = server | ||
| console.log('signalling 2 on:', server.info.uri) | ||
| cb() | ||
| }) |
There was a problem hiding this comment.
these two functions could be one, taking the options as a parameter
| } | ||
| function second (cb) { | ||
| sigS2.stop(() => cb()) | ||
| } |
There was a problem hiding this comment.
could be simpler
async.forEach([first, second], (server) => server.stop(cb), done)
|
Hi @michaelfakhri, would love to merge this so that we can actually have more than one signalling rendezvous point (will be super useful when we replace the server with a libp2p node //cc @lgierth ) Are you up to update this PR (rebase master onto it) ? |
|
Ping @michaelfakhri :) |
|
As we completely refactored this module and this PR is not active for a long time I will close it |
Continuing #54
A few mishaps and that branch is no longer merge-able, so I created this new and cleaner branch
Here is a summary of all the changes:
Variable maSelf was redirecting all dials to come back through the last added listener. This definitely does not make any sense, since the last added listener will most likely be another signalling server which will have a completely different multi-address. This means that the peer will never get the answer for the offer creating an error state on the server side and on the peer's side.
Closing the listener did not close the socket connection to the signalling server. This safe to do at the moment since each listener creates a new socket connection to be used.
Moved the close and getObservedAddr functions to be added only when the listen function is called since they do not make any sense if the listener is not listening.There is no multi-address to return or a listener to close if its not listening.
Fixed ss-leave event which did not pass the multi-address to the server
Fixed an issue where the listener was added with a function as the key to the listeners map/array
Moved adding the listener to the listeners array to occur only once its connected to the signalling server, otherwise its just have a bad listener that cannot actually dial anywhere
Part of this PR addresses Websocket stays connected even after webrtc-star listener is closed Websocket stays connected even after webrtc-star listener is closed #51
EDIT: This PR also adds support for handling multiple signalling servers
Let me know if you need me to make any changes.