discordjs / discord.js

A powerful JavaScript library for interacting with the Discord API
https://discord.js.org
Apache License 2.0
25.39k stars 3.97k forks source link

isReady returns false although the client is ready #10493

Open GeniusTimo opened 1 month ago

GeniusTimo commented 1 month ago

Which package is this bug report for?

discord.js

Issue description

I wanted to try out if my login retry strategy is working and therefore disabled a privileged gateway intent my bot is requesting on login. This makes it 100 % reproducible but of course this happens too if there are network issues or issues from Discords side on the first login attempt(s)

Steps to reproduce:

  1. Make sure the server members intent is disabled in the developer portal for the bot you're trying to log in to
  2. Start the execution of the code sample below (I used the example main file from the guide as template)
  3. Observe the first one or two failed login attempts
  4. Enable the server members intent in the developer portal again
  5. Observe the successful login attempt

I'd expect the readyClient to return true for isReady() but since PR #9942 got merged it also takes ws.destroyed into account which is still set to destroyed despite the new connection attempt is successful. So this is not a bug introduced in this PR but rather an already exisiting inconsistency with the ws.destroyed not being set back to true when the websocket has successfully connected and is fully operational I believe.

It could also be an implementation error of my retry strategy, but I assumed it's common practice to simply try again via login if it didn't work the first time (of course, with a deeper look at the returned error and not retrying infitite, but I tried to keep the code example as simple as possible)

Code sample

const { Client, Events, GatewayIntentBits } = require('discord.js');
const { token } = require('./config.json');

const client = new Client({ intents: [GatewayIntentBits.Guilds, GatewayIntentBits.GuildMembers] });

client.once(Events.ClientReady, readyClient => {
    console.log(`Ready! Logged in as ${readyClient.user.tag}`);

    console.log(`isReady(): ${readyClient.isReady()}`);
    console.log(`ws.destroyed: ${readyClient.ws.destroyed}`);
});

const loginClient = () => {
    console.log('Attempt to login the client...');

    client.login(token).catch(() => {
        console.log('Attempt to login the client failed');

        setTimeout(() => {
            loginClient();
        }, 10 * 1000);
    });
}

loginClient();

Versions

Issue priority

Low (slightly annoying)

Which partials do you have configured?

No Partials

Which gateway intents are you subscribing to?

Guilds, GuildMembers

I have tested this issue on a development release

No response

didinele commented 1 month ago

While an issue, I'd recommend avoiding retry logic like that. By design just about any error will be retried internally other than those "really bad" exceptions (using an intent you can't use, bad token, etc). Generally, if we throw a hard error it means just restarting won't fix it and you need to adjust something.

GeniusTimo commented 1 month ago

My specific case was that I encountered the error "Opening handshake has timed out" and after the retry it worked by itself (at least that is my assumption so far) As I use isReady() quite often for type validation that's why everything else got messed up, unfortunately. Is there any recommendable retry strategy at all? It sounds to me like the best idea would be to initiate a new instance of the client to make sure that everything is OK internally?

Qjuh commented 1 month ago

You are correct. As didinele already pointed out: if Client#login() rejects you shouldn't use that Client instance anymore at all, because the Client itself will be destroyed by then.

didinele commented 1 month ago

My specific case was that I encountered the error "Opening handshake has timed out" and after the retry it worked by itself (at least that is my assumption so far) As I use isReady() quite often for type validation that's why everything else got messed up, unfortunately.

And it didn't recover by itself from that?

GeniusTimo commented 1 month ago

That's something I'm not 100 % sure about. My error log stated (using discord.js 14.15.3 at that point)

Error: Opening handshake has timed out
    at ClientRequest.<anonymous> (.../node_modules/ws/lib/websocket.js:873:7)
    at ClientRequest.emit (node:events:519:28)
    at TLSSocket.emitRequestTimeout (node:_http_client:856:9)
    at Object.onceWrapper (node:events:633:28)
    at TLSSocket.emit (node:events:519:28)
    at TLSSocket.Socket._onTimeout (node:net:591:8)
    at listOnTimeout (node:internal/timers:581:17)
    at processTimers (node:internal/timers:519:7)

without a timestamp. The next log entry of my own logging told me Error: ConnectTimeoutError: Connect Timeout Error needed to be catched while attempting to log in at a timestamp that would fit the time after that my bot got unresponsive. As the error message sounds quite even I assumed that the "Opening handshake has timed out" could be the more specific error of the "ConnectTimeoutError: Connect Timeout Error". At least that's the error I'm 100 % sure that it didn't recover itself

didinele commented 1 month ago

I recommend listening to the error event (and ShardError), and also the debug event to help diagnose this sort of thing better.

GeniusTimo commented 1 month ago

So far I was only listening to the error event in production. I added the shardError event now too. As this error did not occur in ages for me, I think it should be fine to redo my retry logic to not only reattempt the login but rather initiating a new client. That solves the problem for me, so thank you very much for your (as always) fast and helpful responses! Possibly this issue is obsolete if the client instance is not supposed to recover from a failed login, but I don't understand why the ready event is called then and some additional documentation could be helpful to other developers implementing some sort of retry logic too!