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

Fix race condition in dispatcher start/stop #887

Merged
merged 3 commits into from Oct 21, 2017

Conversation

Projects
None yet
3 participants
@tsnoam
Member

tsnoam commented Oct 21, 2017

Fixes #881

@jsmnbom

LGTM, seems like a simple fix.
Can we test it?

Args:
ready (:obj:`threading.Event`, optional): If specified, the event will be set once the
dispatcher is ready.
"""
if self.running:

This comment has been minimized.

@jh0ker

jh0ker Oct 21, 2017

Member

Shouldn't we do ready.set() here as well?

@jh0ker

jh0ker Oct 21, 2017

Member

Shouldn't we do ready.set() here as well?

This comment has been minimized.

@tsnoam

tsnoam Oct 21, 2017

Member

that makes sense. i'll fix.

@tsnoam

tsnoam Oct 21, 2017

Member

that makes sense. i'll fix.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 21, 2017

Codecov Report

Merging #887 into master will decrease coverage by 0.18%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #887      +/-   ##
=========================================
- Coverage   91.89%   91.7%   -0.19%     
=========================================
  Files         101     101              
  Lines        4057    4063       +6     
  Branches      621     623       +2     
=========================================
- Hits         3728    3726       -2     
- Misses        194     198       +4     
- Partials      135     139       +4
Flag Coverage Δ
#Appveyor 86.85% <75%> (-0.03%) ⬇️
#Travis 91.28% <75%> (-0.19%) ⬇️
Impacted Files Coverage Δ
telegram/ext/updater.py 76.92% <100%> (+0.25%) ⬆️
telegram/ext/dispatcher.py 91.81% <60%> (-1.01%) ⬇️
telegram/message.py 96.22% <0%> (-0.76%) ⬇️
telegram/bot.py 87.22% <0%> (-0.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ed0599...086f1b8. Read the comment docs.

codecov bot commented Oct 21, 2017

Codecov Report

Merging #887 into master will decrease coverage by 0.18%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #887      +/-   ##
=========================================
- Coverage   91.89%   91.7%   -0.19%     
=========================================
  Files         101     101              
  Lines        4057    4063       +6     
  Branches      621     623       +2     
=========================================
- Hits         3728    3726       -2     
- Misses        194     198       +4     
- Partials      135     139       +4
Flag Coverage Δ
#Appveyor 86.85% <75%> (-0.03%) ⬇️
#Travis 91.28% <75%> (-0.19%) ⬇️
Impacted Files Coverage Δ
telegram/ext/updater.py 76.92% <100%> (+0.25%) ⬆️
telegram/ext/dispatcher.py 91.81% <60%> (-1.01%) ⬇️
telegram/message.py 96.22% <0%> (-0.76%) ⬇️
telegram/bot.py 87.22% <0%> (-0.52%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ed0599...086f1b8. Read the comment docs.

@jh0ker

jh0ker approved these changes Oct 21, 2017

Love it :)

@tsnoam

This comment has been minimized.

Show comment
Hide comment
@tsnoam

tsnoam Oct 21, 2017

Member

@bomjacob I don't think that we can properly test it.
We can pass an Event object to dispatcher.start() but what will it tell us? Only that we've called set() on the Event. It won't guarantee that it had been done properly.
So basically, I don't think that testing this is worth the effort.

Edit: properly = in proper order

Member

tsnoam commented Oct 21, 2017

@bomjacob I don't think that we can properly test it.
We can pass an Event object to dispatcher.start() but what will it tell us? Only that we've called set() on the Event. It won't guarantee that it had been done properly.
So basically, I don't think that testing this is worth the effort.

Edit: properly = in proper order

@tsnoam tsnoam merged commit 4b3315d into master Oct 21, 2017

3 of 5 checks passed

codecov/patch 75% of diff hit (target 91.89%)
Details
codecov/project 91.7% (-0.19%) compared to 3ed0599
Details
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tsnoam tsnoam deleted the fix_881 branch Oct 21, 2017

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