Fix / Verify the connect event and recv_connect on the namespace #9

Closed
sontek opened this Issue Mar 14, 2012 · 19 comments

Comments

Projects
None yet
5 participants
@sontek
Collaborator

sontek commented Mar 14, 2012

Currently connect isn't firing, is this the expected behavior?

@abourget

This comment has been minimized.

Show comment
Hide comment
@abourget

abourget Apr 17, 2012

Owner

Is this fixed ?

Owner

abourget commented Apr 17, 2012

Is this fixed ?

@sontek

This comment has been minimized.

Show comment
Hide comment
@sontek

sontek Apr 18, 2012

Collaborator

I will verify tonight

Collaborator

sontek commented Apr 18, 2012

I will verify tonight

@dswarbrick

This comment has been minimized.

Show comment
Hide comment
@dswarbrick

dswarbrick Apr 22, 2012

Contributor

I've found that BaseNamespace.recv_connect() doesn't get called for the global namespace, but does get called on specific endpoints. Conversely, I also noticed that the "connect" event never fires on the client if an endpoint name is used, but does fire if no endpoint name is specified.

Contributor

dswarbrick commented Apr 22, 2012

I've found that BaseNamespace.recv_connect() doesn't get called for the global namespace, but does get called on specific endpoints. Conversely, I also noticed that the "connect" event never fires on the client if an endpoint name is used, but does fire if no endpoint name is specified.

@eoranged

This comment has been minimized.

Show comment
Hide comment
@eoranged

eoranged May 30, 2012

Contributor

Is it any progress on the issue?

Contributor

eoranged commented May 30, 2012

Is it any progress on the issue?

@eoranged

This comment has been minimized.

Show comment
Hide comment
@eoranged

eoranged May 30, 2012

Contributor

Tested master branch: both recv_connect and recv_disconnect doesn't work for me.
I use code like the following and see only "called" message, but no "connected" or "disconnected":

class MainNamespace(namespace.BaseNamespace):

    def on_my_init(self, data):
        print "called"

    def recv_connect(self):
        print "connected"

    def recv_disconnect(self):
        print "disconnected"

