feat: support dial only on transport manager to tolerate errors #643
Conversation
doc/CONFIGURATION.md
Outdated
| connEncryption: [SECIO] | ||
| }, | ||
| transportManager: { | ||
| supportDialOnly: true |
There was a problem hiding this comment.
We originally talked about this being a bit more flexible, libp2p/js-libp2p-websocket-star#61 (comment). Perhaps we could change this to something like faultTolerance, and we could export an enum of values that could be used. For now we could just support FATAL_ALL (no transport was able to listen) and NO_FATAL (no listening is required).
There was a problem hiding this comment.
I didn't see this discussion. I agree with adding more flexibility :)
Should we go with NO_FATAL as the default?
There was a problem hiding this comment.
FATAL_ALL, as that's the current behavior. In reality this should be different for browsers/node, but we can update that when we add base configuration support.
| @@ -212,4 +218,11 @@ class TransportManager { | |||
| } | |||
| } | |||
|
|
|||
| const FAULT_TOLERANCE = { | |||
There was a problem hiding this comment.
We should add some jsdocs to describe each of these as they're not specified anywhere.
There was a problem hiding this comment.
I did not find any standard to document the Enum values. I added them in the main body description for now. If we increase this list, we should rethink this.
jacobheun
deleted the
feat/support-dial-only-on-transport-manager-to-tolerate-errors
branch
* feat: support dial only on transport manager to tolerate errors * chore: address review * chore: add jsdoc to transport manager tolerance errors
* feat: support dial only on transport manager to tolerate errors * chore: address review * chore: add jsdoc to transport manager tolerance errors
On `js-ipfs@0.41`, we removed the `websocket-star` module from the libp2p default configuration for browser nodes in `js-ipfs`. We removed it because this module was not refactored during #1670, since the libp2p goal is to [sunset the star protocols](libp2p/js-libp2p#385) and we preferred to use the time refactoring `websocket-star` into improving the browser support. In the refactor, the `webrtc-star` module was refactored so that browser users could discover other peers in the network. In addition, `circuit-relay` may be used for establishing connections between browser nodes using `websockets`. However, this migration has not been smooth for `js-ipfs` users. Users previously using `websocket-star` swarm addresses, did not have enough visibility of this change, since the removal of `websocket-star` was transparent for them and on the default `js-ipfs` release and not technically a programmable breaking change. With the above in mind, once `js-ipfs` users updated they started getting errors from `js-libp2p` in scenarios where they were providing only `websocket-star` swarm addresses. When `js-libp2p` receives listening multiaddrs, it throws an error if it cannot use any to listen on the configured transports, which was the case here (For instance #2779). In `js-libp2p`, we added an option to tolerate these errors [libp2p/js-libp2p#643](libp2p/js-libp2p#643), which was also mentioned for some other cases earlier. After syncing about this issue, we decided to temporarily throw an error when `js-ipfs` receives `websocket-star` multiaddrs. This aims to help users who still need to migrate to identify the problem faster.
This PR allows libp2p users to support dial only mode, which will make transport manager tolerate errors, if none of the multiaddrs provided were successful used for listening on the configured transports.
We can also have this in the
addressesconfiguration, but I preferred to have it assigned to the correct component. Moreover, I default this to the current behaviour but I do not have a strong opinion if we should go this way, or invert the logic. Let me know your thoughtsCloses #575