Skip to content

Commit

Permalink
fix: do not reset the ping timer after upgrade
Browse files Browse the repository at this point in the history
There was two issues with this behavior:

- v3 clients (with allowEIO3: true) were also receiving a "ping" after
a successful upgrade, which is incorrect (in v3, it's the client that
sends the "ping", and the server answers with a "pong")

- the ping timer is not reset after upgrade on the client-side, so an
upgrade which took longer than the `pingTimeout` duration could lead to
a "ping timeout" error on the client-side

I think the latter issue is present since the initial implementation.

Related: socketio/socket.io-client-swift#1309 (comment)

Backported from ff2b8ab
  • Loading branch information
darrachequesne committed Jun 6, 2022
1 parent 3ad0567 commit 1f5d469
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 1 deletion.
1 change: 0 additions & 1 deletion lib/socket.js
Expand Up @@ -208,7 +208,6 @@ Socket.prototype.maybeUpgrade = function (transport) {
self.clearTransport();
self.setTransport(transport);
self.emit('upgrade', transport);
self.setPingTimeout();
self.flush();
if (self.readyState === 'closing') {
transport.close(function () {
Expand Down
18 changes: 18 additions & 0 deletions test/server.js
Expand Up @@ -1092,6 +1092,24 @@ describe('server', function () {
});
});

it('should not timeout after an upgrade', done => {
const opts = { pingInterval: 200, pingTimeout: 20 };
const engine = listen(opts, port => {
const socket = new eioc.Socket('ws://localhost:%d'.s(port));
socket.on('open', () => {
setTimeout(() => {
socket.removeListener('close');
engine.close();
socket.close();
done();
}, 500);
});
socket.on('close', () => {
done(new Error('should not happen'));
});
});
});

it('should not crash when messing with Object prototype', function (done) {
Object.prototype.foo = 'bar'; // eslint-disable-line no-extend-native
var engine = listen({ allowUpgrades: true }, function (port) {
Expand Down

0 comments on commit 1f5d469

Please sign in to comment.