And attaching using this namespace in request handler this way (stolen from cross_origin example, request is the Werkzeug's Request object):

socketio_manage(request.environ, {'': MainNamespace}, request=request)

And, yes. I'm connecting to server, calling my_init method, waiting a bit and then closing browser tab.

Contributor

eoranged commented May 30, 2012

Tested master branch: both recv_connect and recv_disconnect doesn't work for me.
I use code like the following and see only "called" message, but no "connected" or "disconnected":

class MainNamespace(namespace.BaseNamespace):

    def on_my_init(self, data):
        print "called"

    def recv_connect(self):
        print "connected"

    def recv_disconnect(self):
        print "disconnected"

And attaching using this namespace in request handler this way (stolen from cross_origin example, request is the Werkzeug's Request object):

socketio_manage(request.environ, {'': MainNamespace}, request=request)

And, yes. I'm connecting to server, calling my_init method, waiting a bit and then closing browser tab.

@abourget

This comment has been minimized.

Show comment
Hide comment
@abourget

abourget May 30, 2012

Owner

Yeah, there is ambiguity here. The "recv_connect" call is received only when we receive a CONNECT packet.. On the "global namespace", the empty string that is, no such packet is sent.. because the opening of the connection is itself the "connect" event. Instead, on the global namespace, we should implement the "connect(self)" method.

Same thing goes for disconnect. There is a disconnect packet that can be sent to close an endpoint (or namespace), but it is not sent for the global namespace.

So there might be a solution: we could remove recv_connect(self), and recv_disconnect(self), and call in both cases (even though they are different events) "connect(self, type)" and "disconnect(self, type)".. where 'type' would be what triggered the event. I know it's not very nice.. or maybe we should just update the documentation ? Or maybe we should discourage the usage of the global namespace, as it is there mostly for backwards compatibility.

What do you think ?

Owner

abourget commented May 30, 2012

Yeah, there is ambiguity here. The "recv_connect" call is received only when we receive a CONNECT packet.. On the "global namespace", the empty string that is, no such packet is sent.. because the opening of the connection is itself the "connect" event. Instead, on the global namespace, we should implement the "connect(self)" method.

Same thing goes for disconnect. There is a disconnect packet that can be sent to close an endpoint (or namespace), but it is not sent for the global namespace.

So there might be a solution: we could remove recv_connect(self), and recv_disconnect(self), and call in both cases (even though they are different events) "connect(self, type)" and "disconnect(self, type)".. where 'type' would be what triggered the event. I know it's not very nice.. or maybe we should just update the documentation ? Or maybe we should discourage the usage of the global namespace, as it is there mostly for backwards compatibility.

What do you think ?

@eoranged

This comment has been minimized.

Show comment
Hide comment
@eoranged

eoranged May 30, 2012

Contributor

30.05.2012, 22:35, "Alexandre Bourget" reply@reply.github.com:

Yeah, there is ambiguity here.  The "recv_connect" call is received only when we receive a CONNECT packet..  On the "global namespace", the empty string that is, no such packet is sent.. because the opening of the connection is itself the "connect" event.  Instead, on the global namespace, we should implement the "connect(self)" method.

Same thing goes for disconnect.  There is a disconnect packet that can be sent to close an endpoint (or namespace), but it is not sent for the global namespace.

Thanks, I understand the problem now.

So there might be a solution: we could remove recv_connect(self), and recv_disconnect(self), and call in both cases (even though they are different events) "connect(self, type)" and "disconnect(self, type)".. where 'type' would be what triggered the event.  I know it's not very nice.. or maybe we should just update the documentation ?  Or maybe we should discourage the usage of the global namespace, as it is there mostly for backwards compatibility.

What do you think ?

I wonder how does it work with node.js implementation (it actually works!). I suppose the best way is to choose the same or similar solution.

Handlers "connect(self, type)" and "disconnect(self, type)" looks like a spike for me, because it negates all advantages of RPC.

Also, It will be nice to mention in documentation that recv_* functions doesn't work for global namespace.


Reply to this email directly or view it on GitHub:
#9 (comment)

Best regards,
Vladimir Protasov.

Contributor

eoranged commented May 30, 2012

30.05.2012, 22:35, "Alexandre Bourget" reply@reply.github.com:

Yeah, there is ambiguity here.  The "recv_connect" call is received only when we receive a CONNECT packet..  On the "global namespace", the empty string that is, no such packet is sent.. because the opening of the connection is itself the "connect" event.  Instead, on the global namespace, we should implement the "connect(self)" method.

Same thing goes for disconnect.  There is a disconnect packet that can be sent to close an endpoint (or namespace), but it is not sent for the global namespace.

Thanks, I understand the problem now.

So there might be a solution: we could remove recv_connect(self), and recv_disconnect(self), and call in both cases (even though they are different events) "connect(self, type)" and "disconnect(self, type)".. where 'type' would be what triggered the event.  I know it's not very nice.. or maybe we should just update the documentation ?  Or maybe we should discourage the usage of the global namespace, as it is there mostly for backwards compatibility.

What do you think ?

I wonder how does it work with node.js implementation (it actually works!). I suppose the best way is to choose the same or similar solution.

Handlers "connect(self, type)" and "disconnect(self, type)" looks like a spike for me, because it negates all advantages of RPC.

Also, It will be nice to mention in documentation that recv_* functions doesn't work for global namespace.


Reply to this email directly or view it on GitHub:
#9 (comment)

Best regards,
Vladimir Protasov.

@eoranged

This comment has been minimized.

Show comment
Hide comment
@eoranged

eoranged May 30, 2012

Contributor

Okay, I've got deeper into docs and now I see that global namespace is supported only in 0.6.

So, I think it will be enough to mention in documentation for recv_* functions that it doesn't supported for global namespace.

Best regards,
Vladimir Protasov.

Contributor

eoranged commented May 30, 2012

Okay, I've got deeper into docs and now I see that global namespace is supported only in 0.6.

So, I think it will be enough to mention in documentation for recv_* functions that it doesn't supported for global namespace.

Best regards,
Vladimir Protasov.

@dswarbrick

This comment has been minimized.

Show comment
Hide comment
@dswarbrick

dswarbrick May 30, 2012

Contributor

It seems that the Python code and JS code are at odds with one another. For example, I have some JS code as follows:

var global_socket = io.connect();
var socket = io.connect("/console");

// "connecting" event fires for specific namespaces
socket.on("connecting", function (transport_type) {
    $("#state").text("connecting to " + transport_type + "...");
});

// Note global_socket, as "connect" event does not fire for specific namespaces
global_socket.on("connect", function () {
    $("#state").text("connected");
});

But then on the Python side of things:

class ConsoleNamespace(BaseNamespace):

    def recv_connect(self):
        # Spawn custom method upon Socket.IO client-namespace connect
        self.spawn(self.listener)

    def listener(self):
        while True:
            # Do custom background stuff
            gevent.sleep()

The recv_connect() is called upon 'socket = io.connect("/console")' (routed to the ConsoleNamespace by socketio_manage()), which spawns my custom class method (which lurks in the background, monitoring a TCP socket and occasionally emit()'ing packets to the Socket.IO client connection).

However, if I didn't have the explicit no-namespace "global_socket = io.connect()" in the JS, then I'm not able to determine on the client when it has successfully connected.

I'm open to any suggestions on how to more gracefully handle this.

Contributor

dswarbrick commented May 30, 2012

It seems that the Python code and JS code are at odds with one another. For example, I have some JS code as follows:

var global_socket = io.connect();
var socket = io.connect("/console");

// "connecting" event fires for specific namespaces
socket.on("connecting", function (transport_type) {
    $("#state").text("connecting to " + transport_type + "...");
});

// Note global_socket, as "connect" event does not fire for specific namespaces
global_socket.on("connect", function () {
    $("#state").text("connected");
});

But then on the Python side of things:

class ConsoleNamespace(BaseNamespace):

    def recv_connect(self):
        # Spawn custom method upon Socket.IO client-namespace connect
        self.spawn(self.listener)

    def listener(self):
        while True:
            # Do custom background stuff
            gevent.sleep()

The recv_connect() is called upon 'socket = io.connect("/console")' (routed to the ConsoleNamespace by socketio_manage()), which spawns my custom class method (which lurks in the background, monitoring a TCP socket and occasionally emit()'ing packets to the Socket.IO client connection).

However, if I didn't have the explicit no-namespace "global_socket = io.connect()" in the JS, then I'm not able to determine on the client when it has successfully connected.

I'm open to any suggestions on how to more gracefully handle this.

@sontek

This comment has been minimized.

Show comment
Hide comment
@sontek

sontek Jun 21, 2012

Collaborator

We need to handle this how the node.js implementation does. I assume since their client is only firing the packet on named namespaces, their server probably doesn't do anything different than we are currently doing.

We should make a node.js sample app that tests this.

Collaborator

sontek commented Jun 21, 2012

We need to handle this how the node.js implementation does. I assume since their client is only firing the packet on named namespaces, their server probably doesn't do anything different than we are currently doing.

We should make a node.js sample app that tests this.

@dswarbrick

This comment has been minimized.

Show comment
Hide comment
@dswarbrick

dswarbrick Jun 26, 2012

Contributor

Whilst searching for answers about this, I stumbled across this page http://sideeffect.kr:8005/
If you scroll down a little way to the namespace example, it looks like node.js will send a "connect" event when using a namespace... which would seem to indicate that gevent-socketio needs to do that also.

Contributor

dswarbrick commented Jun 26, 2012

Whilst searching for answers about this, I stumbled across this page http://sideeffect.kr:8005/
If you scroll down a little way to the namespace example, it looks like node.js will send a "connect" event when using a namespace... which would seem to indicate that gevent-socketio needs to do that also.

@abourget

This comment has been minimized.

Show comment
Hide comment
@abourget

abourget Jun 27, 2012

Owner

Oh, so when we create the namespace, we should ourselves send a 'connect'
packet to the browser ?

On Tue, Jun 26, 2012 at 5:16 PM, Daniel Swarbrick <
reply@reply.github.com

wrote:

Whilst searching for answers about this, I stumbled across this page
http://sideeffect.kr:8005/
If you scroll down a little way to the namespace example, it looks like
node.js will send a "connect" event when using a namespace... which would
seem to indicate that gevent-socketio needs to do that also.


Reply to this email directly or view it on GitHub:
#9 (comment)

Owner

abourget commented Jun 27, 2012

Oh, so when we create the namespace, we should ourselves send a 'connect'
packet to the browser ?

On Tue, Jun 26, 2012 at 5:16 PM, Daniel Swarbrick <
reply@reply.github.com

wrote:

Whilst searching for answers about this, I stumbled across this page
http://sideeffect.kr:8005/
If you scroll down a little way to the namespace example, it looks like
node.js will send a "connect" event when using a namespace... which would
seem to indicate that gevent-socketio needs to do that also.


Reply to this email directly or view it on GitHub:
#9 (comment)

@sontek

This comment has been minimized.

Show comment
Hide comment
@sontek

sontek Jul 4, 2012

Collaborator

fixed on 0df3e5c

Collaborator

sontek commented Jul 4, 2012

fixed on 0df3e5c

@sontek sontek closed this Jul 4, 2012

@dswarbrick

This comment has been minimized.

Show comment
Hide comment
@dswarbrick

dswarbrick Jul 5, 2012

Contributor

Sorry, I think this is still not working correctly. The "connect" event still does not fire on the client when connecting to a namespaced endpoint. For example:

socket = io.connect("/foo");

// This never fires
socket.on("connect", function () {
    alert("connected");
});

I don't want to fire some user-connect event, when somebody joins a chat room. I want to know when the socket.io endpoint has successfully connected. This (I thought) is the purpose of the "connect" event firing.

I can get a "connect" event to fire if I use a non-namespaced endpoint.

Contributor

dswarbrick commented Jul 5, 2012

Sorry, I think this is still not working correctly. The "connect" event still does not fire on the client when connecting to a namespaced endpoint. For example:

socket = io.connect("/foo");

// This never fires
socket.on("connect", function () {
    alert("connected");
});

I don't want to fire some user-connect event, when somebody joins a chat room. I want to know when the socket.io endpoint has successfully connected. This (I thought) is the purpose of the "connect" event firing.

I can get a "connect" event to fire if I use a non-namespaced endpoint.

@dswarbrick

This comment has been minimized.

Show comment
Hide comment
@dswarbrick

dswarbrick Jul 5, 2012

Contributor

Also, I noticed while testing, that if I add recv_disconnect() Python method to my namespace, it is being called repeatedly if the page has been refreshed... I'm not sure if this expected behaviour or not.

Contributor

dswarbrick commented Jul 5, 2012

Also, I noticed while testing, that if I add recv_disconnect() Python method to my namespace, it is being called repeatedly if the page has been refreshed... I'm not sure if this expected behaviour or not.

@sontek

This comment has been minimized.

Show comment
Hide comment
@sontek

sontek Jul 5, 2012

Collaborator

If you define a recv_disconnect() you need to call self.disconnect() yourself. (Thinking about ways to make that cleaner), that is why you are getting the repeat.

I'll fix the connect so that it fires properly.

Collaborator

sontek commented Jul 5, 2012

If you define a recv_disconnect() you need to call self.disconnect() yourself. (Thinking about ways to make that cleaner), that is why you are getting the repeat.

I'll fix the connect so that it fires properly.

@sontek sontek reopened this Jul 5, 2012

@sontek

This comment has been minimized.

Show comment
Hide comment
@sontek

sontek Jul 5, 2012

Collaborator

Fixed in e039720

Collaborator

sontek commented Jul 5, 2012

Fixed in e039720

@dswarbrick

This comment has been minimized.

Show comment
Hide comment
@dswarbrick

dswarbrick Jul 5, 2012

Contributor

@sontek Thanks, that finally fixed it :)

Contributor

dswarbrick commented Jul 5, 2012

@sontek Thanks, that finally fixed it :)

@vivekhub

This comment has been minimized.

Show comment
Hide comment
@vivekhub

vivekhub Jul 5, 2012

Contributor

@dswarbrick that is nice.. our kludge goes away as well

Contributor

vivekhub commented Jul 5, 2012

@dswarbrick that is nice.. our kludge goes away as well

@sontek sontek closed this Jul 5, 2012

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