Skip to content
This repository has been archived by the owner on Mar 11, 2020. It is now read-only.

feat: make listen take an array of addrs #46

Merged
merged 2 commits into from
Apr 19, 2019
Merged

Conversation

jacobheun
Copy link
Contributor

@jacobheun jacobheun commented Apr 18, 2019

This changes listener.listen to take an array of multiaddrs.

Implements https://github.com/libp2p/interface-transport/issues/41

@jacobheun
Copy link
Contributor Author

I've also implemented this change at libp2p/js-libp2p-tcp#104

Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

What's the motivation behind this?

src/errors.js Outdated
@@ -11,6 +11,18 @@ class AbortError extends Error {
}
}

class AllListenersFailedError extends Error {
constructor () {
super('AllListenersFailedError')
Copy link
Member

Choose a reason for hiding this comment

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

This is the message right? Shouldn't this be more human readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the error more verbose to try and provide an actionable error for users.

@@ -168,11 +168,11 @@ The listener object created may emit the following events:

### Start a listener

- `JavaScript` - `await listener.listen(multiaddr)`
- `JavaScript` - `await listener.listen(Array<multiaddr>)`
Copy link
Member

Choose a reason for hiding this comment

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

What happens if it fails to bind to one or more of the addresses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally this would eventually be configurable, libp2p/js-libp2p-websocket-star#61 (comment), so you could specify exactly what happens.

The default behavior we decided would be good is to only error if all addresses fail. This is similar to how websocket-star-multi behaves today.

I think a subsequent improvement would be to add the configuration option. We could add them now, but I wanted to try and block the async changes as little as possible.

@jacobheun
Copy link
Contributor Author

Context helps :), I linked the original issue https://github.com/libp2p/interface-transport/issues/41. This is why websocket-star-multi was created, to better support multiple addresses and not fail outright if we were able to listen to at least 1 address.

Copy link
Contributor

@dirkmc dirkmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jacobheun jacobheun merged commit 1dc5baa into master Apr 19, 2019
@jacobheun jacobheun deleted the feat/listen-array branch April 19, 2019 08:21
@ghost ghost removed the in progress label Apr 19, 2019
@alanshaw
Copy link
Member

Apologies I didn't get a chance to review this properly. Just revisiting it now, I just wanted to voice my concerns:

We've noted above it's not obvious what happens if the transport fails to bind to one or more of the passed addresses. We could partially solve this by updating the docs, but really an API without this ambiguity (as we had before) might be better.

Forcing transports to allow listening on multiple addresses adds a significant bookkeeping overhead for every transport module. Now, not only does the switch have to keep track of a "listener" per transport, every transport now has to also keep track of the listeners they create and ensure they all get closed and cleaned up correctly.

This kind of code can be error prone and feels to me like it might be unnecessarily repetitive.

Right now I'm not 100% convinced it's worth changing the interface in this way to solve the non fatal listen failures. Handling that problem as a separate concern feels like a better way to go. Is there a more general rule we can add to the switch - fatal error only if all listeners fail for a particular transport?

If particular transports need more specialist rules I'd actually be ok with the idea of keeping libp2p-websocket-star-multi, or even pulling that logic into a generic wrapper ignoreListenFails(new WSStar(rendezvousAddrs)) or just folding libp2p-websocket-star-multi into libp2p-websocket-star.

@jacobheun
Copy link
Contributor Author

I've been thinking through various potential scenarios for transport usage and functions, and I think we should revert this. I think handling the abstractions elsewhere as we need them is the better approach.

I think we can still cleanly handle thinks like Transport Auto Retry just as easily without overcomplicating the transport. One thing I was thinking about was retry triggers. For example, retrying a bluetooth listener when bluetooth is turned on on a device, but this could be handled for single listeners or possibly better, a specific add-on to libp2p that retries listening. A similar mechanism could also be leveraged for computer sleep/wake ups.

Distinct addresses should likely just be handled via an additional method on the transport itself, https://github.com/libp2p/js-libp2p-switch/blob/v0.42.10/src/transport.js#L132, or by improving .filter to also handle uniqueness.

dirkmc added a commit that referenced this pull request Apr 29, 2019
jacobheun pushed a commit that referenced this pull request Apr 29, 2019
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.

3 participants