fix: Make sure locker allows reentrant lock acquisition

RELEASE_NOTES.md

@@ -66,6 +66,9 @@ The following changes are relevant for v5 custom configs that replaced certain f
     - `/notifications/*`
 - IDP private key generation was moved to a separate generator class.
     - `/identity/handler/*`
+- The Read/Write lockers have changed slightly.
+    - `/util/resource-locker/file.json`
+    - `/util/resource-locker/memory.json`
 
 ### Interface changes
 
@@ -83,6 +86,7 @@ These changes are relevant if you wrote custom modules for the server that depen
   `PermissionSet` and `Permission` were merged into a single interface.
   This impacts all authentication and authorization related classes.
 - `HttpServerFactory.startServer` function was renamed to `createServer` and is no longer expected to start the server.
+- `GreedyReadWriteLocker` constructor parameters have changed.
 
 ## v5.1.0
 

config/util/resource-locker/file.json

@@ -6,7 +6,7 @@
       "@id": "urn:solid-server:default:ResourceLocker",
       "@type": "WrappedExpiringReadWriteLocker",
       "locker": {
-        "@type": "EqualReadWriteLocker",
+        "@type": "PartialReadWriteLocker",
         "locker": {
           "@id": "urn:solid-server:default:FileSystemResourceLocker",
           "@type": "FileSystemResourceLocker",

config/util/resource-locker/memory.json

@@ -10,10 +10,7 @@
         "locker": {
           "@type": "MemoryResourceLocker"
         },
-        "storage": { "@id": "urn:solid-server:default:LockStorage" },
+        "storage": { "@id": "urn:solid-server:default:LockStorage" }
-        "suffixes_count": "count",
-        "suffixes_read": "read",
-        "suffixes_write": "write"
       },
       "expiration": 6000
     }

src/index.ts

@@ -501,11 +501,13 @@ export * from './util/identifiers/SingleRootIdentifierStrategy';
 export * from './util/identifiers/SubdomainIdentifierStrategy';
 
 // Util/Locking
+export * from './util/locking/BaseReadWriteLocker';
 export * from './util/locking/ExpiringReadWriteLocker';
 export * from './util/locking/EqualReadWriteLocker';
 export * from './util/locking/FileSystemResourceLocker';
 export * from './util/locking/GreedyReadWriteLocker';
 export * from './util/locking/MemoryResourceLocker';
+export * from './util/locking/PartialReadWriteLocker';
 export * from './util/locking/ReadWriteLocker';
 export * from './util/locking/RedisLocker';
 export * from './util/locking/ResourceLocker';

src/server/AuthorizingHttpHandler.ts

