fix race conditions in transaction handling#87
fix race conditions in transaction handling#87maxbachmann wants to merge 1 commit intostrichliste:masterfrom
Conversation
|
I did see there is new work happening in https://github.com/foosinn/strichliste/tree/main What is the state of this? The current version of both the backend and frontend doesn't build/run on my development machine anymore. It looks like at least PHP 8.4 is supported there. |
| $sender = null; | ||
| $recipient = null; | ||
| if ($recipientId) { | ||
| if ($senderId < $recipientId) { |
There was a problem hiding this comment.
Hm.... that senderId < recipientId doesnt make much sense, right? :D Why shoild that influence the order of getting the sender and recipient?
There was a problem hiding this comment.
That's the same as the id sort in the revert and done for running:
doTransaction(user1, ..., user2):
and
doTransaction(user2, ..., user1):
at the same time. When locking both user1 and user2 without the ordering this could lead to a deadlock where
the first call locks user1 and the second call locks user2. Both wouldn't be able to lock the other user.
By sorting the users by their id you get a defined locking order that prevents this deadlock.
We started working on making strichliste compatible with php8.x and also updating symfony to the latest version. additionally there is a docker container with frankenphp. but it's all WIP |
maxbachmann
force-pushed
the
patch-3
branch
2 times, most recently
from
874cd14 to
55a7ae9
Compare
| $transactions = []; | ||
| foreach ($transactionIds as $id) { | ||
| $trans = $this->entityManager->getRepository(Transaction::class)->find($id, LockMode::PESSIMISTIC_WRITE); | ||
| if (!$trans) { | ||
| throw new TransactionNotFoundException($id); | ||
| } | ||
|
|
||
| $transactions[] = $trans; | ||
| } | ||
|
|
||
| foreach ($useIds as $id) { | ||
| $user = $this->entityManager->getRepository(User::class)->find($id, LockMode::PESSIMISTIC_WRITE); | ||
| if (!$user) { | ||
| throw new UserNotFoundException($id); | ||
| } |
There was a problem hiding this comment.
I assume you could combine those in less sql queries to make things faster. Not that it likely matters all that much for this project.
|
This is now also roughly tested so it doesn't immediately fall apart. The error paths aren't tested. |
We are currently planning to redo our setup for a couple of reasons:
While working on this I noticed that nothing runs on fedora 42 anymore:
Is there any expected time frame for the 8.4 changes whether it's worth waiting for them or setting up a docker with an old php version? |
2 participants
This is a more complete version of #86. Take this with a grain of salt since I am no php programmer and not regularly working with dbs as well:
The change set does:
Still broken is: