Multiple connects to same endpoint result in connection leakage #51

Closed
cpisto opened this Issue Jul 6, 2012 · 6 comments

Comments

Projects
None yet
2 participants
Contributor

cpisto commented Jul 6, 2012

In the course of writing my own socket.io client, I discovered that a "misbehaving" client that connects to the same endpoint more than once causes any existing connection to that endpoint to never be disconnected in the server (this may be specific to the multiplexed example as it keeps an internal reference to connection and may be interacting poorly with expected garbage collection behavior).

My fix was this:

diff --git a/tornadio2/session.py b/tornadio2/session.py
index c55b5c7..48aadc9 100644
--- a/tornadio2/session.py
+++ b/tornadio2/session.py
@@ -296,6 +296,9 @@ class Session(sessioncontainer.SessionBase):
             return

         conn = conn_class(self, endpoint)
+        old_conn = self.endpoints.get(endpoint, None)
+        if old_conn is not None:
+            self.disconnect_endpoint(endpoint)
         self.endpoints[endpoint] = conn

         self.send_message(proto.connect(endpoint))

Any thoughts on what the proper fix should be?

Owner

mrjoes commented Jul 6, 2012

I think that's proper fix.

Can you create pull request?

Thanks.

Contributor

cpisto commented Jul 6, 2012

Sure, have it for you shortly.

Contributor

cpisto commented Jul 6, 2012

I submitted a pull for this solution, but there are some alternatives that may be better?

An error could be returned when connecting to an already connected endpoint, or the later connect could be silently ignored (and a new connection not created in the server)..

Disconnecting and and closing the existing connection works, but what if there was some state in there that should have been kept? etc..

Thoughts?

Contributor

cpisto commented Jul 6, 2012

Somewhat unfortunate the socket.io spec doesn't specify what should happen in this situation. I guess i can setup the actual socket.io server and see what it does ;)

Contributor

cpisto commented Jul 6, 2012

So, near as I can tell according to ~ line 411 of https://github.com/LearnBoost/socket.io/blob/master/lib/manager.js the official socket.io server silently ignores connects to an already connected endpoint, I will create a new pull request with a fix in that vein.

Contributor

cpisto commented Jul 6, 2012

OK, proper pull request submitted.

@mrjoes mrjoes added a commit that referenced this issue Jul 8, 2012

@mrjoes mrjoes Merge pull request #53 from cpisto/master
Proper fix for issue #51
92a0d24

mrjoes closed this Jul 8, 2012

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