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

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

Closed
Rich-biomni opened this issue Aug 1, 2022 · 11 comments · Fixed by #6448 or #6466
Assignees
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.

Comments

@Rich-biomni
Copy link

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 Rich-biomni 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 Aug 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 Aug 1, 2022
@LeeParrishMSFT
Copy link
Contributor

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 LeeParrishMSFT added the customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. label Aug 1, 2022
@stevkan stevkan self-assigned this Aug 2, 2022
@JamesSinclairBiomni
Copy link

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

@stevkan
Copy link
Collaborator

stevkan commented Aug 5, 2022

@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 stevkan removed the needs-triage The issue has just been created and it has not been reviewed by the team. label Aug 5, 2022
@Rich-biomni
Copy link
Author

@stevkan - Dependency removed

SimpleBot.zip

@stevkan
Copy link
Collaborator

stevkan commented Aug 5, 2022

@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
Copy link
Collaborator

stevkan commented Aug 9, 2022

@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 stevkan assigned ceciliaavila and unassigned stevkan Aug 9, 2022
@tracyboehrer
Copy link
Member

Is this repeatable in 4.17.1?

@Rich-biomni
Copy link
Author

@tracyboehrer

Issue exists in 4.17.1 as well

tracyboehrer pushed a commit that referenced this issue Aug 23, 2022
… ContinueDialogAsync to be recalled (#6448)

* Reacquire childDialogState after ContinueDialogAsync

* Add unit test
tracyboehrer pushed a commit that referenced this issue Aug 24, 2022
… ContinueDialogAsync to be recalled (#6448)

* Reacquire childDialogState after ContinueDialogAsync

* Add unit test
tracyboehrer added a commit that referenced this issue Sep 1, 2022
* README version to 4.18 (#6425)

Co-authored-by: Tracy Boehrer <trboehre@microsoft.com>

* [#6434] Priority broken for RegexRecognizer (#6435)

* Consider priority in OnRecognizeAsync method

* Add unit test

* Update Bool function to use Convert.ToBoolean (#6431)

* Update IsMatch empty string values (#6426)

* [#6428] Adaptive ForEachElement loop's incorrectly when changes cause ContinueDialogAsync to be recalled (#6448)

* Reacquire childDialogState after ContinueDialogAsync

* Add unit test

* Support passing sas token url's for token service (#6449)

Co-authored-by: Swagat Mishra <swagatm@microsoft.com>

* Bump @actions/core from 1.6.0 to 1.9.1 in /actions/verify-pr-labels (#6445)

Bumps [@actions/core](https://github.com/actions/toolkit/tree/HEAD/packages/core) from 1.6.0 to 1.9.1.
- [Release notes](https://github.com/actions/toolkit/releases)
- [Changelog](https://github.com/actions/toolkit/blob/main/packages/core/RELEASES.md)
- [Commits](https://github.com/actions/toolkit/commits/HEAD/packages/core)

---
updated-dependencies:
- dependency-name: "@actions/core"
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Support passing sas token url's for token service (#6449)

Co-authored-by: Swagat Mishra <swagatm@microsoft.com>

* [#6428] Adaptive ForEachElement loop's incorrectly when changes cause ContinueDialogAsync to be recalled (#6448)

* Reacquire childDialogState after ContinueDialogAsync

* Add unit test

* Delete vnext code which is dead (#6440)

* Fix PublishToCoveralls.ps1 (#6439)

* Remove failing Powershell patch download

* Upgrade coveralls.net to 4.0.1

* [#6430] Adaptive ForEachElement does not exit when child action CancelAllDialogs is called (#6452)

* Consider canceled status to end the dialog

* Add unit test

* [#6432] TeamsInfo.GetMemberAsync(...) doesn't work properly in Skill Bot scenario, it returns http 405 error (#6443)

* Implement GetMemberAsync for skills

* Add unit test

* [#6433] Error in AdaptiveDialog.ContinueActionAsync with native dialog SDK (#6444)

* Add condition before loading the resource

* Add unit tests

* Load AdaptiveDialogs dynamically on DialogContext

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Tracy Boehrer <trboehre@microsoft.com>
Co-authored-by: Cecilia Avila <44245136+ceciliaavila@users.noreply.github.com>
Co-authored-by: Joel Mut <62260472+sw-joelmut@users.noreply.github.com>
Co-authored-by: swagat mishra <swagatmishra2007@gmail.com>
Co-authored-by: Swagat Mishra <swagatm@microsoft.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: BruceHaley <v-brucehaley@microsoft.com>
tracyboehrer pushed a commit that referenced this issue Sep 6, 2022
…es cause ContinueDialogAsync to be recalled (#6448)"

This reverts commit bc5d708.
tracyboehrer pushed a commit that referenced this issue Sep 7, 2022
…y when changes cause ContinueDialogAsync to be recalled (#6448)
@tracyboehrer tracyboehrer reopened this Sep 7, 2022
@tracyboehrer
Copy link
Member

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.

@ceciliaavila
Copy link
Collaborator

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?

@tracyboehrer
Copy link
Member

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.

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.
Projects
None yet
7 participants