grammyjs / grammY

The Telegram Bot Framework.
https://grammy.dev
MIT License
2.28k stars 112 forks source link

bug: uncaught Exception thrown in bot.stop() #538

Closed AB-70 closed 5 months ago

AB-70 commented 7 months ago

Hello!

I just wanted to make sure the bot is stopped and disposed after a failed login attempt, and came across this issue where the bot would crash the application regardless of error handler being set or using catch().

Here's how to reproduce:

  1. Create the BotClient instance with an invalid or expired bot token.
  2. Set the error handler by using Bot.catch()
  3. Start the bot using Bot.start(). This would fail due to an invalid bot token, and the error is handled either by catch() or the default error handler.
  4. At this point, calling Bot.stop() will crash the application, even if you try to catch(). This is because the bot is not logged in and stop() function calling getUpdates()

This could also be a problem if let's say your bot token is revoked while the bot was running, and then you have to call stop(), which would result in an app crash.

I am not sure if this is a big deal but it is preventing one from gracefully handling these situations.

Solution: Adding a .catch() handler on the following line to call the default error handler might fix this. E.g.

await this.api.getUpdates({ offset, limit: 1 }).catch((err)=>{
     if(this.errorHandler) this.errorHandler(err); else throw err;
}).finally(() => this.pollingAbortController = undefined);

https://github.com/grammyjs/grammY/blob/b78c72ebec625ddf2d23eb27a9eed895710ac3c5/src/bot.ts#L481

KnorpelSenf commented 7 months ago

and the error is handled either by catch() or the default error handler.

That is not correct. The .start() call will simply reject.

In general, bot.catch only catches errors thrown in middleware. Note how it is a function that handles errors containing a context object. If network requests to start or stop or init fail, they will simply reject themselves. The error is not forwarded to bot.catch. There is no context object.

However, there is still a bug here:

4. At this point, calling Bot.stop() will crash the application

If the bot is started and then crashes, it fails to detect that the polling has stopped. bot.stop should be a no-op after a crash, which it isn't. This needs to be fixed.

KnorpelSenf commented 5 months ago

Please review #577 and see if that fixes it.