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

Added Locking Mechanism to Reminders Handler #7895

Merged
merged 9 commits into from
Feb 19, 2021

Conversation

b-quachtran
Copy link
Contributor

@b-quachtran b-quachtran commented Feb 5, 2021

Proposed changes:

  • Added lock_store as an input to MessageProcessor so that handle_reminder can properly lock the conversation
  • Updated handle_reminder to lock the conversation before retrieving the tracker store & executing
  • Updated default_processor to pass in InMemoryLockStore as a param for test cases
  • Resolves Issue Reminder Events Don't Acquire a Conversation Lock #7887

Status (please check what you already did):

  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@b-quachtran b-quachtran requested a review from wochinge February 5, 2021 21:07
@b-quachtran
Copy link
Contributor Author

@wochinge I added lock_store as an input parameter to MessageProcessor so that handle_reminder could lock the conversation, but not entirely sure if this is the best way to do it. Wdyt?

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Thanks for providing the fix! Will you also implement that for 2.x?

rasa/core/processor.py Outdated Show resolved Hide resolved
rasa/core/processor.py Show resolved Hide resolved
@b-quachtran
Copy link
Contributor Author

@wochinge Yes, definitely. I'll create a PR for 2.X as well once this fix is good to go

@b-quachtran b-quachtran requested a review from wochinge February 13, 2021 02:24
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

did you also test this manually? If so, how?

tests/core/test_processor.py Outdated Show resolved Hide resolved
rasa/core/agent.py Show resolved Hide resolved
@b-quachtran
Copy link
Contributor Author

@wochinge Yes, I tested manually using the ReminderBot with redis-server running locally and the SocketIO channel. Redis logs showed the lock correctly being acquired and deleted when the reminder was handled.

@b-quachtran b-quachtran requested a review from wochinge February 17, 2021 00:22
changelog/7895.bugfix.md Outdated Show resolved Hide resolved
@wochinge
Copy link
Contributor

Nice job 💯

@b-quachtran b-quachtran merged commit 54cc0f9 into 1.10.x Feb 19, 2021
@b-quachtran b-quachtran deleted the reminders-lock-fix-1-10-x branch February 19, 2021 01:02
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.

2 participants