microsoft / botbuilder-python

The Microsoft Bot Framework provides what you need to build and connect intelligent bots that interact naturally wherever your users are talking, from text/sms to Skype, Slack, Office 365 mail and other popular services.
http://botframework.com
MIT License
701 stars 280 forks source link

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

Open Kevv-J opened 1 year ago

Kevv-J commented 1 year ago

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

stevkan commented 1 year ago

@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.

Kevv-J commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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?

anishprasad01 commented 1 year ago

@Kevv-J,

We were able to reproduce this. Currently investigating further.

Kevv-J commented 1 year ago

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 commented 1 year ago

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

anishprasad01 commented 1 year ago

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

anishprasad01 commented 1 year ago

@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 commented 1 year ago

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.