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

Add counter to onError trigger to break possible loops #6776

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,16 @@
"$role": [ "implements(Microsoft.ITrigger)", "extends(Microsoft.OnCondition)" ],
"title": "On error",
"description": "Action to perform when an 'Error' dialog event occurs.",
"type": "object"
"type": "object",
"properties": {
"executionLimit": {
"$ref": "schema:#/definitions/numberExpression",
"title": "Execution limit",
"description": "Number of executions allowed for this trigger. Used to break loops in case of errors inside the trigger.",
"examples": [
3,
"=f(x)"
]
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
"form": {
"order": [
"condition",
"*"
"*",
"executionLimit"
],
"hidden": [
"actions"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@

using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using AdaptiveExpressions.Properties;
using Newtonsoft.Json;

namespace Microsoft.Bot.Builder.Dialogs.Adaptive.Conditions
Expand Down Expand Up @@ -31,6 +33,26 @@ public OnError(List<Dialog> actions = null, string condition = null, [CallerFile
{
}

/// <summary>
/// Gets or sets the number of executions allowed. Used to avoid infinite loops in case of error.
/// </summary>
/// <value>The number of executions allowed for this trigger.</value>
[JsonProperty("executionLimit")]
public NumberExpression ExecutionLimit { get; set; } = new NumberExpression();

/// <summary>
/// Method called to execute the rule's actions.
/// </summary>
/// <param name="actionContext">Context.</param>
/// <returns>A <see cref="Task"/> with plan change list.</returns>
public override Task<List<ActionChangeList>> ExecuteAsync(ActionContext actionContext)
{
// 10 is the default number of executions we'll allow before breaking the loop.
var limit = ExecutionLimit.Value > 0 ? ExecutionLimit.Value : 10;
actionContext.State.SetValue(TurnPath.ExecutionLimit, limit);
return base.ExecuteAsync(actionContext);
}

/// <summary>
/// Called when a change list is created.
/// </summary>
Expand Down
24 changes: 24 additions & 0 deletions libraries/Microsoft.Bot.Builder.Dialogs/DialogExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Microsoft.Bot.Builder.Skills;
using Microsoft.Bot.Connector.Authentication;
using Microsoft.Bot.Schema;
using Newtonsoft.Json.Linq;

namespace Microsoft.Bot.Builder.Dialogs
{
Expand Down Expand Up @@ -64,6 +65,8 @@ internal static async Task<DialogTurnResult> InternalRunAsync(ITurnContext turnC
// or we have had an exception AND there was an OnError action which captured the error. We need to continue the
// turn based on the actions the OnError handler introduced.
var endOfTurn = false;
var errorHandlerCalled = 0;

while (!endOfTurn)
{
try
Expand All @@ -79,8 +82,29 @@ internal static async Task<DialogTurnResult> InternalRunAsync(ITurnContext turnC
var innerExceptions = new List<Exception>();
try
{
errorHandlerCalled++;

// fire error event, bubbling from the leaf.
handled = await dialogContext.EmitEventAsync(DialogEvents.Error, err, bubble: true, fromLeaf: true, cancellationToken: cancellationToken).ConfigureAwait(false);

var turnObject = (JObject)turnContext.TurnState["turn"];

var executionLimit = 0;

foreach (var childToken in turnObject.Children<JProperty>())
{
if (childToken.Name == "executionLimit")
{
executionLimit = (int)childToken.Value;
}
}

if (executionLimit > 0 && errorHandlerCalled > executionLimit)
{
// if the errorHandler has being called multiple times, there's an error inside the onError.
// We should throw the exception and break the loop.
handled = false;
}
}
#pragma warning disable CA1031 // Do not catch general exception types (capture the error in case it's not handled properly)
catch (Exception emitErr)
Expand Down
5 changes: 5 additions & 0 deletions libraries/Microsoft.Bot.Builder.Dialogs/Memory/TurnPath.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ public class TurnPath
/// </summary>
public const string ActivityProcessed = "turn.activityProcessed";

/// <summary>
/// Used to limit the execution of a trigger avoiding infinite loops in case of errors.
/// </summary>
public const string ExecutionLimit = "turn.executionLimit";

/// <summary>
/// The result from the last dialog that was called.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,18 @@ public async Task ConditionalsTests_OnChooseIntent()
await TestUtils.RunTestScript(_resourceExplorerFixture.ResourceExplorer);
}

[Fact]
public async Task ConditionalsTests_OnErrorLoop()
{
await TestUtils.RunTestScript(_resourceExplorerFixture.ResourceExplorer);
}

[Fact]
public async Task ConditionalsTests_OnErrorLoopDefaultLimit()
{
await TestUtils.RunTestScript(_resourceExplorerFixture.ResourceExplorer);
}

[Fact]
public void OnConditionWithCondition()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
{
"$schema": "../../../tests.schema",
"$kind": "Microsoft.Test.Script",
"dialog": {
"$kind": "Microsoft.AdaptiveDialog",
"id": "ErrorLoop",
"autoEndDialog": true,
"defaultResultProperty": "dialog.result",
"triggers": [
{
"$kind": "Microsoft.OnBeginDialog",
"actions": [
{
"$kind": "Microsoft.SendActivity",
"activity": "Throw Exception in BeginDialog."
},
{
"$kind": "Microsoft.ThrowException",
"errorValue": "Exception in BeginDialog."
}
]
},
{
"$kind": "Microsoft.OnError",
"actions": [
{
"$kind": "Microsoft.SendActivity",
"activity": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.ThrowException",
"errorValue": "Exception in OnError."
}
],
"executionLimit": 3
}
]
},
"script": [
{
"$kind": "Microsoft.Test.UserSays",
"text": "hi"
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in BeginDialog."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Exception in OnError."
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
{
"$schema": "../../../tests.schema",
"$kind": "Microsoft.Test.Script",
"dialog": {
"$kind": "Microsoft.AdaptiveDialog",
"id": "ErrorLoop",
"autoEndDialog": true,
"defaultResultProperty": "dialog.result",
"triggers": [
{
"$kind": "Microsoft.OnBeginDialog",
"actions": [
{
"$kind": "Microsoft.SendActivity",
"activity": "Throw Exception in BeginDialog."
},
{
"$kind": "Microsoft.ThrowException",
"errorValue": "Exception in BeginDialog."
}
]
},
{
"$kind": "Microsoft.OnError",
"actions": [
{
"$kind": "Microsoft.SendActivity",
"activity": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.ThrowException",
"errorValue": "Exception in OnError."
}
]
}
]
},
"script": [
{
"$kind": "Microsoft.Test.UserSays",
"text": "hi"
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in BeginDialog."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Throw Exception in OnError."
},
{
"$kind": "Microsoft.Test.AssertReply",
"text": "Exception in OnError."
}
]
}
9 changes: 9 additions & 0 deletions tests/tests.schema
Original file line number Diff line number Diff line change
Expand Up @@ -6498,6 +6498,15 @@
}
},
"properties": {
"executionLimit": {
"$ref": "#/definitions/numberExpression",
"title": "Execution limit",
"description": "Number of executions allowed for this trigger. Used to break loops in case of errors inside the trigger.",
"examples": [
3,
"=f(x)"
]
},
"condition": {
"$ref": "#/definitions/condition",
"title": "Condition",
Expand Down
3 changes: 2 additions & 1 deletion tests/tests.uischema
Original file line number Diff line number Diff line change
Expand Up @@ -1166,7 +1166,8 @@
"label": "Error occurred",
"order": [
"condition",
"*"
"*",
"executionLimit"
],
"subtitle": "Error event"
},
Expand Down
Loading