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

[botbuilder]TeamsActivityHandler — Events are called twice #4202

Closed
alexrecuenco opened this issue Apr 25, 2022 · 5 comments · Fixed by #4203
Closed

[botbuilder]TeamsActivityHandler — Events are called twice #4202

alexrecuenco opened this issue Apr 25, 2022 · 5 comments · Fixed by #4203
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

@alexrecuenco
Copy link
Contributor

Github issues should be used for bugs and feature requests. Use Stack Overflow for general "how-to" questions.

Versions

Current main branch (commit 08b73c4)
What package version of the SDK are you using.

❯ node --version
v14.19.0

OS: MAC OS

Describe the bug

TeamsActivityHandler, when an activity is received, calls each handler added by onEvent(handler) twice

To Reproduce

  1. Self explanatory, add an event, call it

Expected behavior

Event handlers should be called once

@alexrecuenco alexrecuenco 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 Apr 25, 2022
alexrecuenco added a commit to alexrecuenco/botbuilder-js that referenced this issue Apr 25, 2022
Fixes microsoft#4202

- teamsActivityHandler overwrites `dispatchEventActivity`
- `dispatchEventActivity` is only called from within `onEventActivity`
- `onEventActivity` already calls all handlers
- on teamsActivityHandler, when overwriting, previously it was calling the array of handlers again before dispatching

A test is added to prevent the behavior from reappearing

Note that to test the behavior issue, it is enough to apply the test without the change to code to main.
The test fails in that case (since it is called twice)
alexrecuenco added a commit to alexrecuenco/botbuilder-js that referenced this issue Apr 25, 2022
Fixes microsoft#4202

 Description

This change prevents calling TeamsActivityHandler event handlers twice for every event received.

 Specific Changes
- teamsActivityHandler overwrites `dispatchEventActivity`
- `dispatchEventActivity` is only called from within `onEventActivity`
- `onEventActivity` already calls all handlers
- on teamsActivityHandler, when overwriting, previously it was calling the array of handlers again before dispatching

 Testing

A test is added to prevent the behavior from reappearing

Note that to test the issue described, it is enough to run the changed test in main.
The test will fail when run in main currently, since since it is called twice.
alexrecuenco added a commit to alexrecuenco/botbuilder-js that referenced this issue Apr 25, 2022
Fixes microsoft#4202

 Description

This change prevents calling TeamsActivityHandler event handlers twice for every event received.

 Specific Changes

Why were the event handlers getting called twice?

- teamsActivityHandler overwrites `dispatchEventActivity`
- `dispatchEventActivity` is only called from within `onEventActivity`
- `onEventActivity` already dispatches *after* calling all handlers
- on teamsActivityHandler, when overwriting, it is also calling all handlers

How to fix it?

- Prevent the handler call inside the dispatchEventHandler
- This should have no effect on any consumer, since they should already be consuming only from within a onEventActivity call.

 Testing

A test is added to prevent the behavior from reappearing

Note that to test the issue described, it is enough to run the changed test in main.
The test will fail when run in main currently, since since it is called twice.
@alexrecuenco
Copy link
Contributor Author

alexrecuenco commented Apr 25, 2022

The best way to test this issue is to use the PR #4203 and remove the code change:

The test included in that PR will fail without the change included in that PR

@ramfattah
Copy link
Contributor

ramfattah commented Apr 26, 2022

Hey @alexrecuenco,

I'm able to reproduce. I ran the /teamsActivityHandler.test.js / unit test in current main branch and the onEvent method is being called twice.

