Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

High redis latency with large amount of dynamic namespaces #480

Closed
stevebaum23 opened this issue Jan 6, 2023 · 7 comments · Fixed by #485
Closed

High redis latency with large amount of dynamic namespaces #480

stevebaum23 opened this issue Jan 6, 2023 · 7 comments · Fixed by #485
Labels
question Further information is requested

Comments

@stevebaum23
Copy link
Contributor

We are using dynamic namespaces for our app. As users interact with different pages it creates more dynamic namespaces. While monitoring the app we noticed that the redis latency increased slowly over time and eventually got to a point where it was in the hours.

While troubleshooting the issue, we discovered the redis engine cpu got to 100% and then the latency really started to spike. We believe this is due to the high number redis pattern subscriptions (each namespace will do a pattern subscribe).

Looking through the source code it doesn't appear that namespaces ever unsubscribe when there are no sockets connected to them, so over time the number of pattern subscriptions continues to grow, until it gets to a point where the cpu can't keep up as you publish messages.

Are we using dynamic namespaces incorrectly? Are we not supposed to be using a large amount of dynamic namespaces?

@darrachequesne
Copy link
Member

Yes you are right, currently the adapter is not really suitable for a large amount of dynamic namespaces, as the number of subscriptions grows linearly with the number of namespaces (and the subscriptions are not cancelled when the namespace is empty).

For this use case, I think we could provide a mode with a single subscription for all namespaces, in order to relieve the burden from the redis server.

@darrachequesne darrachequesne added the question Further information is requested label Jan 7, 2023
@stevebaum23
Copy link
Contributor Author

Thanks for the quick response @darrachequesne!

Would this single subscription mode open up the possibility that a message might be broadcast to all namespaces inadvertently? Part of our reasoning for using dynamic namespaces is that they appear more secure than rooms, and there is less of a chance that a message would be sent to all namespaces. There was a bug in a previous version of socket.io that allowed a message to be sent to all rooms if you used await within an emit. This is one of the reasons we were looking at using dynamic namespaces instead of rooms.

We came up with a solution that listens for a socket disconnect event. It then checks the namespace the socket was connect to, and if it was the last socket on that namespace, it will unsubscribe in redis, and remove the namespace from the internal list (so that the subscription process will happen if someone connects to the namespace again). This is what that solution looks like:

socket.on('disconnect', () => {
  if (socket.nsp.sockets.size <= 0) {
    // Channels that the redis adapter subscribes to when the dynamic namespace is created
    const channel = `${CHANNEL_PREFIX}#${socket.nsp.name}#`
    const requestChannel = `${CHANNEL_PREFIX}-request#${socket.nsp.name}#`
    const responseChannel = `${CHANNEL_PREFIX}-response#${socket.nsp.name}#`
    const specificResponseChannel = `${responseChannel}${socket.nsp.adapter.uid}#`

    // Unsub from the redis channels
    socket.nsp.adapter.subClient.punsubscribe(channel + '*')
    socket.nsp.adapter.subClient.unsubscribe([
      requestChannel,
      responseChannel,
      specificResponseChannel
    ])

    // Delete the namespace so the adapter will subscribe again when a new client connects to it
    socketIOServer._nsps.delete(socket.nsp.name)
  }
})

We have tested this solution in our stage environment under heavy load and it appears to work. Client are unsubscribed properly and the redis cpu load stays low. However, the solution isn't great for a couple of reasons:

  1. It requires knowing what channels the redis adapter susbcribes to when a namespace is created. If these channels changed in the future, the code would not unsubscribe correctly.
  2. It requires accessing a private variable in socket io (_nsps) to remove a namespace. I couldn't find a public function that would let me remove a created namespace.

Would it make sense to add a server option in socket.io to do the following when a socket disconnects from a namespace?

  1. Call namespace.adapter.close(). For redis adapter this could do the unsubscribe/punsubscribe
  2. Remove the namespace from the internal list

This server option would be false by default, so the behavior matches what currently happens (no unsubscribes, namespaces never get cleaned up).

How does this solution compare to the single subscription mode solution? I know there are other adapters, so I'm not sure how this would affect those.

Thanks again for your feedback on this problem.

@darrachequesne
Copy link
Member

Would it make sense to add a server option in socket.io to do the following when a socket disconnects from a namespace?

  1. Call namespace.adapter.close(). For redis adapter this could do the unsubscribe/punsubscribe
  2. Remove the namespace from the internal list

Yes, that sounds reasonable 👍

Regarding the other adapters:

  • the MongoDB adapter uses a single change stream for all namespaces, and route the messages to the right adapter based on the namespace which is included in the MongoDB document. Each namespace emits a heartbeat though, in order to track the number of servers in the cluster, so it would benefit from this option with lots of dynamic namespaces

Reference: https://github.com/socketio/socket.io-mongo-adapter/blob/4dc9f3ff34e47919937212561d3ccebbbaf9b2cb/lib/index.ts#L130-L142

  • similarly, the Postgres adapter uses a single connection for all namespaces, but one LISTEN/NOTIFY channel per namespace, so it would benefit from this option with lots of dynamic namespaces

Reference: https://github.com/socketio/socket.io-postgres-adapter/blob/4190f4b571cfe29c3db70c68a8250817251effd0/lib/index.ts#L159-L165

@stevebaum23
Copy link
Contributor Author

I've spent this week looking at how to handle these two solutions. I've pushed changes to my forks to handle the "cleanup empty namespaces" solution.

Does this look like the right approach?

socket.io changes
redis adapter changes

I also have a solution for the "single subscription mode", but it needs a little more tweaking. I can post a link to those changes early next week. I'm not sure which one you would prefer, so that's why I haven't put up a formal PR in this repo.

@darrachequesne
Copy link
Member

@stevebaum23 looks good to me, thanks a lot ❤️ could you please open a PR with the changes?

@stevebaum23
Copy link
Contributor Author

I have added a PR for the socket.io changes. I will be putting the PR up for the redis adapter soon.

darrachequesne pushed a commit to socketio/socket.io that referenced this issue Jan 23, 2023
This commit adds a new option, "cleanupEmptyChildNamespaces". With this
option enabled (disabled by default), when a socket disconnects from a
dynamic namespace and if there are no other sockets connected to it
then the namespace will be cleaned up and its adapter will be closed.

Note: the namespace can be connected to later (it will be recreated)

Related: socketio/socket.io-redis-adapter#480
@stevebaum23
Copy link
Contributor Author

I've created the redis adapter PR here: #485

darrachequesne pushed a commit that referenced this issue Feb 8, 2023
When the close function is called it will (p)unsubscribe from the
channels it (p)subscribed to in the constructor.

Related:

- #480
- socketio/socket.io@5d9220b
haneenmahd pushed a commit to haneenmahd/socket.io that referenced this issue Apr 15, 2023
This commit adds a new option, "cleanupEmptyChildNamespaces". With this
option enabled (disabled by default), when a socket disconnects from a
dynamic namespace and if there are no other sockets connected to it
then the namespace will be cleaned up and its adapter will be closed.

Note: the namespace can be connected to later (it will be recreated)

Related: socketio/socket.io-redis-adapter#480
dzad pushed a commit to dzad/socket.io that referenced this issue May 29, 2023
This commit adds a new option, "cleanupEmptyChildNamespaces". With this
option enabled (disabled by default), when a socket disconnects from a
dynamic namespace and if there are no other sockets connected to it
then the namespace will be cleaned up and its adapter will be closed.

Note: the namespace can be connected to later (it will be recreated)

Related: socketio/socket.io-redis-adapter#480
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants