grammyjs / grammY

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

Throwing an error inside the `otherwise` callback of `conversation.waitFor()` doesn't kill/exit the conversation #282

Closed yn4v4s closed 2 years ago

yn4v4s commented 2 years ago

When using Prisma Storage Adapter (regardless of what connector you are using: MySQL, SQLite, etc) for sessions, throwing an error inside the otherwise callback of conversation.waitFor() doesn't kill/exit the conversation.

Repo whit minimal setup for reproduction: https://github.com/yn4v4s/grammy-fastify-errors main branch uses MySQL, checkout the sqlite branch for SQLite setup.

KnorpelSenf commented 2 years ago

I cannot reproduce this. For me, when the otherwise handler is run because something other than a callback query arrives, the error is thrown and the conversation exits exactly as expected.

Do you see the issue happening with the following code, as well?

import {
  Bot,
  Context,
  InlineKeyboard,
  session,
} from "https://deno.land/x/grammy@v1.11.1/mod.ts";
import {
  type Conversation,
  type ConversationFlavor,
  conversations,
  createConversation,
} from "https://raw.githubusercontent.com/grammyjs/conversations/main/src/mod.ts";

type CustomContext = Context & ConversationFlavor;
type CustomConversation = Conversation<CustomContext>;

const bot = new Bot<CustomContext>("");

bot.use(session({ initial: () => ({}) }));

const convo = async (conversation: CustomConversation, ctx: CustomContext) => {
  await ctx.reply("Yo, click any button!", {
    reply_markup: new InlineKeyboard().text("hi"),
  });
  const {
    callbackQuery: { data },
  } = await conversation.waitFor("callback_query:data", () => {
    throw new Error("Told ya man!");
  });
  console.log(data);
  await ctx.reply("Bye!");
};

bot.use(conversations());
bot.use(createConversation(convo));

bot.command("convo", (ctx) => ctx.conversation.enter("convo"));
bot.start();
yn4v4s commented 2 years ago

But, you ain't using Prisma Storage Adapter in your example, this doesn't happen with in-memory sessions

KnorpelSenf commented 2 years ago

This is not a bug.

The session plugin only stores data if the middleware runs successfully. This is good: if the middleware crashes, the session may not write back incomplete and potentially corrupted data.

In the case of conversations, this means that when the conversation crashes, you need to catch the error somewhere in your middleware tree again. If the error propagates up to the session, the data won't be saved. That's why it looks like the conversation isn't left—the conversation plugin does indeed leave the conversation, but this action is not persisted in the session.

In-memory sessions are the only storage that do not have this problem, as everything is kept in memory, so all actions are reflected immediately in the “storage,” no matter whether the middleware fails or not.

The best solution here would be to use error boundaries. They let you catch the error between bot.use(session()) and bot.use(createConversation()), which will solve this problem.

During the design process of the conversations plugin, this case was discussed in great length. There were a number of options on the table, including adding a custom error handling system to conversations, but we decided that it does not seem useful to effectively duplicate error boundaries into the conversations plugin. Also, writing back corrupted data because it's useful in this one case would likely create more problems than it solves.

The documentation says that throwing an error is a correct way to leave a conversation, but it does not elaborate on this pitfall. This will have to be fixed on https://grammy.dev.

KnorpelSenf commented 2 years ago

Can you review https://github.com/grammyjs/website/pull/513 and approve it (Files > Review > Approve) if you think that the explanation is good enough, and would have prevented you from running into this issue?

Note that a preview of the website is built and deployed. You can open the link the was commented on the pull request to see how the documentation changes will be reflected on the website.

I will close this issue due to https://github.com/grammyjs/website/pull/513. Feel free to comment if you have further questions.