chore: add locking mechanism for backend to prevent sync conflicts on map import
Closed this issue · 1 comments
JannikStreek commented
Description
On our servers there are some maps which do not contain any nodes. That can't really be. The only reason which comes to my mind is a failed json import (meaning: someone imported a json file on the server, but during the import an error occurred. As we clean the map first, an empty map would be the result). I think we delete all nodes before adding the new nodes. If the import fails. the map might end up with zero nodes. Best case would be a transaction which is rolled back or something similar.
// updates map nodes
async updateMap(clientMap: IMmpClientMap): Promise<MmpMap | null> {
// remove existing nodes, otherwise we will end up with multiple roots
await this.nodesRepository.delete({ nodeMapId: clientMap.uuid })
// Add new nodes from given map
await this.addNodesFromClient(clientMap.uuid, clientMap.data)
// reload map
return this.findMap(clientMap.uuid)
}
Originally posted by @JannikStreek in #296 (comment)
Proposed solution
There are multiple aspects within this issue and we can split them up in different merge requests:
- Using a transaction in the provided method, to prevent successfully removing all nodes and then failing when importing new ones.
- Integrating a new locking service, essentially working as a mutex service that protects against race conditions. We can at first apply this service when importing a whole map. See first comment #455 (comment)
- Leveraging the new locking service, to also lock on other problematic actions (removing a node or multiple nodes)
- Checking on the client side, that the import actually worked and otherwise falling back to the old map version. See #467
JannikStreek commented
I would propose the following:
- A nestjs locking service, that synchronizes access to maps and manages locks. I would propose to save
mapId
, 'createdAt' and the socket session id maybe. - Don't forget an index by mapId, as access will be high on those locks.
- The lock service starts with leveraging Postgres, later we could change it to using redis. Currently we don't have redis in our project, so I would defer this decision.
- Locks should only be set when a complete map is updated (because a new file was imported). Later we can use locks in other places. But keep it simple in the beginning.
- Changing data should only be allowed if no lock for a map is present or the old lock is too old (see below). Otherwise warn in the console, do not throw an exception.
- Locks older than a defined env variable
SYNC_LOCK_LIFETIME_THRESHOLD
can be ignored. - In the client app, a toast should be displayed with an error, if actions were refused due to a lock.