grammyjs / grammY

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

message handler not called for weird reasons #527

Closed the-tchaw closed 4 months ago

the-tchaw commented 7 months ago

Dears,

thanks for all the effort you put into this.

I am having a weird issue where the handler for bot.on('message', ()=>{}) is not being called after I click on a inline keyboard button and answer. now if I comment out the code that returns the promise it will work :/

code explanation: I create a bot that when /start is called it spawns a message with a menu with dynamic range that contains one button. when the user click this button the bot will send a message asking for his age and this message has force_reply=true also I save a function to an object that is called when this message is replied to (from the bot.on('message') handler). when the user answers I call the saved function which logs the user's his reply.

Another thing to note is when it is not working (promise return not commented). if you stop the app and run it again you will get the force_reply message asking for your age another time.

const awaitedReplies: { [key: string]: (context: Context) => void } = {};
export const startBot = async () =>
  new Promise<Bot>((resolve) => {
    const bot = new Bot(config.grammy.botToken);

    bot.on('message', (context, next) => {
      console.log('onMessage');
      if (context.message.reply_to_message) {
        const chatId = context.message.reply_to_message.chat.id;
        const messageId = context.message.reply_to_message.message_id;
        const id = `${chatId}:${messageId}`;
        console.log('replied to: ', id, awaitedReplies[id] ? 'has listener' : 'no listener');
        awaitedReplies[id]?.(context);
      }
      next();
    });

    const startMenu = new Menu('StartMenu').dynamic(async () => {
      const range = new MenuRange();

      range.text('Age', async (context) => {
        await bot.api
          .sendMessage(context.chat!.id, 'What is your age?', {
            reply_markup: {
              force_reply: true,
            },
          })
          .then((message: Message) => {
            const chatId = message.chat.id;
            let resolve: (context: Context) => void = () => {};
            const messageId = message.message_id;
            const id = `${chatId}:${messageId}`;
            console.log('force reply id:', id);
            awaitedReplies[id] = (context: Context) => {
              console.log('user reply:', context.message);
              resolve(context);
            };
            // comment the below three lines and it works :/
            return new Promise<Context | { message: { text: string } }>((_resolve) => {
              resolve = _resolve;
            });
          })
          .catch(console.error);
      });
      return range;
    });
    bot.use(startMenu);

    bot.command('start', (context) => {
      context.api.sendMessage(context.chat.id, 'Welcome\\. what to do next?', {
        parse_mode: 'MarkdownV2',
        reply_markup: startMenu,
      });
    });
    bot.start({
      onStart: () => resolve(bot),
    });
  });
KnorpelSenf commented 7 months ago

That is expected. Updates are processed in sequence for built-in polling. If you return a promise from a handler, this promise needs to resolve before the next update can be processed. If, however, that promise depends on the next update, you effectively just deadlocked yourself.

Why do you even create your own promise instances manually anyway? Those are always big foot guns unless you know exactly what you're doing. I suggest you drop them, and drop all the chaining with .then, and use proper async/await the normal way.

the-tchaw commented 7 months ago

I am all in if there is a native way to do it. What I want is to be able to ask the user a question like "what is your age?" and then await (think promise) his response with a timeout. ill parse and validarte the user input no problem. Like is there a native way to do that?

KnorpelSenf commented 7 months ago

That cannot work with promises. What if you restart your server, then the bot forgets everything it was doing. What if a lot of people don't answer, then you have hundreds of thousands of promises floating around in memory, wasting RAM.

There's a plugin that solves all this, but you should be aware that it doesn't combine very well with menus. It makes sense to use inline keyboards instead. You can then call waitForCallbackQuery. https://grammy.dev/plugins/conversations

the-tchaw commented 7 months ago

The bot restart is an edge case even then so what the user can restart the conversation. The memory issue well its up to the dev right. for example the code I provided above has no memory issues it cleans up after itself and there is a timeout if the user never answers. the use of promises makes the dev experience much more streamlined. Now I have to use nested callbacks which still has all the issues you mentioned but makes the code more complicated than it needs to :(

edit: There is also an issue that if I use promises the bot's on('message') stops to be called for the reply message AND for all messages afterwards :/ it like it is stuck

KnorpelSenf commented 7 months ago

It is not an edge case to restart the bot. You do this for every new deployment. Changing the code is part of every software that isn't dead yet.

On serverless platforms, grammY is constantly started and stopped many times. Those platforms are a popular choice due to their ease of use.

The code you provided has a dead lock. Its memory usage doesn't matter at that point.

Conversations are an abstraction that works like promises but doesn't store stuff in memory. You get a comparable developer experience to promises but without all the issues.

You do not need nested callbacks, that is just wrong. Where did you read that this is a good way of writing your code?

the-tchaw commented 7 months ago

an edge case not in that sense. It is an edge case in the sense that if some users needs to reprompt when a restart occures so be it. You cant solve that even with using persistant sessions.

The code I provided has no deadlock the user has 1 minute to respond otherwise a timeout occures. now in production code I will notify him that he took too long to respond and delete the prompt message.

I do not like callbacks neither did I claim it was a good way to write code. But with your restriction you left me no choice.

Anyways thanks for your response it must be very stressfull maintianing such a big library and I apritiate your efforts. If you would consider rethinking this restriction ping me. You know developers can figure things out and when you close doors so that devs cant create memoryleaks/deadlock you dont only kill possible bugs but may also kill innovation inadvertedly

If anyone is interested i can post the code you give it a set of questions and a callack and it will ask the user all these questions and then call the callback with all responses or an error (case timeout or validation error)

KnorpelSenf commented 7 months ago

You cant solve that even with using persistant sessions.

I did.

But with your restriction you left me no choice.

Neither am I preventing you from anything, nor do I see how you ended up deciding on callbacks. That would be the first grammY bot I see that uses callbacks. Why do you think you need them?

If you would consider rethinking this restriction ping me.

Welp … I don't know which restriction you're talking about ._.

kill innovation

I can't follow

If anyone is interested i can post the code you give it a set of questions and a callack and it will ask the user all these questions and then call the callback with all responses or an error (case timeout or validation error)

There are many such examples out there already. We should start collecting some of them in the example bots repository. Perhaps that's the best place to start?

KnorpelSenf commented 7 months ago

Oh btw, if all you're asking for is concurrency, then this is obviously possible (and very easy). It's still not a good idea to write a bot the you way you illustrated, but I also don't want to leave you with a feeling that I would stop you from doing anything. Check out https://grammy.dev/plugins/runner but remember that you will shoot yourself in the foot with that :)

KnorpelSenf commented 4 months ago

Closing due to inactivity