mrjoes / tornadio2

Python socket.io server implementation on top of Tornado framework
Other
523 stars 118 forks source link

Multiple connects to same endpoint result in connection leakage #51

Closed cpisto closed 12 years ago

cpisto commented 12 years ago

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?

mrjoes commented 12 years ago

I think that's proper fix.

Can you create pull request?

Thanks.

cpisto commented 12 years ago

Sure, have it for you shortly.

cpisto commented 12 years ago

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?

cpisto commented 12 years ago

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 ;)

cpisto commented 12 years ago

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.

cpisto commented 12 years ago

OK, proper pull request submitted.