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

Fix reconnection after opening socket asynchronously #1253

Merged
merged 2 commits into from Sep 30, 2020

Conversation

lolney
Copy link
Contributor

@lolney lolney commented Dec 15, 2018

Fix reconnection after opening socket asynchronously

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

socketio/socket.io#3358

Assume we want to open two sockets, a and b, in the same pool:

var manager = io.Manager(
  `http://localhost:9823`,
  { reconnect: true, reconnectionAttempts: 2 }
);

var a = manager.socket('/room1');

setTimeout(() => {
  var b = manager.socket(
    `/room2`,
  );
}, 500);

If the timeout is short enough (or the sockets are opened synchronously), the manager's connect will be called before errorSub is called, so readyState is opening and b won’t attempt to connect again.

But if the readyState is closed, b will try to connect, fail, and reset the subs, which include the reconnect timer. But since reconnecting is still true, this stops future reconnect attempts.

New behaviour

New sockets will not attempt to connect as long as there is an outstanding reconnect timer (ie, reconnecting on the manager is true).

Other information (e.g. related issues)

You could also reset reconnecting and maybe the Backoff object in cleanup, attempting to connect again immediately. But I wasn't sure whether this should reset the connection attempts or interact at all with autoConnect.

@lolney
Copy link
Contributor Author

lolney commented Dec 16, 2018

Seems like the second option I gave would actually be the better behavior. The socket probably ought to try to connect when you first open it (either by autoconnecting or calling connect yourself). And I think this would naturally lead to resetting the number of connection attempts -- currently, having at least one connection attempt would prevent future reconnects from happening, on top of the fact that we're still in reconnecting == true state.

I can push those changes + some additional tests, if that's the case. Also removes the arrow function in the tests that caused the node v4 job to fail (although those seem to be failing anyway because of the debug package).

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 this pull request may close these issues.

None yet

2 participants