Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

WebSocket event listeners not cleaned up after WebSocket is closed #581

Closed
tretne opened this issue Jun 14, 2019 · 1 comment · Fixed by #615
Closed

WebSocket event listeners not cleaned up after WebSocket is closed #581

tretne opened this issue Jun 14, 2019 · 1 comment · Fixed by #615

Comments

@tretne
Copy link
Contributor

tretne commented Jun 14, 2019

After restarting the backend, our client ends up in a continuous reconnection loop.

Our backend is using gqlgen for its GraphQL API. The current version implements keep alive, but only sends the "KA" messages on a regular basis after the connection has been set up, default is every 25 seconds. So there is no "KA" directly after the "ACK" only after 25 seconds.

The SubscriptionClient in our client is configured to with reconnect: true and to use an extended ws implementation. ws is only extended so we can pass an Agent with certificates to the constructor of ws, there are no other changes to ws logic.

The flow causing our issue as far as I have analyzed it is close to the following:

  1. The client, i.e. SubscriptionClient, connects to the backend, that is running on the same computer, by creating a new WebSocket, ws, and adds an event listener to the WebSockets onclose event (as well as other events)
  2. Client receives "ACK"
  3. After 25 seconds the client receives the first "KA" and starts the CheckConnectionInterval that is triggered every 30 seconds from now on
  4. A about 10 seconds after the first "KA" I restart the backend
  5. The WebSocket detects this and emits a close event that triggers the onclose event handler
  6. The client calls close on the WebSocket and sets it reference to null, but it does not clear the event listeners
  7. The client reconnects to the backend, creates and sets up a new WebSocket. Let's call this one "B"
  8. Client receives "ACK"
  9. Before the "KA" message is received the client triggers the check connection logic, that detects that it has not received a "KA" message and thus closes WebSocket "B" as in 6.
  10. WebSocket "B" initiates the closing handshake and sets a timeout that destoys the socket after 30 seconds if the closing handshake fails, i.e. does not get a close event from the socket. And this handshake fails, but I have not figured out why or how. The onclose is not triggered yet.
  11. The client reconnects to the backend, creates and sets up yet another new WebSocket. Let's call it "C".
  12. Client receives "ACK"
  13. For the sake of simplicity here let's assume "KA" is received so the check connection interval does not close the connection.
  14. However the WebSocket "B"'s closing handshake timeout now triggers and destroys it's socket
  15. Since the client has not cleaned up the event listeners from WebSocket "B", WebSocket "B" will now trigger the onclose and thus the client will now close WebSocket "C"

This combination of late "KA" messages, the failure of the WebSocket's closing handshake and the event listeners not being properly cleaned up causes our client to end up in a continuous reconnection loop.

client deps:

backend deps:

@skimi
Copy link

skimi commented Jun 23, 2020

I'm reproducing the loop with step 4 being client loses internet and @tretne PR fix works. I'm monkey patching the client for now :/. It needs to be merged asap

const originalClose = SubscriptionClient.prototype.close;
SubscriptionClient.prototype.close = function close(...args) {
  if (this.client) {
    this.client.onopen = null;
    this.client.onclose = null;
    this.client.onerror = null;
    this.client.onmessage = null;
  }
  originalClose.apply(this, args);
};

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants