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 mutex protection on ConversationHandler #1533

Merged
merged 4 commits into from
Oct 16, 2019

Conversation

SnowyCoder
Copy link
Contributor

Advances #1029 adding thread safety on the mutable variables of the class (conversations and timeout_jobs).
The original issue also asked to internalize all the other properties but it would incur breaking changes and a lot of refactoring, I'll leave this task to someone more proficient with the codebase.

@Poolitzer
Copy link
Member

Poolitzer commented Oct 10, 2019

Hey man, this looks good to me, but there should be definitly someone else to look over this cough cough @Eldinnie? cough cough. Can you remove the #Todo line though and either put it in this conversation or in the related issue? Then codacy should be fine with it.

Copy link
Member

@jh0ker jh0ker left a comment

Choose a reason for hiding this comment

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

If you address the TODO, I think this is good

@SnowyCoder
Copy link
Contributor Author

I've addressed the todo, should I squash the commits?
Sorry for the trouble, I wanted to help you a bit but It's my first time doing a PR in a big repo 😅.

@Poolitzer
Copy link
Member

No need to, we will before merging into master. Just go ahead and make as many commits as you want :)

@jh0ker
Copy link
Member

jh0ker commented Oct 11, 2019

@SnowyCoder Now you have the chance to get the true big repo experience™️ 😉
It would be great to have a test for that last change you made regarding the removal of the timeout job. Basically a test that demonstrates that the timeout job will be removed and not executed, even if you have a child handler that starts before the timeout would run out and ends after.

Refer to https://github.com/python-telegram-bot/python-telegram-bot/blob/master/tests/test_conversationhandler.py line 414 and following for inspiration.

@SnowyCoder
Copy link
Contributor Author

SnowyCoder commented Oct 13, 2019

I've been studying the test code and I noticed something strange: the assignment at line 494 (that has moved to 552 in the master branch) is duplicated, it seems like a merge error or a typo.

@jh0ker
Copy link
Member

jh0ker commented Oct 16, 2019

@SnowyCoder Interesting find ^^
Edit: e75615c introduced it, not quite sure what was going on here...

@jh0ker jh0ker merged commit 3d8771b into python-telegram-bot:master Oct 16, 2019
@jh0ker
Copy link
Member

jh0ker commented Oct 16, 2019

Thanks a lot for your contribution @SnowyCoder!

@SnowyCoder
Copy link
Contributor Author

Thank you for the support!

@github-actions github-actions bot locked and limited conversation to collaborators Aug 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants