-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Manager.socket() re-uses a manually disconnected socket without reconnecting it #1460
Comments
Thanks, I could indeed reproduce. Since we cannot have two (or more) active Socket instances for the same namespace and the same Manager, the most reasonable behavior would be to call What do you think? Note: this behavior is present since forever: https://github.com/socketio/socket.io-client/blob/1.0.0/lib/manager.js#L278-L289 |
I agree, that sounds good. |
@darrachequesne is this an easy fix? |
This should be fixed by b7dd891, included in version Please reopen if needed! |
@darrachequesne I've noticed that the fix in b7dd891 causes another bug. If the socket uses multiplexing, the socket will emit two connection packets to the server. For example: // Server receives the following connection packet *once*:
// 0/one
let one = io.connect('/one');
// Server receives the connection packet below *twice*:
// 0/two
let two = io.connect('/two'); This effectively creates two connected sockets to the // Server
let nsp = io.of('/two');
nsp.on('connection', socket => {
socket.join('room');
});
// Somewhere later
nsp.to('room').emit('foo', 'bar'); the The issue is quite critical for me because it means that chat messages in a chat system of mine get delivered twice. |
I've been thinking a bit, and I think that the most elegant fix would be to only call
So let socket = this.nsps[nsp];
if (!socket) {
socket = new Socket(this, nsp, opts);
this.nsps[nsp] = socket;
} else if (socket.disconnected && this._autoConnect) {
socket.connect();
}
return socket; instead of how it's done now, being let socket = this.nsps[nsp];
if (!socket) {
socket = new Socket(this, nsp, opts);
this.nsps[nsp] = socket;
}
if (this._autoConnect) {
socket.connect();
}
return socket; |
This bug was introduced in [1]: a multiplexed socket could in some cases send multiple CONNECT packets, resulting in duplicate connections on the server side. A cached socket will now be reopened only if it was inactive, that is, if one had explicitly called socket.disconnect() before. Related: #1460 [1]: b7dd891
@sebamarynissen you are absolutely right, the fix did indeed contain a bug. This should be finally fixed by 46213a6, included in version |
@darrachequesne Checked and confirmed that |
Describe the bug
When calling Manager.socket('/someNamespace') a second time after already having called it before, it will re-use the same socket. If that socket has been manually disconnected using Socket.disconnect(), it will be returned in a disconnected state.
To Reproduce
Please fill the following code example:
Socket.IO server version:
3.1.1
Server
Socket.IO client version:
3.1.1
Client
Expected behavior
I expected Manager.socket('/someNamespace') to return a socket that connects to the server. Either the old socket that tries to connect again, or a fresh socket.
Additional context
The context is a single page application where not all namespaces are relevant to all "pages" in the application, so we disconnect from a page-specific namespace when leaving a page. The reason for explicitly creating a Manager instance is to avoid creating a new underlying connection when we reconnect to the namespace.
Our current workaround is to manually call Socket.open() after Manager.socket().
The text was updated successfully, but these errors were encountered: