Skip to content

Redis adapter is not correctly cleaning up/unsubscribing topics when handling socketIO middleware errors #508

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

Closed
wants to merge 2 commits into from

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