feat: use ws-star-multi instead of ws-star

[mkg20001]

This replaces ws-star with ws-star-multi transparently

resolves #1619

[alanshaw]

@mkg20001 please can I get some context? We can already pass multiple discovery/transports to libp2p, what are the benefits of using this module?

[mkg20001]

@alanshaw It allows passing multiple ws-star servers in a way that makes the node only fail to start if all servers are down (or optionally never at all), as compared to previous behavior where the app would fail if any server was down

[lidel]

IIUC this is a fix for ipfs-shipyard/ipfs-share-files#63 (comment) (cc @olizilla @fsdiogo) and #1619 (comment) (cc @satazor)

[parkan]

is this superseded by libp2p/js-libp2p-websocket-star#70 ?

[mkg20001]

@parkan It depends. This is a fix that could be released now. Stardust still needs to be polished which could take a week or two.

[alanshaw]

@alanshaw It allows passing multiple ws-star servers in a way that makes the node only fail to start if all servers are down (or optionally never at all), as compared to previous behavior where the app would fail if any server was down

Yes, that is preferable! Ok so, the other thing I don't understand is why this is implemented as a separate module and not as a fix to libp2p or libp2p-websocket-star?

[mkg20001]

Because that would only allow for ignoring any down server or none
This also allows to ignore some and throw if all are down

This module "listens" on /p2p-websocket-star which internally makes it start listen on all the different server, ignoring the offline ones.

Later it swaps /p2p-websocket-star for the actual addresses (which is actually done by libp2p in the same way /tcp/0 gets replaced by the actual port)

[alanshaw]

Because that would only allow for ignoring any down server or none
This also allows to ignore some and throw if all are down

I can see how that's true if we were to "fix" libp2p-websocket-star, but isn't this behaviour desirable for all libp2p transports? Why not fix it in libp2p?

[mkg20001]

I can see how that's true if we were to "fix" libp2p-websocket-star, but isn't this behaviour desirable for all libp2p transports? Why not fix it in libp2p?

Libp2p has no concept of an "optional listening address",so that would need to get invented first. Additionally there is this problem that not listening on TCP for ex. disables the ability to dial /tcp addrs, even though TCP still could work without listening on any address. So if tcp silently fails the whole app suddenly is unable to dial anything.

[jacobheun]

We've had some discussion that would allow us to support optional listening addresses in libp2p, and I think that it's reasonable for us to get this implemented relatively soon. The idea relates to https://github.com/libp2p/interface-transport/issues/41, which would support an array of listening addresses.

The idea being, that we could pass additional options to the switch, and subsequently to the transports to determine our level of tolerance on listening. So if configured to be very tolerant, we could end up failing to listening to any addresses but still continue to run.

Some additional context (edited for ease of reading) from the comments around this discussion:

Maybe the transport should have an option for retrying with a basic backoff and jitter. If retrying is not specified and we have 0 servers then an error is passed back. Otherwise, if we fail to listen on any addresses we will do a graceful backoff to retry listening. This could be repeated in the event the server goes down while we're listening.
We could make the retry logic on by default instead of having to specify it.

I think this might be a good option to pass to the switch and let it handle errors passed back by the listeners. We could also set the level of fatal, (no fatal errors, fatal if all transports failed to listen, fatal if one transport failed to listen), so users could get further control. We could do "fatal if all transports failed to listen" by default.
If the transport has retry turned on, it would never return an error, so the switch options wouldn't matter in this case. But with other transports in use that becomes a lot more useful.

[parkan]

@alanshaw @jacobheun the idea of supporting this across a variety of transports is sound, though the semantics of failing to bind a port are pretty different for ws-star vs TCP so the behavior would need to be configurable

however, the issue described in #1619 (and fixed in this PR) is a showstopper bug that prevents multiple prospective users from shipping otherwise working js-ipfs code, and I am having a very hard time explaining to them why we haven't put a fix in place (especially given that we've had this PR up for almost a month) -- I would say this is among the top 3 blockers to js-ipfs adoption, and by far the easiest of those to fix

I could advise them to inject ws-star-multi themselves, but I am concerned that it will again drift out of sync with js-ipfs core and break in a subsequent update

would it be possible to ship this as is, with a note to re-evaluate once we have a comprehensive, configurable solution across libp2p?

[alanshaw]

@mkg20001 please could you also put a comment in the code (as @parkan suggested) in both instances where libp2p-websocket-star-multi is used that this can be removed once libp2p supports optional listening addresses along with a link to the issue.

This'll unblock @parkan and others right now and gives the libp2p guys some breathing room to get it implemented in the mean time.

[mkg20001]

Btw, should I move ws-star-multi to the libp2p org?

[parkan]

thank you @alanshaw + @mkg20001 🙏

[mkg20001]

Added changes, LGTM

[mkg20001]

Note that while ws-star-multi should fix the "server if offline = app crashes" kinds of issues it still leaves the infrastructural issues of overloaded and slow servers untouched. I'd prefer if a switch to stardust would be made, at least until rendezvous & circuit-relay are ready for replacing it.

[alanshaw]

Merging as windows CI failure is unrelated and fixed by #1834

[parkan]

@alanshaw big up for including this in the patch release! https://github.com/ipfs/js-ipfs/releases/tag/v0.34.2