Skip to content

[FEATURE] Automatic dismiss repeated callbackquery updates #2539

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Poolitzer opened this issue May 29, 2021 · 9 comments
Closed

[FEATURE] Automatic dismiss repeated callbackquery updates #2539

Poolitzer opened this issue May 29, 2021 · 9 comments
Assignees

Comments

@Poolitzer
Copy link
Member

Is your feature request related to a problem? Please describe.

Clients allow users to spam button presses. This leads to race conditions where the code edited the reply_markup and then tries to edit again even though it was already edited because several updates resulted from the same button.

Describe the solution you'd like

I would like to have the library handle these race conditions with some simple code checks and not propagate these repeated updates to the handlers.

Doing this for non async handlers is very easy. All this library needs to store is the callback_data from the first inline button send/edit_message request, and then the callback_data from the next one or a deleted flag if the reply_markup is removed; all this valued to a unique key from chat+message_id or inline_message_id. If then an update happens which has the first callback_data even though the next one is expected, or another callbackquery update happens when the delete flag is set, the query should be answered (for this, maybe the answer value should be stored as well) and then the update dismissed.

I am not sure how to handle this for async handlers, but I think we could try and apply the same logic, and warn in the docs that it can just happen that two Updates slip through. I hope someone here can think of a better way to handle this. One alternative would be to not handle a multiple update with the same callback_data from the unique key. But that could lead to an issue where something else in the code breaks temporarily, and a user isn't able to try hitting the button again after some time when that issue is resolved. Maybe we can work with try expect for that?

Describe alternatives you've considered

This can be easily implemented client side, and with minimal coding effort with a decorator. I vote against this for two reasons: I would need to do this for every callbackqueryhandler manually, and I mostly think beginners will overlook this issue. Then they are facing errors they cant understand because no one thinks about spamming a button that fast.

@Bibo-Joshi
Copy link
Member

Hi. I didn't follow the discussion in the user group about this too closely, but you did a nice summary and I see the use case 🙂 I'd like to add two things to this discussion:

  1. An approach that might work
  2. Arguments against having this in the main lib :D

Workable approach:

Basic idea: have the handler make the decision. That a rather clean design IMO and this should also be pretty safe for run_async, as check_update is called before any handle_update logic.

Click to expand MWE

from collections import defaultdict
from typing import Optional, Union, Tuple, DefaultDict

from telegram import Update, InlineKeyboardMarkup, InlineKeyboardButton
from telegram.ext import Updater, CommandHandler, CallbackQueryHandler, CallbackContext

import logging

# Enable logging
logging.basicConfig(
    format='%(asctime)s - %(name)s - %(levelname)s - %(message)s', level=logging.INFO
)
logger = logging.getLogger(__name__)


class CustomCQH(CallbackQueryHandler):
    handled_queries: DefaultDict[
        Tuple[Optional[int], Optional[int], Optional[int], str], bool
    ] = defaultdict(bool)

    def check_update(self, update: object) -> Optional[Union[bool, object]]:
        if super().check_update(update):
            assert isinstance(update, Update)
            chat_id = update.effective_chat.id if update.effective_chat else None
            message_id = update.effective_message.message_id if update.effective_message else None
            inline_message_id = update.callback_query.inline_message_id
            data = update.callback_query.data
            key = (chat_id, message_id, inline_message_id, data)
            if self.handled_queries[key]:
                # Button was already handled
                logger.info('Ignoring CallbackQuery because the button was already handled.')
                return False
            else:
                # Mark query as handled
                self.handled_queries[key] = True
                return True
        return False


def start(update: Update, _: CallbackContext) -> None:
    update.message.reply_text(
        update.message.text,
        reply_markup=InlineKeyboardMarkup.from_button(
            InlineKeyboardButton('Click me', callback_data='data')
        )
    )


def button(update: Update, _: CallbackContext) -> None:
    update.callback_query.answer()
    update.effective_message.edit_text('This button was already handled.')


def main() -> None:
    updater = Updater("TOKEN")
    dispatcher = updater.dispatcher
    dispatcher.add_handler(CommandHandler("start", start))
    dispatcher.add_handler(CustomCQH(button, run_async=True))
    updater.start_polling()
    updater.idle()


if __name__ == '__main__':
    main()

Arguments agains adding this to PTB

While I do see that this would be useful for working with inline buttons in general, I fell uncomfortable with adding this to PTB - at least as a first impression. My reasons:

  • There are other viable approaches. E.g. if you just don't want to get errors when a effective_message.edit_text fails b/c it was already called once, an appropriate error handler might already do the trick
  • It's hard to be sure that it's in fact the same button again. Even when we check (chat_id, message_id, inline_message_id, callback_data), the user might have edited the message and appended the same keyboard again, or a different one but the same callback_data appears again. So we'd rely on the user not doing that - risky …
  • I guess we agree that it should be opt-in only anyway, but the mix of uncertainty (2nd bullet point) and the fact that this is a rather low-level trick makes it feel like we'd be handing a gun to the user to shoot themself in the foot. We'd probably have to add more warnings than lines of code.

I would therefore suggest to move this to ptbcontrib, where we don't have to be as careful about what the user understands and what they don't. Maybe I'll be thinking differently about this in a week, but that's my thoughts for today :)

@Poolitzer
Copy link
Member Author

That a rather clean design IMO and this should also be pretty safe for run_async, as check_update is called before any handle_update logic.