it('should not call middleware handler twice', async function () {

Screenshot:
image

@ramfattah
Copy link
Contributor

I'm not sure if this is by design or a bug within the JS SDK.
I will discuss this with the SDK engineering team today.

@ramfattah ramfattah removed the needs-triage The issue has just been created and it has not been reviewed by the team. label Apr 26, 2022
@ramfattah
Copy link
Contributor

ramfattah commented Apr 27, 2022

Update:

I attempted to repro on the .NET SDK, and it looks like the OnEventActivityAsync method for TeamsActivityHandler is called once.

Here are the steps I took:

  1. Cloned and built the botbuilder-dotnet repo on my local machine environment

  2. Navigate to /TeamsActivityHandlerTests.cs / test file

  3. In TestActivityHandler class, I created getter and setter property set to 0 by default, to count the number of calls for OnEventActivityAsyn

       private class TestActivityHandler : TeamsActivityHandler
       {
    +      public int NumberOfOnEventCalls { get; set; } = 0;
    
           public List<string> Record { get; } = new List<string>();
    
  4. Then, in OnEventActivityAsync method, I increment the NumberOfOnEventCalls property by one if it gets called.

           protected override Task OnEventActivityAsync(ITurnContext<IEventActivity> turnContext, CancellationToken cancellationToken)
           {
               Record.Add(MethodBase.GetCurrentMethod().Name);
    +          NumberOfOnEventCalls++;
               return base.OnEventActivityAsync(turnContext, cancellationToken);
           }
  5. Ran the TestOnEventActivity() test case, and debugged the NumberOfOnEventCalls property. It seems it was only called once.

    image

Also, I created a temporary branch of my changes here microsoft/botbuilder-dotnet@1d18e1f

@alexrecuenco
Copy link
Contributor Author

Thank you for your effort verifying the inconsistency. I hope #4203 is a sufficient solution.

@msomanathan msomanathan self-assigned this Apr 27, 2022
@msomanathan msomanathan added the customer-replied-to Indicates that the team has replied to the issue reported by the customer. Do not delete. label Apr 28, 2022
@lauren-mills lauren-mills 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 May 2, 2022
alexrecuenco added a commit to alexrecuenco/botbuilder-js that referenced this issue May 28, 2022
Fixes microsoft#4202

 Description

This change prevents calling TeamsActivityHandler event handlers twice for every event received.

 Specific Changes

Why were the event handlers getting called twice?

- teamsActivityHandler overwrites `dispatchEventActivity`
- `dispatchEventActivity` is only called from within `onEventActivity`
- `onEventActivity` already dispatches *after* calling all handlers
- on teamsActivityHandler, when overwriting, it is also calling all handlers

How to fix it?

- Prevent the handler call inside the dispatchEventHandler
- This should have no effect on any consumer, since they should already be consuming only from within a onEventActivity call.

 Testing

A test is added to prevent the behavior from reappearing

Note that to test the issue described, it is enough to run the changed test in main.
The test will fail when run in main currently, since since it is called twice.
@gabog gabog assigned EricDahlvang and unassigned msomanathan Jun 1, 2022
EricDahlvang pushed a commit that referenced this issue Jun 8, 2022
… handlers twice (#4203)

* [botbuilder] TeamsActivityHandler — Prevent calling event handlers twice

Fixes #4202

 Description

This change prevents calling TeamsActivityHandler event handlers twice for every event received.

 Specific Changes

Why were the event handlers getting called twice?

- teamsActivityHandler overwrites `dispatchEventActivity`
- `dispatchEventActivity` is only called from within `onEventActivity`
- `onEventActivity` already dispatches *after* calling all handlers
- on teamsActivityHandler, when overwriting, it is also calling all handlers

How to fix it?

- Prevent the handler call inside the dispatchEventHandler
- This should have no effect on any consumer, since they should already be consuming only from within a onEventActivity call.

 Testing

A test is added to prevent the behavior from reappearing

Note that to test the issue described, it is enough to run the changed test in main.
The test will fail when run in main currently, since since it is called twice.

* onConversationUpdate also has a double event issue

Per @johnataylor suggestions, it seems that the ConversationUpdate activity type had a similar issue to onEvent

The tests and the code are adjusted accordingly to reflect this.

Other event types were reviewed and it doesn't seem that they have a similar issue.
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
5 participants