grammyjs / website

The grammY documentation website.
https://grammy.dev
MIT License
46 stars 104 forks source link

Please update the documentation of error handling #944

Closed vsly-ru closed 11 months ago

vsly-ru commented 1 year ago

As I discovered in grammyjs/grammY#503, grammy can throw some errors outside of bot.catch() handling. The problem is: this behaviour is not currently documented or even mentioned in the docs on error handling.

I don't know anymore when and why grammy can throw which errors. But I don't want grammy to crash my app (which doesn't consist of just a telegram bot).

So please update the docs to make it clear, that bot.start() can also throw errors

KnorpelSenf commented 1 year ago

What else would you like to do if you misconfigure the application, if not crashing?

vsly-ru commented 1 year ago

@KnorpelSenf My particular app can work without a telegram bot (you can leave tg token empty or disable tg feature entirely). So if it failed to start, I would just treat it as not active, as usual.

It only delivers me notifications when backend starts or some rare important lifetime events occurs. Also I can check uptime and other live stats. I just thought it would be cool to receive notifications on my smart watch when automatic deploy succeeds. And now, due to undocumented error throw, this little qol improvement crashed my entire backend.

Yes, another developer copied config from production to run it somewhere else and did not change the telegram token env, misconfiguration occurred. But it wasn't documented and thus expected to crash the app in this case.

I hope a clarification in the docs will help others with similar use cases to avoid an unexpected crash.

vsly-ru commented 1 year ago

Now, when I learned bot.start() can throw, I updated my tg service like so:

    try {
      this.bot.start();
    } catch (error) {
      this.error(error, `start`);
      this.bot = null;
      return;
    }
    this.logger(`started`);

Other methods won't try to do anything while this.bot is not defined.

KnorpelSenf commented 1 year ago

There are two types or errors.

  1. Expected errors, they can happen anytime and the application needs to handle them. With grammY, this is done using bot.catch.
  2. Unexpected errors, they are caused by the programmer and there is no way of recovering from them, so your application must die immediately so you can fix the problem.

Using multiple instances for the same bot token is a human error (the second type).

In fact, your particular case confirms that crashing is a good strategy. Otherwise you would now be running a dysfunctional application without even noticing. As a result, grammY would randomly lose updates and it would be fairly hard to investigate the problem.

It does not make a lot of sense to document all the different ways how things go wrong when humans make mistakes. We can of course add a generic sentence along the lines of “all other errors are not caught,” but I doubt that this will be any useful. For example, did you know that calling bot.use can throw if used in a way that your code leaks memory? Did you know that bot.on can throw if an invalid filter query is passed? In all cases where us humans are causing the error, there is one main objective: fail as fast as possible (with an expressive error message!) so we can find out what is wrong.

By the way, this code

    try {
      this.bot.start();
    } catch (error) {
      this.error(error, `start`);
      this.bot = null;
      return;
    }
    this.logger(`started`);

is incorrect. If you use TypeScript, you may have noticed that bot.start is async (it performs network calls which are always async). The catch clause is unreachable code.

KnorpelSenf commented 1 year ago

Also, it sounds like your project already starts a web server. Don't you want to use webhooks instead so that you can integrate your bot into your web server? TG will send requests to your endpoints whenever new updates arrive, which saves you from constantly polling the TG servers for updates just for

this little qol improvement

KnorpelSenf commented 11 months ago

I will close this soon if there is no further activity

KnorpelSenf commented 11 months ago

I'll risk that you call me rude again since you don't seem to be around anymore