botbuilder-dotnet icon indicating copy to clipboard operation
botbuilder-dotnet copied to clipboard

Adaptive ForEachElement loop's incorrectly when changes cause ContinueDialogAsync to be recalled

Open Rich-biomni opened this issue 3 years ago • 7 comments

Version

c# 4.16.1

Describe the bug

When looping using the ForEachElement, if something causes changes the ForEachElement runs the first action in its actions list rather the continuing with the next.

To Reproduce

Use the ForEachElement with > 1 loops In the Actions Property Ask a question Begin Dialog where the dialog is an adaptive dialog, and its action causes change (for example use EditAction) Show a message when the dialog starts Show the result of Question

Run the bot, notice on the 2nd time around the loop it jumps back to first action in its actions list

Expected behavior

Each loop of the ForEachElement should carry out each action sequentially

Additional context

Just guessing, but because action changes cause ContinueDialogAsync to be recursively called the 'childDialogState' can get out of sync when the final pop of recursion returns from ContinueDialogAsync. Is the fix to reacquire the 'childDialogState' after ?

Attached is Simple Bot that shows it going wrong

SimpleBot.zip

Rich-biomni avatar Aug 01 '22 11:08 Rich-biomni

Thanks for the issue details and repro project. We have some fixes for an issue related to this in the main branch here Handle DialogEvents.RepromptDialog in ForEachElement that will be part of an upcoming release. We will use your sample to test this and see if it will solve this issue.

LeeParrishMSFT avatar Aug 01 '22 18:08 LeeParrishMSFT

Thanks for the issue details and repro project. We have some fixes for an issue related to this in the main branch here Handle DialogEvents.RepromptDialog in ForEachElement that will be part of an upcoming release. We will use your sample to test this and see if it will solve this issue.

Looks like this issue was reported against Bot Framework DotNet SDK 4.16.1 which includes the fix you're referring to but this issue is still broken

JamesSinclairBiomni avatar Aug 03 '22 09:08 JamesSinclairBiomni

@Rich-biomni - Unfortunately, I am unable to test using your Simplebot project. I am getting dependency errors related to the BotFramework Solutions library you have included that I am unable to get around. The library was marked as obscelete on Mar 31, 2022 and is no longer supported. Additionally, its docs state the same while adding that there is no guarantee it will work if used beyond v4.9.1 of the SDK.

Would you be able to provide a more stipped down version of your bot (or just a new project) that focuses on and reproduces the error?

stevkan avatar Aug 05 '22 00:08 stevkan

@stevkan - Dependency removed

SimpleBot.zip

Rich-biomni avatar Aug 05 '22 07:08 Rich-biomni

@Rich-biomni - Thank you for the updated project. I have it working now, without issue, using a local build of the SDK. I have started testing and will let you know what I find. I've found a couple curiosities, but I haven't had a chance to dig into them just yet. I will let you know what I find when I know more.

stevkan avatar Aug 05 '22 23:08 stevkan

@Rich-biomni - I was able to repro the issue using your sample. Unfortunately, I was unable to find a simple solution / workaround to help mitigate this for you.

Escalating to @ceciliaavila for continued support on this.

stevkan avatar Aug 09 '22 18:08 stevkan

Is this repeatable in 4.17.1?

tracyboehrer avatar Aug 10 '22 14:08 tracyboehrer

@tracyboehrer

Issue exists in 4.17.1 as well

Rich-biomni avatar Aug 15 '22 13:08 Rich-biomni

This PR for this was reverted due to a breaking change. We will need to revisit.

Notes for SDK: This breaks Bot Designers (PVA) "SlotFillingTest" and some "IntentSwitchingTests". We will need to extract the tests to reproduce outside PVA.

tracyboehrer avatar Sep 08 '22 13:09 tracyboehrer

This PR for this was reverted due to a breaking change. We will need to revisit.

Notes for SDK: This breaks Bot Designers (PVA) "SlotFillingTest" and some "IntentSwitchingTests". We will need to extract the tests to reproduce outside PVA.

Hi @tracyboehrer, the reverted PR was related to another issue (#6430). Which issue is causing problems with PVA?

ceciliaavila avatar Sep 08 '22 14:09 ceciliaavila

Ah. I reopen the other then and reclose this.

I don't have the exact steps to reproduce outside PVA yet. Since you don't have access to that code, we'll have to bring those tests into SDK somehow.

tracyboehrer avatar Sep 08 '22 15:09 tracyboehrer