lukeed / sockette

The cutest little WebSocket wrapper! 🧦
MIT License
2.45k stars 81 forks source link

Reconnections are not being queued. #30

Closed sli closed 5 years ago

sli commented 6 years ago

Basically the opposite of #23, onmaximum triggers immediately. I have my Sockette instances configured to attempt to connect 5 times before giving up. If my backend server is not running, it attempts one connection, fails, and gives up. I can confirm that if the server is running and it successfully connects, then the server is shut down, it will attempt one reconnect before failing and giving up again.

After inspecting the events, I'm not getting a 1000 or 1005. In fact, the only event code I'm able to actually see is 1006, which should trigger a reconnect according to the docs. What's happening is a connection attempt is made, fails, then calls onerror and onclose (which is expected), then straight to onmaximum.

image

Here's the code currently (with logging stuff removed):

this._ws = new Sockette(url, {
  timeout: 60000,
  maxAttempts: 5,
  onopen: () => {
    console.log('[Chat] Connection successful.')
    this.setState({ connected: true }, () => {
      const payload = {
        channel: 'chat-login',
        payload: { from: this.state.myself }
      }
      this._ws.json(payload)
    })
  },
  onclose: () => console.log('[Chat] Connection closed.'),
  onmessage: ({ data }) => this._onMessage(JSON.parse(data)),
  onmaximum: () => console.error('[Chat] Too many connection attempts. Giving up.'),
  onreconnect: () => console.log('[Chat] Reconnecting...'),
  onerror: () => {
    console.error('[Chat] Disconnected.')
    this.setState({ connected: false })
  }
})

Shouldn't it attempt to connect four more times, or have I misread or missed something in the documentation?

sli commented 6 years ago

I've figured out that I'm not getting ECONNREFUSED from the connection attempt, so that is clearly the issue. Not really a problem when things go to production and the backend is long-running, but in development that's going to be a hassle.

I suppose I could just manually trigger a reconnect to workaround this for the moment. E: Nevermind, it doesn't attempt to reconnect even when triggering it manually, as it still thinks it's hit the maximum number of attempts already.

E2: After further tinkering (read: editing dependencies manually) and manually attempting a reconnect, it seems that the current number of attempts is undefined inside the library's own onreconnect handler, then becomes NaN. Left is num, right is maxAttempts:

image

This makes sense, as the connection is never opened, so onopen is not called, and so num is never initialized to 0. Can confirm that initializing num to 0 allows for this workflow.

lukeed commented 6 years ago

IMO, this makes sense, unless I'm missing something here?

You can only re-connect after having connected in the first place.

Initializing num=0 at the start would fix this for you, but I feel like it's encouraging lack of error handling. You should definitely be handling ECONNREFUSED & the like manually, or kill the process. That's what the hooks are intended for, really.

It'd be much worse to (potentially) indefinitely ping your server that's shut down or choking.

What do you think?

lukeed commented 6 years ago

Hey @sli – it looks like you edited your response after I replied, so I never saw that last portion (and was never notified)

Are you saying that the fix was to declare num=0 at the top of the constructor?