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

async sockets functions should return immediately on getNumSub === 0 #376

Closed
fliespl opened this issue Jan 4, 2021 · 3 comments
Closed
Labels
enhancement New feature or request

Comments

@fliespl
Copy link

fliespl commented Jan 4, 2021

public async sockets(rooms: Set<Room>): Promise<Set<SocketId>> {
}

It's possible for getNumSub to return 0. In this case no response will be received and adapter will fail with error.

This could happen in situation where you have only one server active (i.e. all others are down) and want to immediately get clients list (and redis is not ready yet).

It will end with "timeout reached while waiting for clients response".

Sample repro:

const server = require('http').createServer();
const redisAdapter = require('socket.io-redis')(process.env.REDIS_DSN);
const io = require('socket.io')(server, {
    serveClient: false,
    pingInterval: 10000,
    pingTimeout: 5000,
    cookie: false
});
io.adapter(redisAdapter);
io.listen(3000);
io.sockets.adapter.clients(function (err, clients) {
    console.log(err, clients);
});

This can be of course fixed by doing it this way:

// wait for redis to connect
setTimeout(function() {
    io.sockets.adapter.clients(function (err, clients) {
        console.log(err, clients);
    });
}, 1000);

but I guess it should be handled cleanly in library.

I guess the best resolution would be that sockets resolve promise immediately when getNumSub === 0.

@darrachequesne
Copy link
Member

Yes, that makes sense. Could you please open a PR with that fix?

Thinking about it, I guess we could also handle the case when getNumSub returns 1 (that should not be a common case though).

@darrachequesne darrachequesne added the enhancement New feature or request label Jan 4, 2021
@fliespl
Copy link
Author

fliespl commented Jan 5, 2021

PR ready #378

[edit] I was also planning to provive PR for version 5.4.X, but seems there is no branch for that.

[edit2] Handling getNumSub === 1 is much more trickier. It needs to find out it it's current instance connected or other one, so I didn't touch it with this PR.

darrachequesne added a commit that referenced this issue Mar 12, 2021
@darrachequesne
Copy link
Member

This should be fixed by 6c8d770.

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

No branches or pull requests

2 participants