Skip to content

Fix race condition between bot and user activity#3705

Merged
compulim merged 7 commits intomicrosoft:masterfrom
compulim:fix-3431
Feb 5, 2021
Merged

Fix race condition between bot and user activity#3705
compulim merged 7 commits intomicrosoft:masterfrom
compulim:fix-3431

Conversation

@compulim
Copy link
Copy Markdown
Contributor

@compulim compulim commented Feb 4, 2021

Fixes #3431.

Changelog Entry

Fixed

  • Fixes #3431. Race condition between the first bot activity and first user activity, should not cause the first bot activity to be delayed, by @compulim in PR #3705

Description

A race condition between the first bot activity and first user activity will cause the bot activity to be delayed if the user activity shows up first.

Design

When the bot activity (or any activity with "replyToId") has arrived, we will check if its precedence is already in the transcript or not. If it is not, we will wait for up to 5 seconds. This is for resolving an accessibility issue due to protocol/SDK deficiency.

But since the first bot activity will have a "replyToId" pointing to a bogus activity (a.k.a. never exists), thus, the bot activity will always subject to delay.

To mitigate this issue, we will check the transcript to see if the bot activity is the very first (i.e. with a bogus activity). If it is the very first, then, we allow it to show up without delay.

The problem lies in our transcript check; we check for all kinds of activities, including activities from both bot and user. Due to the race condition, the user's activity is already in the transcript. Thus, the bot activity is not the "first"; we don't allow the first bot activity to show up immediately.

The check's primary object is to check if the "replyToId" is a bogus ID or not. The check should be true if the transcript does not have any bot activities, instead of any activities.

Specific Changes

  • Updated the transcript check in queueIncomingActivitySaga.js, we should only check for bot's activities, not every activity
  • Added tests to prevent regression
  • I have added tests and executed them locally
  • I have updated CHANGELOG.md
  • I have updated documentation

Review Checklist

This section is for contributors to review your work.

  • Accessibility reviewed (tab order, content readability, alt text, color contrast)
  • Browser and platform compatibilities reviewed
  • CSS styles reviewed (minimal rules, no z-index)
  • Documents reviewed (docs, samples, live demo)
  • Internationalization reviewed (strings, unit formatting)
  • package.json and package-lock.json reviewed
  • Security reviewed (no data URIs, check for nonce leak)
  • Tests reviewed (coverage, legitimacy)

Comment thread CHANGELOG.md Outdated
Comment thread packages/core/src/sagas/queueIncomingActivitySaga.js
Comment thread packages/testharness/src/index.js
Comment thread __tests__/html/accessibility.delayActivity.withReplyToId.html Outdated
@corinagum
Copy link
Copy Markdown
Contributor

Might want to change 'bogus' to 'non-existent' - I don't know if this is a polycheck violation or not.

Copy link
Copy Markdown
Contributor

@corinagum corinagum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending a few comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

First welcome message is delayed for 5 seconds

2 participants