I dont think this will work. Your MWE doesn't include the corresponding API call to update the key. If we introduce this, which is a very low level approach, we can completely mitigate your second point. But if we do this, we can't fully say if an update needs to be handled or not

the user might have edited the message and appended the same keyboard again, or a different one but the same callback_data appears again. So we'd rely on the user not doing that - risky …

Or we introduce code in the ext.bot to actually include this in our expected key list. This might just require the user to link the method to the handler. then we should probably really put it in contrib. If we can do without this, I would vote to add it to main though.

@Bibo-Joshi
Copy link
Member

That a rather clean design IMO and this should also be pretty safe for run_async, as check_update is called before any handle_update logic.

I dont think this will work. Your MWE doesn't include the corresponding API call to update the key. If we introduce this, which is a very low level approach, we can completely mitigate your second point. But if we do this, we can't fully say if an update needs to be handled or not

You want to only dismiss doublicate callback queries, if the callback contains a edit_message_*? That seems nearly impossible without any logic in in the callback. And I don't see, how this makes my second bullet point invalid. Or am I missing your point?

the user might have edited the message and appended the same keyboard again, or a different one but the same callback_data appears again. So we'd rely on the user not doing that - risky …

Or we introduce code in the ext.bot to actually include this in our expected key list. This might just require the user to link the method to the handler. then we should probably really put it in contrib. If we can do without this, I would vote to add it to main though.

Sorry, I can't follow your here. Iclude what in which key? Link which method to which handler, and what does "link" mean here?

@Poolitzer
Copy link
Member Author

Poolitzer commented May 31, 2021

After a bit in depth internal discussion with @Bibo-Joshi, the idea on moving forward with this issue is the following:

  • implement the basic way of checking if a callback_query update is a duplicate (key value unique identifier to callback_data, if they are the same and one update like that already happened, ignore the update)
  • store this in a non persistent way without draining too much memory. we will achieve this via the external library introduced with Arbitrary callback data #1844
  • activate this behaviour via a handler flag
  • add a big warning to the docs that the buttons need to have unique callback data

@Bibo-Joshi Bibo-Joshi mentioned this issue Jun 5, 2021
6 tasks
@Bibo-Joshi
Copy link
Member

Almost forgot to comment this:

With arbitrary callback data, you can now already cover the non-async case:

def list_button(update: Update, context: CallbackContext) -> None:
"""Parses the CallbackQuery and updates the message text."""
query = update.callback_query
query.answer()
# Get the data from the callback_data.
# If you're using a type checker like MyPy, you'll have to use typing.cast
# to make the checker get the expected type of the callback_data
number, number_list = cast(Tuple[int, List[int]], query.data)
# append the number to the list
number_list.append(number)
query.edit_message_text(
text=f"So far you've selected {number_list}. Choose the next item:",
reply_markup=build_keyboard(number_list),
)
# we can delete the data stored for the query, because we've replaced the buttons
context.drop_callback_data(query)

Line 73 makes sure that the second incoming callback query will have InvalidCallbackData.

@Poolitzer Poolitzer self-assigned this Oct 29, 2021
@Bibo-Joshi
Copy link
Member

Update: We have plans to allow for updates to be processed asynchronously in v14, so moving this back to Stage 3 …

@Bibo-Joshi
Copy link
Member

It's been a while since we last discussed this issue and quite a bit has changed. In particular, we now have new mechanism of corcurrent update handling in v20, also now with custom UpdateProcessors (#3654). A few thoughts that I now have on this topic:

IMO two very crucial parts of making a feature like this workable are

  • assigning unique ids to buttons. Otherwise the user needs to keep track of whether they are re-using the same callback_data.
  • a sane caching mechanism

With aribtray callback data, we actually already have a solution for both of these things in place and it would even be possible to re-use that for this purpose quite easily. We could add a parameter auto_drop_callback_data to CallbackQueryHandler that makes the handler call context.drop_callback_data(callback_query) in handle_update. In check_update we can then simply return False if the callback_data is of type InvalidCallbackData. Several notes on this are in order:

  1. Just ignoring InvalidCallbackData might yield false negatives, i.e. if buttons actually timed out or are invalid, we won't know. To avoid this, we'd have to keep an additional cache in CallbackQueryHandler. This is an overhead, but can certainly be discussed.
  2. This would ofc only work if arbitrary callback data is enabled. We can add corresponding runtime warnings
  3. As discussed above, concurrency is an issue. We have two kinds of concurreny: non-blocking handlers and concurrent updates. TBH I don't see a good way to make this feature work reliable in these cases and as orginially stated by Pool, I'd be fine with just excluding these use cases. We can add some runtime warnings if the CQH is non-blocking or if we detect that an updateprocessors with max_concurrent_updates > 1 is being used.

@Bibo-Joshi
Copy link
Member

Yet more additional thoughts:

  1. Concurrency is not only a proboblem if max_concurrent_updates > 1. Indeed, already by using blocking=False or run_async it should be able to contsruct situations where a second incoming is accepted by check_update before the button is marked as already used
  2. Actually this the case not only for this feature request but in general for arbitrary callback_data. We might want to add a few more notes to the docs about that …

@Bibo-Joshi
Copy link
Member

As there is no profound interest in this feature in the dev team at the moment and the technical realization is still rather unclear, I'm closing this issue as won't-do for now. If interest spikes in the future, we can revisit.

@Bibo-Joshi Bibo-Joshi closed this as not planned Won't fix, can't repro, duplicate, stale Oct 21, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants