From 35de5fbe64624091c0742c28f811a0ea6da7cd4a Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 3 Jan 2021 15:52:22 -0800 Subject: HelpChannels: fix race condition between claiming and unclaiming The race condition is when a user claims a channel while their other channel is being unclaimed. Specifically, it's while their cooldown is being removed. The lock ensures that either the cooldown will be re-applied after it's removed or that it won't be removed since `unclaim_channel` will see the user has another claimed channel. --- bot/exts/help_channels/_cog.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/bot/exts/help_channels/_cog.py b/bot/exts/help_channels/_cog.py index bc42b5c2a..7ebec675a 100644 --- a/bot/exts/help_channels/_cog.py +++ b/bot/exts/help_channels/_cog.py @@ -90,6 +90,7 @@ class HelpChannels(commands.Cog): @lock.lock_arg(NAMESPACE, "message", attrgetter("channel.id")) @lock.lock_arg(NAMESPACE, "message", attrgetter("author.id")) + @lock.lock_arg(f"{NAMESPACE}.unclaim", "message", attrgetter("author.id"), wait=True) async def claim_channel(self, message: discord.Message) -> None: """ Claim the channel in which the question `message` was sent. @@ -360,7 +361,20 @@ class HelpChannels(commands.Cog): `caller` is used to track stats on how `channel` was unclaimed (either 'auto' or 'command'). """ - claimant_id = await _caches.claimants.pop(channel.id) + claimant_id = await _caches.claimants.get(channel.id) + coroutine = self._unclaim_channel(channel, claimant_id, caller) + + # It could be possible that there is no claimant cached. In such case, it'd be useless and + # possibly incorrect to lock on None. Therefore, the lock is applied conditionally. + if claimant_id is not None: + decorator = lock.lock_arg(f"{NAMESPACE}.unclaim", "claimant_id", wait=True) + coroutine = decorator(coroutine) + + return await coroutine + + async def _unclaim_channel(self, channel: discord.TextChannel, claimant_id: int, caller: str) -> None: + """Actual implementation of `unclaim_channel`. See that for full documentation.""" + await _caches.claimants.delete(channel.id) # Ignore missing tasks because a channel may still be dormant after the cooldown expires. if claimant_id in self.scheduler: -- cgit v1.2.3