chore: throw error no swarm on multiaddrs using websocket-star #3051
Conversation
| try { | ||
| await node.start() | ||
| } catch (err) { | ||
| expect(err).to.exist() |
There was a problem hiding this comment.
Please assert the correct error was thrown.
Better yet, do something like
await expect(node.start()).to.eventually.be.rejectedWith(/regex to use on error message/)or
await expect(node.start()).to.eventually.be.rejected().with.property('code', 'ERR_SOME_CODE')then you don't need the try/catch and the following throw.
| @@ -130,7 +138,6 @@ module.exports = ({ | |||
| apiManager.update(api, () => undefined) | |||
| } catch (err) { | |||
| cancel() | |||
| startPromise.reject(err) | |||
There was a problem hiding this comment.
Why did this get removed? If the user calls node.start() twice, the second promise will never resolve/reject.
I'm not sure how common this is, but I'd like to re-evaluate the use of the api proxy as part of #2762.
There was a problem hiding this comment.
Oh, I did not understand that this was used to restart.
So, what happens with the startPromise.reject(err) is that this promise rejection is not being handled anywhere. I looked closely on this now, and it seems that the ApiManager should take care of handling promise rejections. Is that fair to assume? I am not sure on how to actually do this though.
There was a problem hiding this comment.
It's not used to restart, it's if the user calls node.start() on a node that's already starting up.
There was a problem hiding this comment.
sorry, I understood, but misspelled it: parallel starts
So, how to you think we should go on handling the promise rejections on the APIManager
There was a problem hiding this comment.
I added a catch, but I am not sure this is the best approach here. Let me know
vasco-santos
force-pushed
the
chore/thrown-error-on-swarm-multiaddrs-using-websocket-star
branch
from
0a92322
to
f8d1c80
Compare
achingbrain
deleted the
chore/thrown-error-on-swarm-multiaddrs-using-websocket-star
branch
On
js-ipfs@0.41, we removed thewebsocket-starmodule from the libp2p default configuration for browser nodes injs-ipfs. We removed it because this module was not refactored during #1670, since the libp2p goal is to sunset the star protocols and we preferred to use the time refactoringwebsocket-starinto improving the browser support.In the refactor, the
webrtc-starmodule was refactored so that browser users could discover other peers in the network. In addition,circuit-relaymay be used for establishing connections between browser nodes usingwebsockets.However, this migration has not been smooth for
js-ipfsusers. Users previously usingwebsocket-starswarm addresses, did not have enough visibility of this change, since the removal ofwebsocket-starwas transparent for them and on the defaultjs-ipfsrelease and not technically a programmable breaking change.With the above in mind, once
js-ipfsusers updated they started getting errors fromjs-libp2pin scenarios where they were providing onlywebsocket-starswarm addresses. Whenjs-libp2preceives 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, which was also mentioned for some other cases earlier.After syncing about this issue, we decided to temporarily throw an error when
js-ipfsreceiveswebsocket-starmultiaddrs. This aims to help users who still need to migrate to identify the problem faster.