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

Fix locking issue and optimize a bit #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

suutari-ai
Copy link
Member

Fix an issue with locking by creating just a single PePuRunner on the
process start up in run_pepubot function.

Previously a new PePuRunner was created for each incoming Slack message
and the locking of the shared resources (e.g. runner state and the
lottery box) was done with an instance attribute of the
PePuRunner. Therefore there was actually a different lock for each
PePuRunner and the locking wasn't able to protect two distinct message
processing flows to mess up each other.

Also add an optimization that skips the state loading when its already
loaded.

Fixes #2

Fix an issue with locking by creating just a single PePuRunner on the
process start up in run_pepubot function.

Previously a new PePuRunner was created for each incoming Slack message
and the locking of the shared resources (e.g. runner state and the
lottery box) was done with an instance attribute of the PePuRunner.
Therefore there was actually a different lock for each PePuRunner and
the locking wasn't able to protect two distinct message processing flows
to mess up each other.
There is no need to load the state of the PePuRunner more than once.
Now that there is only a single PePuRunner instantiated, this actually
makes a difference too.
@codecov
Copy link

codecov bot commented Oct 13, 2019

Codecov Report

Merging #3 into master will decrease coverage by 0.45%.
The diff coverage is 30.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #3      +/-   ##
==========================================
- Coverage   34.16%   33.71%   -0.46%     
==========================================
  Files          11       11              
  Lines         518      522       +4     
  Branches       74       75       +1     
==========================================
- Hits          177      176       -1     
- Misses        340      345       +5     
  Partials        1        1
Impacted Files Coverage Δ
pepubot/__main__.py 38.7% <28.57%> (-6.46%) ⬇️
pepubot/runner.py 23.59% <33.33%> (+0.03%) ⬆️

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 1b3f9a6...2cf4fb8. Read the comment docs.

@suutari-ai suutari-ai requested a review from frwickst October 13, 2019 09:06
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.

The locking is done incorrectly is PePuRunner
1 participant