Skip to content

Conversation

@carera
Copy link
Contributor

@carera carera commented Jul 17, 2023

hello, found a bug where redis adapter is not correctly cleaning up/unsubscribing topics when handling socketIO middleware errors:

  • When we call next(new Error('Foo') in a socketIO middleware (as suggested here), socketIO lib calls _cleanup on error here
  • (note that, at this point, redis-adapter is already subscribed to topics in Redis)
  • _cleanup implementation calls leaveAll, which is here and which calls adapter.delAll
  • socketIO adapter interface defines delAll here (it calls this._del in a loop)
  • redis adapter however does not override/extend this method with custom behaviour, which, i think, should clean up Redis subscriptions

This causes leaking subscriptions on Redis servers of clients that are long gone.

Added a draft of what seems to fix the problem, though not sure whether this is the most correct approach, and also I don't know how to test this 🙈.

@carera carera force-pushed the add-delAll-impl branch 4 times, most recently from e79a033 to 0ecebeb Compare July 17, 2023 06:32
@carera carera force-pushed the add-delAll-impl branch from 0ecebeb to fe34526 Compare July 17, 2023 06:33
@carera
Copy link
Contributor Author

carera commented Jul 18, 2023

Realised this is not a correct approach, will discuss in socketio/socket.io#4602 (comment) first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants