Atomic Locks not working properly?

My site is having issues with Race Conditions, and I've tried using atomic locks to prevent such issues - however the locks just aren't functioning for me in most cases at all. Allowing the race conditions to continue and for the scripts to be exploitable, which is not ideal.

For example:

$lock = Cache::lock('withdraw_bank_' . Auth::id(), 10);

		if(!$lock->get())
		{
			return redirect()->back()->withErrors(['withdrawal' => 'Moving too quickly for the bank teller to properly help you. Slow down.']);
		}

		$user = Auth::user();

		$validate = $request->validate([
			'withdrawal' => ['integer', 'min:0', 'max:' . $user->bank_balance, function($attribute, $value, $fail) use ($user) {
				if($user->times_withdrawn >= 15) {
					$fail('You\'ve already withdrawn enough times today!');
				}
			}]
		]);

		$user->bank_balance -= $request->post('withdrawal');
		$currency_control->addCurrency(Auth::user(), 1, $request->post('withdrawal'));
		$user->times_withdrawn++;
		$user->save();
		$lock->release();

		return redirect()->back();

To my knowledge this should make the code atomic - and protect against race conditions. So when multiple requests are sent to withdraw the balance, the validations won't be out of date and cause issues.

However when I test it (sending roughly 25 requests in at the same time) it fails and ends up withdrawing multiple times - causing balance duplication.

Sometimes it seems to work - giving the error as it properly should, however even one slip up causes an issue so I'm wondering if i'm just not coding it in correctly or if it's a server setting issue or something. Would love if someone could shed some light on this.

I am using a NTS PHP as that's what the server LAMP stack came with, and I was hoping to avoid having to rebuild it into a TS PHP as it might break things and I'm not too good on server ops myself, so if anyone could point out where I'm going wrong or any other solutions it would be amazing. Thank you.

Melancholy

4 years ago

Level 1

I have also tried this way of doing it and still have the same issue (2 requests at least go through successfully.)

$lock = Cache::lock('withdraw_bank_' . Auth::id(), 10);

		try {
			$lock->block(5);

			$user = Auth::user();

			$validate = $request->validate([
				'withdrawal' => ['integer', 'min:0', 'max:' . $user->bank_balance, function($attribute, $value, $fail) use ($user) {
					if($user->times_withdrawn >= 15) {
						$fail('You\'ve already withdrawn enough times today!');
					}
				}]
			]);

			$user->bank_balance -= $request->post('withdrawal');
			$currency_control->addCurrency(Auth::user(), 1, $request->post('withdrawal'));
			$user->times_withdrawn++;
			$user->save();

			return redirect()->back();
		} catch (LockTimeoutException $e) {
			return redirect()->back()->withErrors(['withdrawal' => 'Moving too quickly for the bank teller to properly help you. Slow down.']);
		} finally {
			optional($lock)->release();
		}

If anyone needs any more information (like laravel ver, php ver, etc...) just ask. I'm new to the whole asking for help via forums I'm just kind of desperate to fix this.

Funfare

4 years ago

Best Answer

Level 13

I think the problem could be, that the user is loaded on the beginning of the request with the old bank balance. Try to load the User from DB after getting the lock: $user = Auth::user()->fresh();

1 like

Melancholy

4 years ago

Level 1

@Funfare I think that did the trick, after a few tests! I did also try some other things so I'm not sure if they helped it too. Thank you!

Alexzz

2 years ago

Level 1

@Melancholy please how were you able to test locally I have the same problem

Please or to participate in this conversation.