grammyjs / runner

Scale bots that use long polling to receive updates.
https://grammy.dev/plugins/runner
MIT License
18 stars 2 forks source link

`stop()` and `task()` promises never resolve #22

Closed andrew-sol closed 2 months ago

andrew-sol commented 2 months ago

The promises from stop() and task() never resolve on my local machine even though there are no updates to handle (I'm the only user of the bot).

Here is the init and shutdown code from my NestJS app:

@Injectable()
export class BotService implements OnApplicationShutdown {
  protected runnerHandle: RunnerHandle;

  async run(): Promise<void> {
    // ...bot initialization code

    this.runnerHandle = run(bot);
  }

  async onApplicationShutdown(): Promise<void> {
    if (this.runnerHandle) {
      this.logger.log(`Shutting down the bot...`);

      await this.runnerHandle.task();

      this.logger.log(`The bot has been shut down.`);
    }
  }
}

whenever the code recompiles, it stucks at Shutting down the bot... and the following await never resolves.

What are the possible reasons for such behavior?

KnorpelSenf commented 2 months ago

Both promises resolve as soon as all middleware is complete. Whatever handlers you have installed on bot have to resolve normally.

there are no updates to handle

What does this mean? If there are no updates to handle, the promises should resolve immediately.

andrew-sol commented 2 months ago

there are no updates to handle

That's the weird part. The bot is stable and works in production. For local testing I created a separate TG bot, it uses the same code base, it handles my commands well, nothing hangs, but the shutdown hook hangs forever and the app does not recompile. Even if I just start the app, do not send any bot commands from Telegram, change the app's code, it hangs on shutdown.

KnorpelSenf commented 2 months ago

as soon as all middleware is complete

This is actually how the built-in polling behaves, the runner can be stopped regardless of how long the middleware takes. The promise resolves as soon as the middleware has started running for all updates.

I don't really know what else to tell you. The runner works identically across environments. It sounds like your dev setup is broken in one way or another?

If you need someone else to figure out the problem for you, it would be helpful to see an https://sscce.org.

andrew-sol commented 2 months ago

I've created a reproduction repo. Please check it out: https://github.com/andrew-sol/grammy-runner-reproduction

KnorpelSenf commented 2 months ago

An entire NestJS application and 770 deps just to reproduce a bug with 5 lines of code? Why?

Anyway, the reason why the bot is never stopped is because you never call stop.

You can call task if you want to have a promise that resolves as soon as the bot stops for whatever reason, but in order for that to happen, you need to either stop the bot from elsewhere or cause unrecoverable errors that make this promise reject.

I just copied https://github.com/grammyjs/examples/blob/main/runner.ts to a few file and added SIGINT and SIGTERM handlers to see if the runner somehow broke since it was written. That is not the case, everything works as expected. Closing.

andrew-sol commented 2 months ago

Oh, I see, I have to use both stop() and task() to stop the bot.

I read this doc https://grammy.dev/plugins/runner#graceful-shutdown and it does not mention directly that the stop() method should be called (I see now that it's mentioned down the link there). So I thought the task() method would also stop fetching the updates.

Also, the stop() method alone does not shut down the bot in my main app (though it works in the reproduction app).

Thank you very much.

KnorpelSenf commented 2 months ago

Oh, I see, I have to use both stop() and task() to stop the bot.

No. You only have to call stop. Calling task is an observability feature. It returns the same promise that stop returns, so if you already call stop, then you do not need task in the same place.

I read this doc grammy.dev/plugins/runner and it does not mention directly that the stop() method should be called (I see now that it's mentioned down the link there). So I thought the task() method would also stop fetching the updates.

You are right, this part of the docs could be improved. I just did this in https://github.com/grammyjs/website/pull/1096. Could you leave a review?

Also, the stop() method alone does not shut down the bot in my main app (though it works in the reproduction app).

Can you elaborate?

andrew-sol commented 2 months ago

Can you elaborate?

It seems like the cause is the auto-retry plugin:

bot.api.config.use(autoRetry());

when I remove this line the stop() method resolves well.

KnorpelSenf commented 2 months ago

Which version of the plugin do you use? I remember fixing a bug related to this plugin, but I just checked and it was in December 2021.

Most likely, there are API calls that received a rate limits, and the plugin is waiting for a while before retrying the request. The call to stop will therefore wait that long, too. It is not uncommon to receive flood wait errors that take minutes or hours.

andrew-sol commented 2 months ago
"@grammyjs/auto-retry": "^2.0.1"
KnorpelSenf commented 2 months ago

Then everything is correct

andrew-sol commented 2 months ago

Added it to my reproduction repo, now the second shutdown log does not appear after stop().

KnorpelSenf commented 2 months ago

If adding the auto-retry plugin makes a difference, there should be a new issue in the other repo. That would also mean that built-in polling via bot.{start,stop} is affected.

andrew-sol commented 2 months ago

Ok, I'll create one later. Thanks.