-
Notifications
You must be signed in to change notification settings - Fork 12
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
Ensure rejected connections are not stored - which causes memory leaks #34
Ensure rejected connections are not stored - which causes memory leaks #34
Conversation
Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
- to avoid memory - also some minor syntax cleanup Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing stuff! Thanks so much for taking this on. It's a pretty big PR, so I want to make sure I really go over it. I left a few suggestions, but only real change I think is to just leave the version alone for now. Otherwise I'm open to your input on the other things.
Signed-off-by: Marc Jeffrey <mjeffrey18@gmail.com>
@@ -1,84 +1,54 @@ | |||
require "uuid" | |||
|
|||
module Cable | |||
class Connection | |||
abstract class Connection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense. What do you think @fernandes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, totally, specially because this class should never get instantiated itself.. way better! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I dig it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's awesome! thank you @mjeffrey18 !
Background
I found a memory leak in production due to unathorised connections still being stored on the
Cable::Server#connections
hash.Implementation
reject_connection!
helper methods to connection class and use this to ensure rejected connections are never added to theCable::Server#connections
hashidentified_by(field)
can be used with a different key other thanidentifier
while still providing access viaconnection.identifier
connect.reject(channel : Cable::Channel)
to instead supportconnection.reject(payload : Cable::Payload)
keeping it consistent with the other methods likesubscribe, unsubscribe and message