Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: add locking mechanism for backend to prevent sync conflicts on map import #455

Closed
JannikStreek opened this issue Sep 12, 2024 · 1 comment
Assignees

Comments

@JannikStreek
Copy link
Member

JannikStreek commented Sep 12, 2024

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 chore: add locking mechanism for backend to prevent sync conflicts on map import #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 Bug: Maps with zero nodes #467
@JannikStreek
Copy link
Member Author

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.

@JannikStreek JannikStreek changed the title bug: maps with zero nodes chore: add locking mechanism for backend to prevent sync conflicts on map import Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants