search

dgurkaynak / slack-poker-planner

Poker planning app for Slack
https://deniz.co/slack-poker-planner
MIT License
229 stars 36 forks source link

Race condition when registering votes #31

Closed loehnertz closed 3 years ago

loehnertz commented 3 years ago

We are using the bot weekly in remote pokering in these current times. We overall really like it 👍🏻

However, since the latest release, we are struck by a severe race condition when pokering. It seems like the bot only registers a single vote at a time, overwriting the internal Session::votes state in some way. The effect is also visually noticeable: When the votes come in in quick succession, the checkmarks behind the names of other people that already voted disappear again. The only solution for now is just to try to get the vote into the state by continuously retrying until one's own vote won the current race. This makes it a bit annoying.

I will investigate this myself a bit. It was definitely introduced in one of the last few commits (I think in the latest release). I'll update this issue if I find something and possibly turn in a PR.

loehnertz commented 3 years ago

Pretty surely it comes from commit 4c740e.

dgurkaynak commented 3 years ago

Hmmm, yeah, you're right, there is a chance of race condition, definitely. But I think the commit you mentioned has nothing to do with it. There's always been a chance of race condition, even before that commit. Just when an in-memory database is used, there is no chance of race condition. However, I'm using Redis in production. There is always a network-delay; in case of race condition, the latest request wins.

Feel free to look it up @loehnertz, really appreciated for that. But I guess this issue is a tough one 😅

loehnertz commented 3 years ago

Yeah, I think it needs a bigger refactor then to upsert the votes separately from the entire state or do it transactionally. If I can find time, I'll look into it soon!

dgurkaynak commented 3 years ago

I've just released v1.1.2 and deployed to production, which should fix race condition 🚀 Closing this issue, let me know if the problem persists.