Replace database locks with transactions #1701
Conversation
kradalby
force-pushed
the
db-remove-lock-use-tx
branch
from
c914393 to
a757f44
Compare
There was a problem hiding this comment.
I was reviewing the PR in preparation for #1572 (comment), I put down comments I had incase it was useful.
hscontrol/poll.go
Outdated
| @@ -337,7 +359,7 @@ func (h *Headscale) handlePoll( | |||
| // to receive a message to make sure we dont block the entire | |||
| // notifier. | |||
| // 12 is arbitrarily chosen. | |||
| ChangeNodes: types.Nodes{node}, | ||
| } | ||
| if stateSelfUpdate.Valid() { | ||
| hsdb.notifier.NotifyByMachineKey(stateSelfUpdate, node.MachineKey) |
There was a problem hiding this comment.
❓ This logic got moved out of the DB layer and up to the API level, right?
There was a problem hiding this comment.
Yep, because both the notifier and db has locks (or lock like things like transactions) which became easy to deadlock and hard to detect.
|
@kradalby I had this PR rebased on top of main running for a few hours, and did not have it lock up on me once! I had two other issues though that I assume are related to changes here.
|
kradalby
force-pushed
the
db-remove-lock-use-tx
branch
from
a757f44 to
05834aa
Compare
This commits removes the locks used to guard data integrity for the database and replaces them with Transactions, turns out that SQL had a way to deal with this all along. This reduces the complexity we had with multiple locks that might stack or recurse (database, nofitifer, mapper). All notifications and state updates are now triggered _after_ a database change. Signed-off-by: Kristoffer Dalby <[email protected]> wops Signed-off-by: Kristoffer Dalby <[email protected]>
kradalby
force-pushed
the
db-remove-lock-use-tx
branch
from
05834aa to
0d773b0
Compare
|
Thanks! Could you file them as two new issues and tag this PR? It is so big already that I do not really want to dump more stuff onto it, but of course that needs to be addressed. I think the ephemeral tests need to be improved and we need to find a way to replicate it as part of fixing it so we ensure there is no regression in the future. For the port 80, I think the curling in the ACL tests would cover that, so it is strange that they are passing 🤔 |
This commits removes the locks used to guard data integrity for the
database and replaces them with Transactions, turns out that SQL had
a way to deal with this all along.
This reduces the complexity we had with multiple locks that might stack
or recurse (database, nofitifer, mapper). All notifications and state
updates are now triggered after a database change.
Signed-off-by: Kristoffer Dalby [email protected]