fix: do not close connections when sigserver goes away

[achingbrain]

Tweaks the logic around server errors so that if we lose our connection to the sigserver, we now let socket.io's reconnection logic handle trying to reconnect instead of emitting an error that causes everything to be torn down.

Fixes: #453

[abetaev]

as a user i would prefer the following:

/* make transport instance "startable" */
class WebRTCStar implements Transport, Initializable, Startable {
  /* ... */
  start: () => this.sigServers.values()
      .forEach(sigServer => sigServer.start()),
  stop: () => this.sigServers.values()
      .forEach(sigServer => sigServer.stop())
  /* ... */
}
/* make SigServer instances "startable" */
class SigServer extends EventEmitter<SignalServerServerEvents> implements SignalServer, Startable {
  /* ... */
  start() {
    this.socket = connect(this.signallingUrl, this.sioOptions);
    /* ... */
  }
  stop() {
    /* ... */
    this.socket.close();
    /* ... */
  }
} 

example use case:
pwa is loaded from browser's storage, at the moment app is started one of signalling servers is unavailable

• in current implementation server is excluded from the list, i need to handle it and implement retry for that server
• in suggested implementation the connection attempt will be retried by socket.io oob

i don't see any significant semantic difference between the first and other values, i think in general it would be easier to handle different scenarios without this special case.

[achingbrain]

example use case

I could be misreading, but this sounds like it's solving a slightly different problem - e.g. that the list of sigservers is a bit more speculative - they could be up, they could be down, and we should try to connect in future if they appear.

It sounds like it could be done in a separate PR to this one?

[abetaev]

i was trying to understand the purpose of previouslyConnected when came to this suggestion.

why is it a specific case to handle first failed connection as transport failure?

[achingbrain]

why is it a specific case to handle first failed connection as transport failure?

It's to give the user fast feedback that their configuration isn't correct. A similar thing happens with, for example, the tcp transport trying to listen on a port that's in use - it'll throw on startup rather than waiting to see if the port becomes available at some point in the future.

Trying to connect to remote signalling servers as part of node startup may be a little different since they aren't necessarily under your control, we could add an option to allow tolerance for failures around this, for example. We may not wish to have this enabled by default though as it can become hard to reason about the correctness of your config on startup otherwise.

[abetaev]

if i understand correctly, there are some agreements how transports are supposed to behave.
i did not see anything like that in the docs.

i don't insist on changing anything then, here are my thoughts though:

my common sense says that in bigger/complex systems (which libp2p undoubtedly is) softer borders (i.e. to "success" and "failure" add "recoverable failure") generally mean smoother operation (automated recovery).
user is still able to know about transport failures by subscribing to corresponding events.

transport failure may be either configuration error (hard border) or connectivity issue (soft border), but previouslyConnected logic makes it something in between.

from the perspective of other components, transport is the only component whose recoverable failure (connectivity issue) will fail the system startup. e.g. if i create streams with broken multiplexer it fails only calls where i try to create new stream, same dht.

connectivity issue is recoverable because it can be retried and may eventually succeed

[mpetrunic]

maybe out of scope for this PR but shouldn't we remove all event listeners on close?

[achingbrain]

We do this when the listener is closed.

[github-actions]

🎉 This PR is included in version @libp2p/webrtc-star-signalling-server-v2.0.4 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version @libp2p/webrtc-star-v3.0.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