Skip to content
This repository was archived by the owner on Sep 6, 2020. It is now read-only.

Conversation

@mike-stewart
Copy link

Fixed issue where the onConnect() event in a SocketIOClient handler is fired twice for a single connection: once from the run() method in WebSocketClient, and once from the onMessage() event in SocketIOClient. It would seem that the later is redundant.

@koush
Copy link
Owner

koush commented Mar 9, 2013

Good catch.

Should probably actually comment out the other one, as that is when the socketio handshake is actually completed and connected.

@mike-stewart
Copy link
Author

Thanks for the tip. I've switched it to use the other onConnect() call, which is the one that is called second and therefore ensures that the connection is complete.

@koush
Copy link
Owner

koush commented Mar 9, 2013

wrong one, that breaks the WebSocket onConnect callback for any pure websocket consumers.

The one you want is line 242 in SocketIOClient.

@mike-stewart
Copy link
Author

That doesn't solve the problem, as then the onConnect() method in the handler is never called. Both of the onConnect() calls I mention above call the onConnect() method on line 234 of SocketIOClient (an implementation of the WebSocketClient listener), which in turn calls the onConnect() method in the SocketIO handler (line 242).

It seems that we need to get rid of one of the onConnect() calls that I mentioned in the initial pull request. My initial thinking was that we should get rid of the one in SocketIOClient so as to not break pure websocket clients, but as you say the one in WebSocketClient is called earlier than the one in SocketIOClient, so there is the risk that we're calling it before SocketIO has finished handshaking.

Thoughts?

@koush
Copy link
Owner

koush commented Mar 9, 2013

Ah gotcha.

Maybe move replace line 132 with line 242?

So

onConnect();

becomes

 mHandler.onConnect();

That way, the websocket client code remains untouched and working.

This removes the duplicate call to

mHandler.onConnect();

as then it would only be called once the socket IO handshake is actually complete, and not also when the web socket connection is made.

@mike-stewart
Copy link
Author

That works for my application as well. Definitely a more robust solution. I've pushed the fix, thanks!

koush added a commit that referenced this pull request Mar 9, 2013
Fixed double firing of onConnect().
@koush koush merged commit 878842d into koush:master Mar 9, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants