feat: support multiple listeners #330
Conversation
vasco-santos
force-pushed
the
feat/support-multiple-listeners
branch
from
671cbae to
faa4e47
Compare
vasco-santos
force-pushed
the
feat/support-multiple-listeners
branch
from
faa4e47 to
0bc8068
Compare
|
Needs to be tested in |
| @@ -28,9 +28,6 @@ | |||
| "coverage": "aegir coverage", | |||
| "coverage-publish": "aegir coverage --provider coveralls" | |||
| }, | |||
| "pre-push": [ | |||
| "lint" | |||
| ], | |||
There was a problem hiding this comment.
yes, we have been moving this out of the repos to better enable contributors to reach out with a PR. Most libp2p repos don't have it anymore, just a CI job
| const defer = pDefer() | ||
|
|
||
| // Should be kept unmodified |
There was a problem hiding this comment.
can we have something to enforce this instead otherwise it's easy to miss / make mistake e.g.:
// line 25
const listeningAddr = new ImmutalbleRef()
// line 40
listeningAddr.value = maWhere LazyRef is something along the lines of
class LazyRef {
set value(value) {
Object.defineProperty(this, 'value', { value, writable: false, configurable: false, enumerable: true })
}
get value() {
throw RangeError(`not initialized yet`)
}
}There was a problem hiding this comment.
Thanks, this is a good idea. I will merge this as it is, given that this was already as it is.
Will keep a note for next time I get to times code
| listeningAddr = ma | ||
|
|
||
| let signallingAddr |
There was a problem hiding this comment.
I think it would be a lot easier to comprehend this code if this was
| let signallingAddr | |
| const signallingAddr = withPeerId(ma, upgrader.localPeer) |
where withPeerId would encapsulate logic below.
P.S. This is a third place I see this logic in last two days, makes me wonder if it should be just part of ma itself.
There was a problem hiding this comment.
Perhaps, it would be a good idea! I will merge this as it is, given that this was already as it is.
Will keep a note for next time I get to times code
src/listener.js
Outdated
| @@ -23,23 +23,32 @@ const sioOptions = { | |||
| module.exports = ({ handler, upgrader }, WebRTCStar, options = {}) => { | |||
| const listener = new EventEmitter() | |||
| let listeningAddr | |||
| let sioUrl | |||
There was a problem hiding this comment.
Can we use a more descriptive name ? I'm unable to figure what it stands for I'm guessing s is for signaling, but I'm not sure what's io for.
There was a problem hiding this comment.
Changing to signallingUrl
src/listener.js
Outdated
| } | ||
|
|
||
| listener.on('error', () => defer.reject()) | ||
|
|
||
| const sioUrl = cleanUrlSIO(ma) | ||
| sioUrl = cleanUrlSIO(ma) |
There was a problem hiding this comment.
Do we really need this reference ? Can we instead derive it where it's been used ? which think is only in io.connect and and in close and I don't think either is perf critical.
There was a problem hiding this comment.
I tried to get rid of it in favour of using signallingAddr only and recompute it. But I found the logic to recompute it back clumsy due to the possibility of having the p2p or not
…rrect address (#3633) Error in CI: https://travis-ci.com/github/ipfs/js-ipfs/jobs/500525900 In the past webrtc did not proper support multiple listeners. This basically mean that if trying to listen on multiple addresses the webrtc sdp offers could end up with wrong signal server address throwing random errors in CI. libp2p/js-libp2p-webrtc-star#330 fixed this and now a libp2p node allows multiple listeners. When a peer with multiple webrtc-star listeners attempts to dial another peer via a star signalling server, the signal server of the target peers is identified among the listeners and used for starting the dial. In the test changed in this PR (note that ports might change): ``` nodeA: ['/ip4/127.0.0.1/tcp/53466/ws/p2p-webrtcstar/p2p/QmPqSo3Kqf9XtWG6g3WKVrx2PZ2q3BoFZssVjixaweABNW'] nodeB: [ '/ip4/127.0.0.1/tcp/53466/ws/p2p-webrtc-star/p2p/QmNLojvkVdRTD2bmy7Yzpc7vUBv6bUDEpamrgQeRwPQAg1', '/ip4/127.0.0.1/tcp/53467/ws/p2p-webrtc-star/p2p/QmNLojvkVdRTD2bmy7Yzpc7vUBv6bUDEpamrgQeRwPQAg1' ] ``` and the test did: ``` await nodeA.swarm.connect(nodeB.peerId.addresses[isBrowser ? 1 : 0]) ``` We were using nodeA to dial nodeB with an address of a signalServer it does not use, which obviously failed. The solution here is to either have nodeB dial nodeA address (has same signalServer), or we need to do in the test a match function to understand what address of nodeB we should use (with the same signalServer). The error in CI is poor and webrtc-star should validate we actually have the listener for the signalServer and throw a proper error if not instead of the Cannot read property 'listener' of undefined. This will be fixed shortly in a new PR
This PR adds support for multiple listeners in the same transport instance. The tracking logic for listener references was also simplified.
Closes #315