Skip to content
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

Server side "connection" event triggered twice when a socket.io cpp client is connected #10

Closed
jianjunz opened this issue May 3, 2015 · 3 comments · Fixed by #11
Closed

Comments

@jianjunz
Copy link
Contributor

jianjunz commented May 3, 2015

If we add some log inside connection event handler, like

io.on('connection', function(){
  console.log('connection.');
});

It will output twice.

After a briefly investigation, I found socket.io cpp client sent a "connect" packet to the server after connection has been established. Server side seems to recognize it as a new connection. So "connection" event triggered twice.

Currently, I deleted

m_client->send(p);

from sio_socket.cpp (line 269). Then the second "connection" is gone. But I'm not sure if the "connect" packet is necessary or not.

@melode11
Copy link
Contributor

melode11 commented May 3, 2015

Hi, Thanks for your report, this is a known issue. In protocol,the connect packet is necessary for initializing a new namespace. But there is one special case: the default namespace '/' which is initialized in serverside by default. So no positive connect packet is needed in clientside. You can patch it for that spacial case.

But anyway, server side connection callback will return the same socket instance for the same namespace, so don't be afraid, no extra recources will be constructed in both sides.

@jianjunz
Copy link
Contributor Author

jianjunz commented May 3, 2015

Thanks for your explanation.

In most cases, this issue doesn't have negative impacts. But it will be a problem if we add some code in "connection" callback and expect it to be executed only once per connection. I thinks it's a reasonable usage for "connection" event.

I'll submit a patch for this special case. Maybe several days later.

@melode11
Copy link
Contributor

melode11 commented May 4, 2015

You're right, it's a reasonable usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants