From fef64342f771795e4e09d472a63d85bc74d3a738 Mon Sep 17 00:00:00 2001 From: Tuomas Suutari Date: Sun, 13 Oct 2019 10:16:56 +0300 Subject: [PATCH 1/2] Create just single PePuRunner on start 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. --- pepubot/__main__.py | 23 ++++++++--------------- pepubot/runner.py | 8 ++++++-- 2 files changed, 14 insertions(+), 17 deletions(-) diff --git a/pepubot/__main__.py b/pepubot/__main__.py index e5c45e3..fc53953 100644 --- a/pepubot/__main__.py +++ b/pepubot/__main__.py @@ -2,13 +2,11 @@ import asyncio import logging import sys -from typing import Any, Mapping, Sequence - -import slack +from typing import Any, Sequence +from . import slack from .runner import PePuRunner from .settings import initialize_settings -from .slack import get_rtm_client LOG = logging.getLogger(__name__) @@ -30,24 +28,19 @@ def parse_args(argv: Sequence[str]) -> Any: def run_pepubot() -> None: LOG.info('Initializing the Slack RTM client') - slack_rtm_client = get_rtm_client() + slack_rtm_client = slack.get_rtm_client() + + LOG.info('Creating PePuRunner with a Slack Web client') + slack_web_client = slack.get_web_client() + pepu_runner = PePuRunner(slack_web_client) LOG.info('Binging our event handler') - slack_rtm_client.on(event='message', callback=handle_new_message) + slack_rtm_client.on(event='message', callback=pepu_runner.feed_message) LOG.info('Starting the Slack RTM session in an event loop') loop = asyncio.get_event_loop() loop.run_until_complete(slack_rtm_client.start()) -async def handle_new_message( - *, - data: Mapping[str, Any], - web_client: slack.WebClient, - **kwargs: object, -) -> None: - await PePuRunner(web_client).feed_message(data) - - if __name__ == '__main__': main() diff --git a/pepubot/runner.py b/pepubot/runner.py index 64296f4..08d0478 100644 --- a/pepubot/runner.py +++ b/pepubot/runner.py @@ -28,8 +28,12 @@ def __init__(self, slack_client: slack.WebClient) -> None: self.round_participants: List[str] = [] self._lock = asyncio.Lock() - async def feed_message(self, event_data: Mapping[str, Any]) -> None: - message = Message.from_message_event(event_data) + async def feed_message( + self, + data: Mapping[str, Any], + **kwargs: object, + ) -> None: + message = Message.from_message_event(data) if not message: return From 2cf4fb8ce03676d76ab71bde721ee0fc559ebed2 Mon Sep 17 00:00:00 2001 From: Tuomas Suutari Date: Sun, 13 Oct 2019 10:51:55 +0300 Subject: [PATCH 2/2] PePuRunner: Load only once (optimization) 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. --- pepubot/runner.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pepubot/runner.py b/pepubot/runner.py index 08d0478..2c89cac 100644 --- a/pepubot/runner.py +++ b/pepubot/runner.py @@ -23,6 +23,7 @@ class PePuState(Enum): class PePuRunner: def __init__(self, slack_client: slack.WebClient) -> None: self.slack = slack_client + self._state_loaded: bool = False self.state: PePuState = PePuState.not_started self.start_message: Optional[MessageInfo] = None self.round_participants: List[str] = [] @@ -270,6 +271,9 @@ async def dump_lottery_box(self, message: Message) -> None: channel=message.channel, text=text) async def _load(self) -> None: + if self._state_loaded: + return + storage = get_default_storage() async with self._lock: @@ -285,6 +289,8 @@ async def _load(self) -> None: self.round_participants = (participants or '').splitlines() + self._state_loaded = True + async def _save(self) -> None: storage = get_default_storage() await storage.store_item('runner_state', self.state.value)