Blasman / mIRC-Twitch-Scripts

Various scripts and games to use with a mIRC bot designed for Twitch.tv
30 stars 15 forks source link

Improve adding / removing points avoiding race conditions #1

Closed patrickdappollonio closed 8 years ago

patrickdappollonio commented 8 years ago

Hey Blasman!

I just took a look to a bit of your source code. I want to suggest an improvement. The aliases addpoints and removepoints are susceptible to a race conditions. Let me explain. Let's say someone is playing a game in your bot, but also the moment where Ankhbot update everyone's points is about to come. Say you executed the addpoints function, so you're executing ankhbot.mrc line 37 that says:

var %sql_points = SELECT Points FROM CurrencyUser WHERE Name = ' $+ $1 $+ ' COLLATE NOCASE
var %request_points = $sqlite_query(%ankhbot_currency, %sql_points)

Now let's assume the moment the points are read by $sqlite_query then Ankhbot starts updating anybody points because it's time (giving x points every y minutes in chat). Then your code addpoints will have the old point amount and not the new, updated one. So the moment the code reaches this moment...

SET %newpoints = $calc(%ankhbot_points + $2)
sqlite_exec %ankhbot_currency UPDATE CurrencyUser SET Points %newpoints WHERE Name = ' $+ $1 $+ ' COLLATE NOCASE

You would've done the math by mistake with the old number. Same thing might happen in other cases (say, you're paying points from game A but you're also removing points from game B). If it's a small amount, then it's fine, I guess 50 points give or take could be simple, but if it is a big amount, then you'll be returning him plus the extra points or deleting him with the given amount as well.

I know the odds of this to happen are really close to zero, but it happens. This is how you can get Starbucks unlimited refills with a single gift card by redeeming it several times in the same second or so on.

To solve it? Simple: You will save a few lines. SQLite accepts something like this:

update CurrencyUser SET Points = Points + 1000 WHERE Name = 'PatrickDappollonio'

Same for deletion, you'te telling SQLite: "Add to whatever amount of points this guy has, plus 1000". You can rest, add and pretty much any math calculus there. If the user doesn't exists, the code should return that zero users were edited...

update CurrencyUser SET Points = Points + 1000 WHERE Name = 'PatrickDappollonioDoesntExists'

It will update no one, since that user PatrickDappollonioDoesntExists doesn't indeed exists.

Have a great day!

Blasman commented 8 years ago

Thanks, Patrick! It's fixed now! :D

patrickdappollonio commented 8 years ago

Great! You see that every issue has a number? This, for example is "1". In your commit messages you wrote "fixed race condition", you can write "fixed race condition, closes #1" and it will be linked here (in the issue) and there (in the commit), then you know which commit corresponds to which issue. 😁

Have a great afternoon Mr Hacker!

Blasman commented 8 years ago

Thanks! I am still learning! 😁