Skip to content
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

ShowTypingMiddleware - Infinite loop of typing activity when Adapter on_error() method is called. #1980

Open
Kevv-J opened this issue Nov 1, 2022 · 10 comments
Labels
Bot Services Required for internal Azure reporting. Do not delete. Do not change color. bug Indicates an unexpected problem or an unintended behavior. customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. customer-reported Issue is created by anyone that is not a collaborator in the repository. ExemptFromDailyDRIReport Use this label to exclude the issue from the DRI report.

Comments

@Kevv-J
Copy link

Kevv-J commented Nov 1, 2022

Version

I am using the following packages

botbuilder-schema==4.13.0
botbuilder-core==4.13.0
botbuilder-dialogs==4.13.0

Describe the bug

ShowTypingMiddleware goes into an infinite loop of sending typing activity to user when on_error() method of the BotFrameworkAdapter is called. It doesn't stop till the application is restarted. This is easily reproduced in BotBuilder-Samples tested with BotFramework Emulator.

To Reproduce

Steps to reproduce the behavior:

  1. add ADAPTER.use(ShowTypingMiddleware(delay=0.5, period=1.0)) to app.py of 13, Core Bot Python Sample
  2. Write some code that takes calls the adapter's on_error() method i.e. print(ThisIsABug) in destination step in dialogs/booking_dialog.py
  3. Start the app and test the dialog from BotFramework Emulator.
  4. The app sends infinite typing activity to the emulator when the error is encountered in code.

Expected behavior

ShowTypingMiddleware should stop sending typing activity if on_error() method is called.

Screenshots

Screenshot from 2022-11-01 14-38-03

Additional context

NA

@Kevv-J Kevv-J added bug Indicates an unexpected problem or an unintended behavior. needs-triage The issue has just been created and it has not been reviewed by the team. labels Nov 1, 2022
@stevkan stevkan added customer-reported Issue is created by anyone that is not a collaborator in the repository. Bot Services Required for internal Azure reporting. Do not delete. Do not change color. labels Nov 1, 2022
@stevkan stevkan self-assigned this Nov 1, 2022
@stevkan
Copy link

stevkan commented Nov 1, 2022

@Kevv-J,

ADAPTER.use(ShowTypingMiddleware(delay=0.5, period=1.0))

The delay and period properties take values expressed in milliseconds. So, for example, if you are wanting the delay to be half a second, then you would want to pass in delay=500. As you have it, the delay is set to half a millisecond and the period to one millisecond which likely is contributing to your issue.

Try adjusting those values, test again, and let me know if the issue persists.

@stevkan stevkan added customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. and removed needs-triage The issue has just been created and it has not been reviewed by the team. labels Nov 1, 2022
@Kevv-J
Copy link
Author

Kevv-J commented Nov 2, 2022

Hi @stevkan,
Thanks for the quick reply. The documentation states that the delay and period properties take values in seconds as mentioned here, I had just used the default values of the function.

Just in case the documentation is incorrect I tried your suggestion with 500 as delay and 1000 as period and the typing activity was not sent even after 30 seconds of inactivity. tested by adding an await asyncio.sleep(30) inside the dialog.

Am I doing something wrong?

@stevkan
Copy link

stevkan commented Nov 3, 2022

@Kevv-J,

Apologies for the confusion there. I had just looked over that bit of code but failed to notice it specifies seconds (vs milliseconds which is used in the other three SDKs). While it really shouldn't matter, it also shouldn't be necessary to name the properties when passing in the values to the middleware. Can you try ShowTypingMiddleware(0.5, 1.0) and see if that makes any difference.

In the meantime, I will work on repro'ing this and will get back to you with my results.

@Kevv-J
Copy link
Author

Kevv-J commented Nov 9, 2022

Hi @stevkan

No luck with ShowTypingMiddleware(0.5, 1.0) I'm getting the same infinite typing activities bug when it goes into on_error().
Were you able to reproduce this on your end?

@Kevv-J Kevv-J closed this as not planned Won't fix, can't repro, duplicate, stale Nov 9, 2022
@Kevv-J Kevv-J reopened this Nov 9, 2022
@anishprasad01 anishprasad01 self-assigned this Nov 11, 2022
@anishprasad01
Copy link

@Kevv-J,

We were able to reproduce this. Currently investigating further.

@Kevv-J
Copy link
Author

Kevv-J commented Nov 23, 2022

Hi @anishprasad01,

I have come up with a make-shift solution, can you confirm if this would work.

botbuilder.core.ShowTypingMiddleware

In on_turn() method

I have changed all timer to self.timer so its accessible from instance of the class.

13.core-bot/bots

in on_error() method of BotConfig

I have added the following code block

typing_middleware = [
    x
    for x in self.ADAPTER._middleware._middleware
    if isinstance(x, ShowTypingMiddleware)
]
if typing_middleware:
    typing_middleware[0].timer.set_clear_timer()

This seems to be working as expected for now, but do you see any issues in doing things this way. Is there a better way to do this?

Thanks in advance
Kevin

@GermanZz
Copy link

Is there any progress on this? I am very interested as facing the same issue

@anishprasad01
Copy link

Apologies for the delay. We're still planning on addressing this, it hasn't been forgotten.

@anishprasad01
Copy link

@Kevv-J,

I explored modifying this behavior within the SDK by adding some sort of timeout or auto-cancel of the middleware task, but after some thought, I'm afraid that doing so will break bots with long-running operations that use typing middleware. Stopping the middleware within the SDK would require a bot developer to know exactly how long each operation will take, and that is simply not possible.

Given this, your approach of fixing it within the bot itself would seem to be the best way to accomplish it. I will be looking for an alternative solution just in case, but if it's working for you, then great!

@anishprasad01 anishprasad01 added the ExemptFromDailyDRIReport Use this label to exclude the issue from the DRI report. label Jan 13, 2023
@anishprasad01
Copy link

It seems like your workaround to create a modified middleware is probably the most appropriate solution for the moment, as an SDK fix would likely require a breaking change (as discussed in my previous post).

Thank you again for reporting this issue. An SDK fix will remain under consideration.

@stevkan stevkan removed their assignment Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bot Services Required for internal Azure reporting. Do not delete. Do not change color. bug Indicates an unexpected problem or an unintended behavior. customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. customer-reported Issue is created by anyone that is not a collaborator in the repository. ExemptFromDailyDRIReport Use this label to exclude the issue from the DRI report.
Projects
None yet
Development

No branches or pull requests

4 participants