@@ -64,10 +64,14 @@ export class AuthorizingHttpHandler extends OperationHttpHandler {
     this.logger.verbose(`Extracted credentials: ${JSON.stringify(credentials)}`);
 
     const requestedModes = await this.modesExtractor.handleSafe(operation);
-    this.logger.verbose(`Retrieved required modes: ${[ ...requestedModes.entrySets() ]}`);
+    this.logger.verbose(`Retrieved required modes: ${
+      [ ...requestedModes.entrySets() ].map(([ id, set ]): string => `{ ${id.path}: ${[ ...set ]} }`)
+    }`);
 
     const availablePermissions = await this.permissionReader.handleSafe({ credentials, requestedModes });
-    this.logger.verbose(`Available permissions are ${[ ...availablePermissions.entries() ]}`);
+    this.logger.verbose(`Available permissions are ${
+      [ ...availablePermissions.entries() ].map(([ id, map ]): string => `{ ${id.path}: ${JSON.stringify(map)} }`)
+    }`);
 
     try {
       await this.authorizer.handleSafe({ credentials, requestedModes, availablePermissions });

src/server/WacAllowHttpHandler.ts

@@ -71,6 +71,8 @@ export class WacAllowHttpHandler extends OperationHttpHandler {
       } else {
         // Need to determine public permissions
         this.logger.debug('Determining public permissions');
+        // Note that this call can potentially create a new lock on a resource that is already locked,
+        // so a locker that allows multiple read locks on the same resource is required.
         const permissionMap = await this.permissionReader.handleSafe({ credentials: {}, requestedModes });
         everyone = permissionMap.get(operation.target) ?? {};
       }

src/util/locking/BaseReadWriteLocker.ts

@@ -0,0 +1,95 @@
+import type { ResourceIdentifier } from '../../http/representation/ResourceIdentifier';
+import type { PromiseOrValue } from '../PromiseUtil';
+import { EqualReadWriteLocker } from './EqualReadWriteLocker';
+import type { ResourceLocker } from './ResourceLocker';
+
+/**
+ * A {@link ReadWriteLocker} that allows for multiple simultaneous read operations.
+ * Write operations will be blocked as long as read operations are not finished.
+ * New read operations are allowed while this is going on, which will cause write operations to wait longer.
+ *
+ * Based on https://en.wikipedia.org/wiki/Readers%E2%80%93writer_lock#Using_two_mutexes .
+ * As soon as 1 read lock request is made, the main lock is locked.
+ * Internally a counter keeps track of the amount of active read locks.
+ * Only when this number reaches 0 will the main lock be released again.
+ * The internal count lock is only locked to increase/decrease this counter and is released afterwards.
+ * This allows for multiple read operations, although only 1 at the time can update the counter,
+ * which means there can still be a small waiting period if there are multiple simultaneous read operations.
+ *
+ * Classes extending this need to implement `getCountLockIdentifier` and `modifyCount`.
+ */
+export abstract class BaseReadWriteLocker extends EqualReadWriteLocker {
+  protected readonly countLocker: ResourceLocker;
+
+  /**
+   * @param resourceLocker - Used for creating read and write locks.
+   * @param countLocker - Used for creating locks when updating the counter.
+   */
+  protected constructor(resourceLocker: ResourceLocker, countLocker: ResourceLocker) {
+    super(resourceLocker);
+    this.countLocker = countLocker;
+  }
+
+  public async withReadLock<T>(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue<T>): Promise<T> {
+    await this.acquireReadLock(identifier);
+    try {
+      return await whileLocked();
+    } finally {
+      await this.releaseReadLock(identifier);
+    }
+  }
+
+  /**
+   * Safely updates the count before starting a read operation.
+   */
+  private async acquireReadLock(identifier: ResourceIdentifier): Promise<void> {
+    await this.withInternalCountLock(identifier, async(): Promise<void> => {
+      const count = await this.modifyCount(identifier, +1);
+      if (count === 1) {
+        await this.locker.acquire(identifier);
+      }
+    });
+  }
+
+  /**
+   * Safely decreases the count after the read operation is finished.
+   */
+  private async releaseReadLock(identifier: ResourceIdentifier): Promise<void> {
+    await this.withInternalCountLock(identifier, async(): Promise<void> => {
+      const count = await this.modifyCount(identifier, -1);
+      if (count === 0) {
+        await this.locker.release(identifier);
+      }
+    });
+  }
+
+  /**
+   * Safely runs an action on the count.
+   */
+  private async withInternalCountLock<T>(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue<T>):
+  Promise<T> {
+    const read = this.getCountLockIdentifier(identifier);
+    await this.countLocker.acquire(read);
+    try {
+      return await whileLocked();
+    } finally {
+      await this.countLocker.release(read);
+    }
+  }
+
+  /**
+   * Generate the identifier that will be used to acquire the count lock for the given identifier.
+   * There will be cases where this lock needs to be acquired
+   * while an active lock on the main resource is still being maintained.
+   * This means that if the input `resourceLocker` and `countLocker` use the same locking system
+   * this generated identifier needs to be different.
+   */
+  protected abstract getCountLockIdentifier(identifier: ResourceIdentifier): ResourceIdentifier;
+
+  /**
+   * Update the counter that keeps track of having open read locks there currently are.
+   * @param identifier - Identifier on which to update the number of read locks.
+   * @param mod - `+1` or `-1`.
+   */
+  protected abstract modifyCount(identifier: ResourceIdentifier, mod: number): PromiseOrValue<number>;
+}

src/util/locking/EqualReadWriteLocker.ts

@@ -7,7 +7,7 @@ import type { ResourceLocker } from './ResourceLocker';
  * A {@link ReadWriteLocker} that gives no priority to read or write operations: both use the same lock.
  */
 export class EqualReadWriteLocker implements ReadWriteLocker {
-  private readonly locker: ResourceLocker;
+  protected readonly locker: ResourceLocker;
 
   public constructor(locker: ResourceLocker) {
     this.locker = locker;
@@ -27,7 +27,7 @@ export class EqualReadWriteLocker implements ReadWriteLocker {
    * @param identifier - Identifier of resource that needs to be locked.
    * @param whileLocked - Function to resolve while the resource is locked.
    */
-  private async withLock<T>(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue<T>): Promise<T> {
+  protected async withLock<T>(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue<T>): Promise<T> {
     await this.locker.acquire(identifier);
     try {
       return await whileLocked();

src/util/locking/GreedyReadWriteLocker.ts

@@ -1,132 +1,44 @@
 import type { ResourceIdentifier } from '../../http/representation/ResourceIdentifier';
 import type { KeyValueStorage } from '../../storage/keyvalue/KeyValueStorage';
-import { ForbiddenHttpError } from '../errors/ForbiddenHttpError';
 import { InternalServerError } from '../errors/InternalServerError';
-import type { PromiseOrValue } from '../PromiseUtil';
+import { BaseReadWriteLocker } from './BaseReadWriteLocker';
-import type { ReadWriteLocker } from './ReadWriteLocker';
 import type { ResourceLocker } from './ResourceLocker';
 
-export interface GreedyReadWriteSuffixes {
-  count: string;
-  read: string;
-  write: string;
-}
-
 /**
- * A {@link ReadWriteLocker} that allows for multiple simultaneous read operations.
+ * A {@link BaseReadWriteLocker} that uses the same locker for the main lock and the count lock,
- * Write operations will be blocked as long as read operations are not finished.
+ * and uses a {@link KeyValueStorage} for keeping track of the counter.
- * New read operations are allowed while this is going on, which will cause write operations to wait longer.
  *
- * Based on https://en.wikipedia.org/wiki/Readers%E2%80%93writer_lock#Using_two_mutexes .
+ * Since it is completely dependent on other implementations,
- * As soon as 1 read lock request is made, the write lock is locked.
+ * this locker is threadsafe if its inputs are as well.
- * Internally a counter keeps track of the amount of active read locks.
- * Only when this number reaches 0 will the write lock be released again.
- * The internal read lock is only locked to increase/decrease this counter and is released afterwards.
- * This allows for multiple read operations, although only 1 at the time can update the counter,
- * which means there can still be a small waiting period if there are multiple simultaneous read operations.
  */
-export class GreedyReadWriteLocker implements ReadWriteLocker {
+export class GreedyReadWriteLocker extends BaseReadWriteLocker {
-  private readonly locker: ResourceLocker;
+  protected readonly storage: KeyValueStorage<string, number>;
-  private readonly storage: KeyValueStorage<string, number>;
+  protected readonly readSuffix: string;
-  private readonly suffixes: GreedyReadWriteSuffixes;
+  protected readonly countSuffix: string;
 
   /**
    * @param locker - Used for creating read and write locks.
    * @param storage - Used for storing the amount of active read operations on a resource.
-   * @param suffixes - Used to generate identifiers with the given suffixes.
+   * @param readSuffix - Used to generate the identifier for the lock that is applied when updating the counter.
-   *                   `count` is used for the identifier used to store the counter.
+   * @param countSuffix - Used to generate the identifier that will be used in the storage for storing the counter.
-   *                   `read` and `write` are used for the 2 types of locks that are needed.
    */
   public constructor(locker: ResourceLocker, storage: KeyValueStorage<string, number>,
-    suffixes: GreedyReadWriteSuffixes = { count: 'count', read: 'read', write: 'write' }) {
+    readSuffix = 'read', countSuffix = 'count') {
-    this.locker = locker;
+    super(locker, locker);
     this.storage = storage;
-    this.suffixes = suffixes;
+    this.readSuffix = readSuffix;
+    this.countSuffix = countSuffix;
   }
 
-  public async withReadLock<T>(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue<T>): Promise<T> {
+  protected getCountLockIdentifier(identifier: ResourceIdentifier): ResourceIdentifier {
-    await this.preReadSetup(identifier);
+    return { path: `${identifier.path}.${this.readSuffix}` };
-    try {
-      return await whileLocked();
-    } finally {
-      await this.postReadCleanup(identifier);
-    }
-  }
-
-  public async withWriteLock<T>(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue<T>): Promise<T> {
-    if (identifier.path.endsWith(`.${this.suffixes.count}`)) {
-      throw new ForbiddenHttpError('This resource is used for internal purposes.');
-    }
-    const write = this.getWriteLockKey(identifier);
-    await this.locker.acquire(write);
-    try {
-      return await whileLocked();
-    } finally {
-      await this.locker.release(write);
-    }
   }
 
   /**
    * This key is used for storing the count of active read operations.
    */
-  private getCountKey(identifier: ResourceIdentifier): string {
+  protected getCountKey(identifier: ResourceIdentifier): string {
-    return `${identifier.path}.${this.suffixes.count}`;
+    return `${identifier.path}.${this.countSuffix}`;
-  }
-
-  /**
-   * This is the identifier for the read lock: the lock that is used to safely update and read the count.
-   */
-  private getReadLockKey(identifier: ResourceIdentifier): ResourceIdentifier {
-    return { path: `${identifier.path}.${this.suffixes.read}` };
-  }
-
-  /**
-   * This is the identifier for the write lock, making sure there is at most 1 write operation active.
-   */
-  private getWriteLockKey(identifier: ResourceIdentifier): ResourceIdentifier {
-    return { path: `${identifier.path}.${this.suffixes.write}` };
-  }
-
-  /**
-   * Safely updates the count before starting a read operation.
-   */
-  private async preReadSetup(identifier: ResourceIdentifier): Promise<void> {
-    await this.withInternalReadLock(identifier, async(): Promise<void> => {
-      const count = await this.incrementCount(identifier, +1);
-      if (count === 1) {
-        // There is at least 1 read operation so write operations are blocked
-        const write = this.getWriteLockKey(identifier);
-        await this.locker.acquire(write);
-      }
-    });
-  }
-
-  /**
-   * Safely decreases the count after the read operation is finished.
-   */
-  private async postReadCleanup(identifier: ResourceIdentifier): Promise<void> {
-    await this.withInternalReadLock(identifier, async(): Promise<void> => {
-      const count = await this.incrementCount(identifier, -1);
-      if (count === 0) {
-        // All read locks have been released so a write operation is possible again
-        const write = this.getWriteLockKey(identifier);
-        await this.locker.release(write);
-      }
-    });
-  }
-
-  /**
-   * Safely runs an action on the count.
-   */
-  private async withInternalReadLock<T>(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue<T>):
-  Promise<T> {
-    const read = this.getReadLockKey(identifier);
-    await this.locker.acquire(read);
-    try {
-      return await whileLocked();
-    } finally {
-      await this.locker.release(read);
-    }
   }
 
   /**
@@ -134,7 +46,7 @@ export class GreedyReadWriteLocker implements ReadWriteLocker {
    * Creates the data if it didn't exist yet.
    * Deletes the data when the count reaches zero.
    */
-  private async incrementCount(identifier: ResourceIdentifier, mod: number): Promise<number> {
+  protected async modifyCount(identifier: ResourceIdentifier, mod: number): Promise<number> {
     const countKey = this.getCountKey(identifier);
     let number = await this.storage.get(countKey) ?? 0;
     number += mod;

src/util/locking/PartialReadWriteLocker.ts

@@ -0,0 +1,43 @@
+import type { ResourceIdentifier } from '../../http/representation/ResourceIdentifier';
+import { BaseReadWriteLocker } from './BaseReadWriteLocker';
+import { MemoryResourceLocker } from './MemoryResourceLocker';
+import type { ResourceLocker } from './ResourceLocker';
+
+/**
+ * A {@link BaseReadWriteLocker} that stores the counter and its associated locks in memory.
+ * The consequence of this is that multiple read requests are possible as long as they occur on the same worker thread.
+ * A read request from a different worker thread will have to wait
+ * until those from the current worker thread are finished.
+ *
+ * The main reason for this class is due to the file locker that we use only allowing locks to be released
+ * by the same worker thread that acquired them.
+ */
+export class PartialReadWriteLocker extends BaseReadWriteLocker {
+  private readonly readCount: Map<string, number>;
+
+  public constructor(locker: ResourceLocker) {
+    // This goes against how we generally link classes together using Components.js.
+    // The reason for doing this is that `MemoryResourceLocker` implements `SingleThreaded`,
+    // meaning that when the server is started with worker threads an error will be thrown by Components.js.
+    // Instantiating it here "hides" it from Components.js.
+    // If at some point in the future this causes issues because we want to split up the code,
+    // this should not be blocking and an alternative solution should be used,
+    // such as removing the SingleThreaded interface from the locker.
+    super(locker, new MemoryResourceLocker());
+    this.readCount = new Map();
+  }
+
+  protected getCountLockIdentifier(identifier: ResourceIdentifier): ResourceIdentifier {
+    return identifier;
+  }
+
+  protected modifyCount(identifier: ResourceIdentifier, mod: number): number {
+    const modified = (this.readCount.get(identifier.path) ?? 0) + mod;
+    if (modified === 0) {
+      this.readCount.delete(identifier.path);
+    } else {
+      this.readCount.set(identifier.path, modified);
+    }
+    return modified;
+  }
+}

test/integration/Identity.test.ts

@@ -13,12 +13,24 @@ import type { App } from '../../src/init/App';
 import { APPLICATION_JSON, APPLICATION_X_WWW_FORM_URLENCODED } from '../../src/util/ContentTypes';
 import { joinUrl } from '../../src/util/PathUtil';
 import { getPort } from '../util/Util';
-import { getDefaultVariables, getTestConfigPath, instantiateFromConfig } from './Config';
+import { getDefaultVariables, getTestConfigPath, getTestFolder, instantiateFromConfig, removeFolder } from './Config';
 import { IdentityTestState } from './IdentityTestState';
 
 const port = getPort('Identity');
 const baseUrl = `http://localhost:${port}/`;
 
+const rootFilePath = getTestFolder('Identity');
+const stores: [string, any][] = [
+  [ 'in-memory storage', {
+    config: 'server-memory.json',
+    teardown: jest.fn(),
+  }],
+  [ 'on-disk storage', {
+    config: 'server-file.json',
+    teardown: async(): Promise<void> => removeFolder(rootFilePath),
+  }],
+];
+
 // Don't send actual e-mails
 jest.mock('nodemailer');
 
@@ -37,7 +49,7 @@ async function postForm(url: string, formBody: string): Promise<Response> {
 // They will be simulated by storing the values and passing them along.
 // This is why the redirects are handled manually.
 // We also need to parse the HTML in several steps since there is no API.
-describe('A Solid server with IDP', (): void => {
+describe.each(stores)('A Solid server with IDP using %s', (name, { config, teardown }): void => {
   let app: App;
   const redirectUrl = 'http://mockedredirect/';
   const container = new URL('secret/', baseUrl).href;
@@ -61,8 +73,11 @@ describe('A Solid server with IDP', (): void => {
 
     const instances = await instantiateFromConfig(
       'urn:solid-server:test:Instances',
-      getTestConfigPath('server-memory.json'),
+      getTestConfigPath(config),
-      getDefaultVariables(port, baseUrl),
+      {
+        ...getDefaultVariables(port, baseUrl),
+        'urn:solid-server:default:variable:rootFilePath': rootFilePath,
+      },
     ) as Record<string, any>;
     ({ app } = instances);
     await app.start();
@@ -90,11 +105,12 @@ describe('A Solid server with IDP', (): void => {
       body: aclTurtle,
     });
     if (res.status !== 201) {
-      throw new Error('Something went wrong initializing the test ACL');
+      throw new Error(`Something went wrong initializing the test ACL: ${await res.text()}`);
     }
   });
 
   afterAll(async(): Promise<void> => {
+    await teardown();
     await app.stop();
   });
 

test/unit/util/locking/BaseReadWriteLocker.test.ts

@@ -0,0 +1,315 @@
+import { EventEmitter } from 'events';
+import type { ResourceIdentifier } from '../../../../src/http/representation/ResourceIdentifier';
+import { BaseReadWriteLocker } from '../../../../src/util/locking/BaseReadWriteLocker';
+import type { ResourceLocker } from '../../../../src/util/locking/ResourceLocker';
+import { flushPromises } from '../../../util/Util';
+
+// A simple ResourceLocker that keeps a queue of lock requests
+class MemoryLocker implements ResourceLocker {
+  private readonly locks: Record<string, (() => void)[]>;
+
+  public constructor() {
+    this.locks = {};
+  }
+
+  public async acquire(identifier: ResourceIdentifier): Promise<void> {
+    const { path } = identifier;
+    if (!this.locks[path]) {
+      this.locks[path] = [];
+    } else {
+      return new Promise((resolve): void => {
+        this.locks[path].push(resolve);
+      });
+    }
+  }
+
+  public async release(identifier: ResourceIdentifier): Promise<void> {
+    const { path } = identifier;
+    if (this.locks[path].length > 0) {
+      this.locks[path].shift()!();
+    } else {
+      // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
+      delete this.locks[path];
+    }
+  }
+}
+
+class SimpleReadWriteLocker extends BaseReadWriteLocker {
+  private readonly countMap: Map<string, number>;
+
+  public constructor(resourceLocker: ResourceLocker, countLocker: ResourceLocker) {
+    super(resourceLocker, countLocker);
+    this.countMap = new Map<string, number>();
+  }
+
+  protected getCountLockIdentifier(identifier: ResourceIdentifier): ResourceIdentifier {
+    return identifier;
+  }
+
+  protected modifyCount(identifier: ResourceIdentifier, mod: number): number {
+    const count = (this.countMap.get(identifier.path) ?? 0) + mod;
+    this.countMap.set(identifier.path, count);
+    return count;
+  }
+}
+
+describe('A BaseReadWriteLocker', (): void => {
+  let resourceLocker: ResourceLocker;
+  let countLocker: ResourceLocker;
+  const resourceId = { path: 'http://test.com/resource' };
+  const resource2Id = { path: 'http://test.com/resource2' };
+  let locker: BaseReadWriteLocker;
+
+  beforeEach(async(): Promise<void> => {
+    resourceLocker = new MemoryLocker();
+    countLocker = new MemoryLocker();
+
+    locker = new SimpleReadWriteLocker(resourceLocker, countLocker);
+  });
+
+  it('does not block single read operations.', async(): Promise<void> => {
+    await expect(locker.withReadLock(resourceId, (): number => 5)).resolves.toBe(5);
+  });
+
+  it('does not block single write operations.', async(): Promise<void> => {
+    await expect(locker.withWriteLock(resourceId, (): number => 5)).resolves.toBe(5);
+  });
+
+  it('does not block multiple read operations.', async(): Promise<void> => {
+    const order: string[] = [];
+    const emitter = new EventEmitter();
+
+    const unlocks = [ 0, 1, 2 ].map((num): any => new Promise((resolve): any => emitter.on(`release${num}`, resolve)));
+    const promises = [ 0, 1, 2 ].map((num): any => locker.withReadLock(resourceId, async(): Promise<number> => {
+      order.push(`start ${num}`);
+      await unlocks[num];
+      order.push(`finish ${num}`);
+      return num;
+    }));
+
+    // Allow time to attach listeners
+    await flushPromises();
+
+    emitter.emit('release2');
+    await expect(promises[2]).resolves.toBe(2);
+    emitter.emit('release0');
+    await expect(promises[0]).resolves.toBe(0);
+    emitter.emit('release1');
+    await expect(promises[1]).resolves.toBe(1);
+
+    expect(order).toEqual([ 'start 0', 'start 1', 'start 2', 'finish 2', 'finish 0', 'finish 1' ]);
+  });
+
+  it('blocks multiple write operations.', async(): Promise<void> => {
+    // Previous test but with write locks
+    const order: string[] = [];
+    const emitter = new EventEmitter();
+
+    const unlocks = [ 0, 1, 2 ].map((num): any => new Promise((resolve): any => emitter.on(`release${num}`, resolve)));
+    const promises = [ 0, 1, 2 ].map((num): any => locker.withWriteLock(resourceId, async(): Promise<number> => {
+      order.push(`start ${num}`);
+      await unlocks[num];
+      order.push(`finish ${num}`);
+      return num;
+    }));
+
+    // Allow time to attach listeners
+    await flushPromises();
+
+    emitter.emit('release2');
+
+    // Allow time to finish write 2
+    await flushPromises();
+
+    emitter.emit('release0');
+    emitter.emit('release1');
+    await Promise.all([ promises[2], promises[0], promises[1] ]);
+    expect(order).toEqual([ 'start 0', 'finish 0', 'start 1', 'finish 1', 'start 2', 'finish 2' ]);
+  });
+
+  it('allows multiple write operations on different resources.', async(): Promise<void> => {
+    // Previous test but with write locks
+    const order: string[] = [];
+    const emitter = new EventEmitter();
+
+    const resources = [ resourceId, resource2Id ];
+    const unlocks = [ 0, 1 ].map((num): any => new Promise((resolve): any => emitter.on(`release${num}`, resolve)));
+    const promises = [ 0, 1 ].map((num): any => locker.withWriteLock(resources[num], async(): Promise<number> => {
+      order.push(`start ${num}`);
+      await unlocks[num];
+      order.push(`finish ${num}`);
+      return num;
+    }));
+
+    // Allow time to attach listeners
+    await flushPromises();
+
+    emitter.emit('release1');
+    await expect(promises[1]).resolves.toBe(1);
+    emitter.emit('release0');
+    await expect(promises[0]).resolves.toBe(0);
+
+    expect(order).toEqual([ 'start 0', 'start 1', 'finish 1', 'finish 0' ]);
+  });
+
+  it('blocks write operations during read operations.', async(): Promise<void> => {
+    const order: string[] = [];
+    const emitter = new EventEmitter();
+
+    const promRead = new Promise((resolve): any => {
+      emitter.on('releaseRead', resolve);
+    });
+
+    // We want to make sure the write operation only starts while the read operation is busy
+    // Otherwise the internal write lock might not be acquired yet
+    const delayedLockWrite = new Promise<void>((resolve): void => {
+      emitter.on('readStarted', (): void => {
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
+        locker.withWriteLock(resourceId, (): any => {
+          order.push('write');
+          resolve();
+        });
+      });
+    });
+
+    const lockRead = locker.withReadLock(resourceId, async(): Promise<void> => {
+      emitter.emit('readStarted');
+      order.push('read start');
+      await promRead;
+      order.push('read finish');
+    });
+
+    // Allow time to attach listeners
+    await flushPromises();
+
+    const promAll = Promise.all([ delayedLockWrite, lockRead ]);
+
+    emitter.emit('releaseRead');
+    await promAll;
+    expect(order).toEqual([ 'read start', 'read finish', 'write' ]);
+  });
+
+  it('allows write operations on different resources during read operations.', async(): Promise<void> => {
+    const order: string[] = [];
+    const emitter = new EventEmitter();
+
+    const promRead = new Promise((resolve): any => {
+      emitter.on('releaseRead', resolve);
+    });
+
+    const delayedLockWrite = new Promise<void>((resolve): void => {
+      emitter.on('readStarted', (): void => {
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
+        locker.withWriteLock(resource2Id, (): any => {
+          order.push('write');
+          resolve();
+        });
+      });
+    });
+
+    const lockRead = locker.withReadLock(resourceId, async(): Promise<void> => {
+      emitter.emit('readStarted');
+      order.push('read start');
+      await promRead;
+      order.push('read finish');
+    });
+
+    // Allow time to attach listeners
+    await flushPromises();
+
+    const promAll = Promise.all([ delayedLockWrite, lockRead ]);
+
+    emitter.emit('releaseRead');
+    await promAll;
+    expect(order).toEqual([ 'read start', 'write', 'read finish' ]);
+  });
+
+  it('prioritizes read operations when a read operation is waiting.', async(): Promise<void> => {
+    // This test is very similar to the previous ones but adds an extra read lock
+    const order: string[] = [];
+    const emitter = new EventEmitter();
+
+    const promRead1 = new Promise((resolve): any => emitter.on('releaseRead1', resolve));
+    const promRead2 = new Promise((resolve): any => emitter.on('releaseRead2', resolve));
+
+    const delayedLockWrite = new Promise<void>((resolve): void => {
+      emitter.on('readStarted', (): void => {
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
+        locker.withWriteLock(resourceId, (): any => {
+          order.push('write');
+          resolve();
+        });
+      });
+    });
+
+    const delayedLockRead2 = new Promise<void>((resolve): void => {
+      emitter.on('readStarted', (): void => {
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
+        locker.withReadLock(resourceId, async(): Promise<void> => {
+          order.push('read 2 start');
+          await promRead2;
+          order.push('read 2 finish');
+          resolve();
+        });
+      });
+    });
+
+    const lockRead = locker.withReadLock(resourceId, async(): Promise<void> => {
+      emitter.emit('readStarted');
+      order.push('read 1 start');
+      await promRead1;
+      order.push('read 1 finish');
+    });
+
+    // Allow time to attach listeners
+    await flushPromises();
+
+    const promAll = Promise.all([ delayedLockWrite, lockRead, delayedLockRead2 ]);
+
+    emitter.emit('releaseRead1');
+
+    // Allow time to finish read 1
+    await flushPromises();
+
+    emitter.emit('releaseRead2');
+    await promAll;
+    expect(order).toEqual([ 'read 1 start', 'read 2 start', 'read 1 finish', 'read 2 finish', 'write' ]);
+  });
+
+  it('blocks read operations during write operations.', async(): Promise<void> => {
+    // Again similar but with read and write order switched
+    const order: string[] = [];
+    const emitter = new EventEmitter();
+
+    const promWrite = new Promise((resolve): any => {
+      emitter.on('releaseWrite', resolve);
+    });
+
+    // We want to make sure the read operation only starts while the write operation is busy
+    const delayedLockRead = new Promise<void>((resolve): void => {
+      emitter.on('writeStarted', (): void => {
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
+        locker.withReadLock(resourceId, (): any => {
+          order.push('read');
+          resolve();
+        });
+      });
+    });
+
+    const lockWrite = locker.withWriteLock(resourceId, async(): Promise<void> => {
+      emitter.emit('writeStarted');
+      order.push('write start');
+      await promWrite;
+      order.push('write finish');
+    });
+
+    // Allow time to attach listeners
+    await flushPromises();
+
+    const promAll = Promise.all([ delayedLockRead, lockWrite ]);
+
+    emitter.emit('releaseWrite');
+    await promAll;
+    expect(order).toEqual([ 'write start', 'write finish', 'read' ]);
+  });
+});

test/unit/util/locking/GreedyReadWriteLocker.test.ts

@@ -1,314 +1,42 @@
-import { EventEmitter } from 'events';
-import type { ResourceIdentifier } from '../../../../src/http/representation/ResourceIdentifier';
 import type { KeyValueStorage } from '../../../../src/storage/keyvalue/KeyValueStorage';
-import { ForbiddenHttpError } from '../../../../src/util/errors/ForbiddenHttpError';
 import { InternalServerError } from '../../../../src/util/errors/InternalServerError';
 import { GreedyReadWriteLocker } from '../../../../src/util/locking/GreedyReadWriteLocker';
 import type { ResourceLocker } from '../../../../src/util/locking/ResourceLocker';
-import { flushPromises } from '../../../util/Util';
-
-// A simple ResourceLocker that keeps a queue of lock requests
-class MemoryLocker implements ResourceLocker {
-  private readonly locks: Record<string, (() => void)[]>;
-
-  public constructor() {
-    this.locks = {};
-  }
-
-  public async acquire(identifier: ResourceIdentifier): Promise<void> {
-    const { path } = identifier;
-    if (!this.locks[path]) {
-      this.locks[path] = [];
-    } else {
-      return new Promise((resolve): void => {
-        this.locks[path].push(resolve);
-      });
-    }
-  }
-
-  public async release(identifier: ResourceIdentifier): Promise<void> {
-    const { path } = identifier;
-    if (this.locks[path].length > 0) {
-      this.locks[path].shift()!();
-    } else {
-      // eslint-disable-next-line @typescript-eslint/no-dynamic-delete
-      delete this.locks[path];
-    }
-  }
-}
 
 describe('A GreedyReadWriteLocker', (): void => {
-  let sourceLocker: ResourceLocker;
+  let resourceLocker: jest.Mocked<ResourceLocker>;
+  let storageMap: Map<string, number>;
   let storage: KeyValueStorage<string, number>;
   const resourceId = { path: 'http://test.com/resource' };
-  const resource2Id = { path: 'http://test.com/resource2' };
   let locker: GreedyReadWriteLocker;
 
   beforeEach(async(): Promise<void> => {
-    sourceLocker = new MemoryLocker();
+    resourceLocker = {
+      acquire: jest.fn(),
+      release: jest.fn(),
+    };
 
-    storage = new Map<string, number>() as any;
+    storageMap = new Map<string, number>();
+    storage = storageMap as any;
 
-    locker = new GreedyReadWriteLocker(sourceLocker, storage);
+    locker = new GreedyReadWriteLocker(resourceLocker, storage);
   });
 
-  it('does not block single read operations.', async(): Promise<void> => {
+  it('deletes count resources once locking is finished.', async(): Promise<void> => {
-    await expect(locker.withReadLock(resourceId, (): any => 5)).resolves.toBe(5);
+    await expect(locker.withReadLock(resourceId, (): number => 5)).resolves.toBe(5);
-  });
+    expect(storageMap.size).toBe(0);
-
+    expect(resourceLocker.acquire).toHaveBeenCalledTimes(3);
-  it('does not block single write operations.', async(): Promise<void> => {
+    expect(resourceLocker.acquire).toHaveBeenNthCalledWith(1, { path: `${resourceId.path}.read` });
-    await expect(locker.withWriteLock(resourceId, (): any => 5)).resolves.toBe(5);
+    expect(resourceLocker.acquire).toHaveBeenNthCalledWith(2, resourceId);
-  });
+    expect(resourceLocker.acquire).toHaveBeenNthCalledWith(3, { path: `${resourceId.path}.read` });
-
+    expect(resourceLocker.release).toHaveBeenCalledTimes(3);
-  it('errors when trying to writeLock a count identifier.', async(): Promise<void> => {
+    expect(resourceLocker.release).toHaveBeenNthCalledWith(1, { path: `${resourceId.path}.read` });
-    await expect(locker.withWriteLock({ path: `http://test.com/foo.count` }, (): any => 5))
+    expect(resourceLocker.release).toHaveBeenNthCalledWith(2, resourceId);
-      .rejects.toThrow(ForbiddenHttpError);
+    expect(resourceLocker.release).toHaveBeenNthCalledWith(3, { path: `${resourceId.path}.read` });
-
-    locker = new GreedyReadWriteLocker(sourceLocker, storage, { count: 'dummy', read: 'read', write: 'write' });
-    await expect(locker.withWriteLock({ path: `http://test.com/foo.dummy` }, (): any => 5))
-      .rejects.toThrow(ForbiddenHttpError);
   });
 
   it('errors if the read counter has an unexpected value.', async(): Promise<void> => {
     storage.get = jest.fn().mockResolvedValue(0);
-    await expect(locker.withReadLock(resourceId, (): any => 5)).rejects.toThrow(InternalServerError);
+    await expect(locker.withReadLock(resourceId, (): number => 5)).rejects.toThrow(InternalServerError);
-  });
-
-  it('does not block multiple read operations.', async(): Promise<void> => {
-    const order: string[] = [];
-    const emitter = new EventEmitter();
-
-    const unlocks = [ 0, 1, 2 ].map((num): any => new Promise((resolve): any => emitter.on(`release${num}`, resolve)));
-    const promises = [ 0, 1, 2 ].map((num): any => locker.withReadLock(resourceId, async(): Promise<number> => {
-      order.push(`start ${num}`);
-      await unlocks[num];
-      order.push(`finish ${num}`);
-      return num;
-    }));
-
-    // Allow time to attach listeners
-    await flushPromises();
-
-    emitter.emit('release2');
-    await expect(promises[2]).resolves.toBe(2);
-    emitter.emit('release0');
-    await expect(promises[0]).resolves.toBe(0);
-    emitter.emit('release1');
-    await expect(promises[1]).resolves.toBe(1);
-
-    expect(order).toEqual([ 'start 0', 'start 1', 'start 2', 'finish 2', 'finish 0', 'finish 1' ]);
-  });
-
-  it('blocks multiple write operations.', async(): Promise<void> => {
-    // Previous test but with write locks
-    const order: string[] = [];
-    const emitter = new EventEmitter();
-
-    const unlocks = [ 0, 1, 2 ].map((num): any => new Promise((resolve): any => emitter.on(`release${num}`, resolve)));
-    const promises = [ 0, 1, 2 ].map((num): any => locker.withWriteLock(resourceId, async(): Promise<number> => {
-      order.push(`start ${num}`);
-      await unlocks[num];
-      order.push(`finish ${num}`);
-      return num;
-    }));
-
-    // Allow time to attach listeners
-    await flushPromises();
-
-    emitter.emit('release2');
-
-    // Allow time to finish write 2
-    await flushPromises();
-
-    emitter.emit('release0');
-    emitter.emit('release1');
-    await Promise.all([ promises[2], promises[0], promises[1] ]);
-    expect(order).toEqual([ 'start 0', 'finish 0', 'start 1', 'finish 1', 'start 2', 'finish 2' ]);
-  });
-
-  it('allows multiple write operations on different resources.', async(): Promise<void> => {
-    // Previous test but with write locks
-    const order: string[] = [];
-    const emitter = new EventEmitter();
-
-    const resources = [ resourceId, resource2Id ];
-    const unlocks = [ 0, 1 ].map((num): any => new Promise((resolve): any => emitter.on(`release${num}`, resolve)));
-    const promises = [ 0, 1 ].map((num): any => locker.withWriteLock(resources[num], async(): Promise<number> => {
-      order.push(`start ${num}`);
-      await unlocks[num];
-      order.push(`finish ${num}`);
-      return num;
-    }));
-
-    // Allow time to attach listeners
-    await flushPromises();
-
-    emitter.emit('release1');
-    await expect(promises[1]).resolves.toBe(1);
-    emitter.emit('release0');
-    await expect(promises[0]).resolves.toBe(0);
-
-    expect(order).toEqual([ 'start 0', 'start 1', 'finish 1', 'finish 0' ]);
-  });
-
-  it('blocks write operations during read operations.', async(): Promise<void> => {
-    const order: string[] = [];
-    const emitter = new EventEmitter();
-
-    const promRead = new Promise((resolve): any => {
-      emitter.on('releaseRead', resolve);
-    });
-
-    // We want to make sure the write operation only starts while the read operation is busy
-    // Otherwise the internal write lock might not be acquired yet
-    const delayedLockWrite = new Promise<void>((resolve): void => {
-      emitter.on('readStarted', (): void => {
-        // eslint-disable-next-line @typescript-eslint/no-floating-promises
-        locker.withWriteLock(resourceId, (): any => {
-          order.push('write');
-          resolve();
-        });
-      });
-    });
-
-    const lockRead = locker.withReadLock(resourceId, async(): Promise<void> => {
-      emitter.emit('readStarted');
-      order.push('read start');
-      await promRead;
-      order.push('read finish');
-    });
-
-    // Allow time to attach listeners
-    await flushPromises();
-
-    const promAll = Promise.all([ delayedLockWrite, lockRead ]);
-
-    emitter.emit('releaseRead');
-    await promAll;
-    expect(order).toEqual([ 'read start', 'read finish', 'write' ]);
-  });
-
-  it('allows write operations on different resources during read operations.', async(): Promise<void> => {
-    const order: string[] = [];
-    const emitter = new EventEmitter();
-
-    const promRead = new Promise((resolve): any => {
-      emitter.on('releaseRead', resolve);
-    });
-
-    const delayedLockWrite = new Promise<void>((resolve): void => {
-      emitter.on('readStarted', (): void => {
-        // eslint-disable-next-line @typescript-eslint/no-floating-promises
-        locker.withWriteLock(resource2Id, (): any => {
-          order.push('write');
-          resolve();
-        });
-      });
-    });
-
-    const lockRead = locker.withReadLock(resourceId, async(): Promise<void> => {
-      emitter.emit('readStarted');
-      order.push('read start');
-      await promRead;
-      order.push('read finish');
-    });
-
-    // Allow time to attach listeners
-    await flushPromises();
-
-    const promAll = Promise.all([ delayedLockWrite, lockRead ]);
-
-    emitter.emit('releaseRead');
-    await promAll;
-    expect(order).toEqual([ 'read start', 'write', 'read finish' ]);
-  });
-
-  it('prioritizes read operations when a read operation is waiting.', async(): Promise<void> => {
-    // This test is very similar to the previous ones but adds an extra read lock
-    const order: string[] = [];
-    const emitter = new EventEmitter();
-
-    const promRead1 = new Promise((resolve): any => emitter.on('releaseRead1', resolve));
-    const promRead2 = new Promise((resolve): any => emitter.on('releaseRead2', resolve));
-
-    const delayedLockWrite = new Promise<void>((resolve): void => {
-      emitter.on('readStarted', (): void => {
-        // eslint-disable-next-line @typescript-eslint/no-floating-promises
-        locker.withWriteLock(resourceId, (): any => {
-          order.push('write');
-          resolve();
-        });
-      });
-    });
-
-    const delayedLockRead2 = new Promise<void>((resolve): void => {
-      emitter.on('readStarted', (): void => {
-        // eslint-disable-next-line @typescript-eslint/no-floating-promises
-        locker.withReadLock(resourceId, async(): Promise<void> => {
-          order.push('read 2 start');
-          await promRead2;
-          order.push('read 2 finish');
-          resolve();
-        });
-      });
-    });
-
-    const lockRead = locker.withReadLock(resourceId, async(): Promise<void> => {
-      emitter.emit('readStarted');
-      order.push('read 1 start');
-      await promRead1;
-      order.push('read 1 finish');
-    });
-
-    // Allow time to attach listeners
-    await flushPromises();
-
-    const promAll = Promise.all([ delayedLockWrite, lockRead, delayedLockRead2 ]);
-
-    emitter.emit('releaseRead1');
-
-    // Allow time to finish read 1
-    await flushPromises();
-
-    emitter.emit('releaseRead2');
-    await promAll;
-    expect(order).toEqual([ 'read 1 start', 'read 2 start', 'read 1 finish', 'read 2 finish', 'write' ]);
-  });
-
-  it('blocks read operations during write operations.', async(): Promise<void> => {
-    // Again similar but with read and write order switched
-    const order: string[] = [];
-    const emitter = new EventEmitter();
-
-    const promWrite = new Promise((resolve): any => {
-      emitter.on('releaseWrite', resolve);
-    });
-
-    // We want to make sure the read operation only starts while the write operation is busy
-    const delayedLockRead = new Promise<void>((resolve): void => {
-      emitter.on('writeStarted', (): void => {
-        // eslint-disable-next-line @typescript-eslint/no-floating-promises
-        locker.withReadLock(resourceId, (): any => {
-          order.push('read');
-          resolve();
-        });
-      });
-    });
-
-    const lockWrite = locker.withWriteLock(resourceId, async(): Promise<void> => {
-      emitter.emit('writeStarted');
-      order.push('write start');
-      await promWrite;
-      order.push('write finish');
-    });
-
-    // Allow time to attach listeners
-    await flushPromises();
-
-    const promAll = Promise.all([ delayedLockRead, lockWrite ]);
-
-    emitter.emit('releaseWrite');
-    await promAll;
-    expect(order).toEqual([ 'write start', 'write finish', 'read' ]);
   });
 });

test/unit/util/locking/PartialReadWriteLocker.test.ts

@@ -0,0 +1,25 @@
+import { PartialReadWriteLocker } from '../../../../src/util/locking/PartialReadWriteLocker';
+import type { ResourceLocker } from '../../../../src/util/locking/ResourceLocker';
+
+describe('A PartialReadWriteLocker', (): void => {
+  let resourceLocker: jest.Mocked<ResourceLocker>;
+  const resourceId = { path: 'http://test.com/resource' };
+  let locker: PartialReadWriteLocker;
+
+  beforeEach(async(): Promise<void> => {
+    resourceLocker = {
+      acquire: jest.fn(),
+      release: jest.fn(),
+    };
+
+    locker = new PartialReadWriteLocker(resourceLocker);
+  });
+
+  it('can lock resources.', async(): Promise<void> => {
+    await expect(locker.withReadLock(resourceId, (): number => 5)).resolves.toBe(5);
+    expect(resourceLocker.acquire).toHaveBeenCalledTimes(1);
+    expect(resourceLocker.acquire).toHaveBeenLastCalledWith(resourceId);
+    expect(resourceLocker.release).toHaveBeenCalledTimes(1);
+    expect(resourceLocker.release).toHaveBeenLastCalledWith(resourceId);
+  });
+});