fix: updating vaults locking to use transaction locks where needed

MatrixAI · Aug 11, 2022 · 4c2520b · 4c2520b
1 parent 9914081
commit 4c2520b
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 91 deletions.
diff --git a/src/vaults/VaultInternal.ts b/src/vaults/VaultInternal.ts
@@ -459,6 +459,9 @@ class VaultInternal {
       throw new vaultsErrors.ErrorVaultRemoteDefined();
     }
     return withF([this.lock.write()], async () => {
+      await tran.lock(
+        [...this.vaultMetadataDbPath, VaultInternal.dirtyKey].toString(),
+      );
       await tran.put(
         [...this.vaultMetadataDbPath, VaultInternal.dirtyKey],
         true,
@@ -500,6 +503,9 @@ class VaultInternal {
         // Mirrored vaults are immutable
         throw new vaultsErrors.ErrorVaultRemoteDefined();
       }
+      await tran.lock(
+        [...vaultMetadataDbPath, VaultInternal.dirtyKey].toString(),
+      );
       await tran.put([...vaultMetadataDbPath, VaultInternal.dirtyKey], true);
 
       let result;

diff --git a/src/vaults/VaultManager.ts b/src/vaults/VaultManager.ts
@@ -116,7 +116,6 @@ class VaultManager {
   protected notificationsManager: NotificationsManager;
   protected vaultsDbPath: LevelPath = [this.constructor.name];
   protected vaultsNamesDbPath: LevelPath = [this.constructor.name, 'names'];
-  protected vaultsNamesLock: RWLockWriter = new RWLockWriter();
   // VaultId -> VaultMetadata
   protected vaultMap: VaultMap = new Map();
   protected vaultLocks: LockBox<RWLockWriter> = new LockBox();
@@ -274,42 +273,41 @@ class VaultManager {
     }
     // Adding vault to name map
     const vaultId = await this.generateVaultId();
-    return await this.vaultsNamesLock.withWriteF(async () => {
-      const vaultIdBuffer = await tran.get(
-        [...this.vaultsNamesDbPath, vaultName],
-        true,
-      );
-      // Check if the vault name already exists;
-      if (vaultIdBuffer != null) {
-        throw new vaultsErrors.ErrorVaultsVaultDefined();
-      }
-      await tran.put(
-        [...this.vaultsNamesDbPath, vaultName],
-        vaultId.toBuffer(),
-        true,
-      );
-      const vaultIdString = vaultId.toString() as VaultIdString;
-      return await this.vaultLocks.withF(
-        [vaultId, RWLockWriter, 'write'],
-        async () => {
-          // Creating vault
-          const vault = await VaultInternal.createVaultInternal({
-            vaultId,
-            vaultName,
-            keyManager: this.keyManager,
-            efs: this.efs,
-            logger: this.logger.getChild(VaultInternal.name),
-            db: this.db,
-            vaultsDbPath: this.vaultsDbPath,
-            fresh: true,
-            tran,
-          });
-          // Adding vault to object map
-          this.vaultMap.set(vaultIdString, vault);
-          return vault.vaultId;
-        },
-      );
-    });
+    await tran.lock([...this.vaultsNamesDbPath, vaultName].toString());
+    const vaultIdBuffer = await tran.get(
+      [...this.vaultsNamesDbPath, vaultName],
+      true,
+    );
+    // Check if the vault name already exists;
+    if (vaultIdBuffer != null) {
+      throw new vaultsErrors.ErrorVaultsVaultDefined();
+    }
+    await tran.put(
+      [...this.vaultsNamesDbPath, vaultName],
+      vaultId.toBuffer(),
+      true,
+    );
+    const vaultIdString = vaultId.toString() as VaultIdString;
+    return await this.vaultLocks.withF(
+      [vaultId, RWLockWriter, 'write'],
+      async () => {
+        // Creating vault
+        const vault = await VaultInternal.createVaultInternal({
+          vaultId,
+          vaultName,
+          keyManager: this.keyManager,
+          efs: this.efs,
+          logger: this.logger.getChild(VaultInternal.name),
+          db: this.db,
+          vaultsDbPath: this.vaultsDbPath,
+          fresh: true,
+          tran,
+        });
+        // Adding vault to object map
+        this.vaultMap.set(vaultIdString, vault);
+        return vault.vaultId;
+      },
+    );
   }
 
   /**
@@ -380,9 +378,8 @@ class VaultManager {
       // Removing from map
       this.vaultMap.delete(vaultIdString);
       // Removing name->id mapping
-      await this.vaultsNamesLock.withWriteF(async () => {
-        await tran.del([...this.vaultsNamesDbPath, vaultName]);
-      });
+      await tran.lock([...this.vaultsNamesDbPath, vaultName].toString());
+      await tran.del([...this.vaultsNamesDbPath, vaultName]);
     });
     this.logger.info(`Destroyed Vault ${vaultsUtils.encodeVaultId(vaultId)}`);
   }
@@ -448,6 +445,7 @@ class VaultManager {
     }
 
     await this.vaultLocks.withF([vaultId, RWLockWriter, 'write'], async () => {
+      await tran.lock([...this.vaultsNamesDbPath, newVaultName].toString());
       this.logger.info(`Renaming Vault ${vaultsUtils.encodeVaultId(vaultId)}`);
       // Checking if new name exists
       if (await this.getVaultId(newVaultName, tran)) {
@@ -459,21 +457,20 @@ class VaultManager {
         throw new vaultsErrors.ErrorVaultsVaultUndefined();
       }
       const oldVaultName = vaultMetadata.vaultName;
+      await tran.lock([...this.vaultsNamesDbPath, oldVaultName].toString());
       // Updating metadata with new name;
       const vaultDbPath = [
         ...this.vaultsDbPath,
         vaultsUtils.encodeVaultId(vaultId),
       ];
       await tran.put([...vaultDbPath, VaultInternal.nameKey], newVaultName);
       // Updating name->id map
-      await this.vaultsNamesLock.withWriteF(async () => {
-        await tran.del([...this.vaultsNamesDbPath, oldVaultName]);
-        await tran.put(
-          [...this.vaultsNamesDbPath, newVaultName],
-          vaultId.toBuffer(),
-          true,
-        );
-      });
+      await tran.del([...this.vaultsNamesDbPath, oldVaultName]);
+      await tran.put(
+        [...this.vaultsNamesDbPath, newVaultName],
+        vaultId.toBuffer(),
+        true,
+      );
     });
   }
 
@@ -491,14 +488,13 @@ class VaultManager {
       );
     }
 
-    return await this.vaultsNamesLock.withWriteF(async () => {
-      const vaultIdBuffer = await tran.get(
-        [...this.vaultsNamesDbPath, vaultName],
-        true,
-      );
-      if (vaultIdBuffer == null) return;
-      return IdInternal.fromBuffer<VaultId>(vaultIdBuffer);
-    });
+    await tran.lock([...this.vaultsNamesDbPath, vaultName].toString());
+    const vaultIdBuffer = await tran.get(
+      [...this.vaultsNamesDbPath, vaultName],
+      true,
+    );
+    if (vaultIdBuffer == null) return;
+    return IdInternal.fromBuffer<VaultId>(vaultIdBuffer);
   }
 
   /**

diff --git a/tests/vaults/VaultManager.test.ts b/tests/vaults/VaultManager.test.ts
@@ -317,7 +317,7 @@ describe('VaultManager', () => {
     },
     globalThis.defaultTimeout * 2,
   );
-  test('cannot concurrently create vaults with the same name', async () => {
+  test('Concurrently creating vault with same name only creates 1 vault', async () => {
     const vaultManager = await VaultManager.createVaultManager({
       vaultsPath,
       keyManager: dummyKeyManager,
@@ -329,13 +329,15 @@ describe('VaultManager', () => {
       logger: logger.getChild(VaultManager.name),
     });
     try {
-      const vaults = Promise.all([
-        vaultManager.createVault(vaultName),
-        vaultManager.createVault(vaultName),
-      ]);
-      await expect(() => vaults).rejects.toThrow(
-        vaultsErrors.ErrorVaultsVaultDefined,
-      );
+      await expect(
+        Promise.all([
+          vaultManager.createVault(vaultName),
+          vaultManager.createVault(vaultName),
+        ]),
+      ).rejects.toThrow(vaultsErrors.ErrorVaultsVaultDefined);
+      // @ts-ignore: kidnapping the map
+      const vaultMap = vaultManager.vaultMap;
+      expect(vaultMap.size).toBe(1);
     } finally {
       await vaultManager?.stop();
       await vaultManager?.destroy();
@@ -1760,33 +1762,6 @@ describe('VaultManager', () => {
       await vaultManager?.destroy();
     }
   });
-  test('Concurrently creating vault with same name only creates 1 vault', async () => {
-    const vaultManager = await VaultManager.createVaultManager({
-      vaultsPath,
-      keyManager: dummyKeyManager,
-      gestaltGraph: {} as GestaltGraph,
-      nodeConnectionManager: {} as NodeConnectionManager,
-      acl: {} as ACL,
-      notificationsManager: {} as NotificationsManager,
-      db,
-      logger: logger.getChild(VaultManager.name),
-    });
-
-    try {
-      await expect(
-        Promise.all([
-          vaultManager.createVault(vaultName),
-          vaultManager.createVault(vaultName),
-        ]),
-      ).rejects.toThrow(vaultsErrors.ErrorVaultsVaultDefined);
-      // @ts-ignore: kidnapping the map
-      const vaultMap = vaultManager.vaultMap;
-      expect(vaultMap.size).toBe(1);
-    } finally {
-      await vaultManager?.stop();
-      await vaultManager?.destroy();
-    }
-  });
   test('vaults persist', async () => {
     const vaultManager = await VaultManager.createVaultManager({
       vaultsPath,