diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index df9587f3c..cdbffebd0 100644 --- a/RELEASE_NOTES.md +++ b/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 diff --git a/config/util/resource-locker/file.json b/config/util/resource-locker/file.json index 9ec454d20..24d1e6322 100644 --- a/config/util/resource-locker/file.json +++ b/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", diff --git a/config/util/resource-locker/memory.json b/config/util/resource-locker/memory.json index def462c0e..120420b2c 100644 --- a/config/util/resource-locker/memory.json +++ b/config/util/resource-locker/memory.json @@ -10,10 +10,7 @@ "locker": { "@type": "MemoryResourceLocker" }, - "storage": { "@id": "urn:solid-server:default:LockStorage" }, - "suffixes_count": "count", - "suffixes_read": "read", - "suffixes_write": "write" + "storage": { "@id": "urn:solid-server:default:LockStorage" } }, "expiration": 6000 } diff --git a/src/index.ts b/src/index.ts index 11f75f89a..217958b0e 100644 --- a/src/index.ts +++ b/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'; diff --git a/src/server/AuthorizingHttpHandler.ts b/src/server/AuthorizingHttpHandler.ts index de36bf400..4b061fad6 100644 --- a/src/server/AuthorizingHttpHandler.ts +++ b/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 }); diff --git a/src/server/WacAllowHttpHandler.ts b/src/server/WacAllowHttpHandler.ts index 8e63a58d9..04bd2457d 100644 --- a/src/server/WacAllowHttpHandler.ts +++ b/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) ?? {}; } diff --git a/src/util/locking/BaseReadWriteLocker.ts b/src/util/locking/BaseReadWriteLocker.ts new file mode 100644 index 000000000..5746f00fd --- /dev/null +++ b/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(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue): Promise { + 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 { + await this.withInternalCountLock(identifier, async(): Promise => { + 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 { + await this.withInternalCountLock(identifier, async(): Promise => { + 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(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue): + Promise { + 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; +} diff --git a/src/util/locking/EqualReadWriteLocker.ts b/src/util/locking/EqualReadWriteLocker.ts index e381cd1e0..77e96c852 100644 --- a/src/util/locking/EqualReadWriteLocker.ts +++ b/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(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue): Promise { + protected async withLock(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue): Promise { await this.locker.acquire(identifier); try { return await whileLocked(); diff --git a/src/util/locking/GreedyReadWriteLocker.ts b/src/util/locking/GreedyReadWriteLocker.ts index 5023db4e8..565c37d38 100644 --- a/src/util/locking/GreedyReadWriteLocker.ts +++ b/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 type { ReadWriteLocker } from './ReadWriteLocker'; +import { BaseReadWriteLocker } from './BaseReadWriteLocker'; import type { ResourceLocker } from './ResourceLocker'; -export interface GreedyReadWriteSuffixes { - count: string; - read: string; - write: string; -} - /** - * 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. + * A {@link BaseReadWriteLocker} that uses the same locker for the main lock and the count lock, + * and uses a {@link KeyValueStorage} for keeping track of the counter. * - * 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 write lock is locked. - * 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. + * Since it is completely dependent on other implementations, + * this locker is threadsafe if its inputs are as well. */ -export class GreedyReadWriteLocker implements ReadWriteLocker { - private readonly locker: ResourceLocker; - private readonly storage: KeyValueStorage; - private readonly suffixes: GreedyReadWriteSuffixes; +export class GreedyReadWriteLocker extends BaseReadWriteLocker { + protected readonly storage: KeyValueStorage; + protected readonly readSuffix: string; + 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. - * `count` is used for the identifier used to store the counter. - * `read` and `write` are used for the 2 types of locks that are needed. + * @param readSuffix - Used to generate the identifier for the lock that is applied when updating the counter. + * @param countSuffix - Used to generate the identifier that will be used in the storage for storing the counter. */ public constructor(locker: ResourceLocker, storage: KeyValueStorage, - suffixes: GreedyReadWriteSuffixes = { count: 'count', read: 'read', write: 'write' }) { - this.locker = locker; + readSuffix = 'read', countSuffix = 'count') { + super(locker, locker); this.storage = storage; - this.suffixes = suffixes; + this.readSuffix = readSuffix; + this.countSuffix = countSuffix; } - public async withReadLock(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue): Promise { - await this.preReadSetup(identifier); - try { - return await whileLocked(); - } finally { - await this.postReadCleanup(identifier); - } - } - - public async withWriteLock(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue): Promise { - 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); - } + protected getCountLockIdentifier(identifier: ResourceIdentifier): ResourceIdentifier { + return { path: `${identifier.path}.${this.readSuffix}` }; } /** * This key is used for storing the count of active read operations. */ - private getCountKey(identifier: ResourceIdentifier): string { - return `${identifier.path}.${this.suffixes.count}`; - } - - /** - * 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 { - await this.withInternalReadLock(identifier, async(): Promise => { - 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 { - await this.withInternalReadLock(identifier, async(): Promise => { - 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(identifier: ResourceIdentifier, whileLocked: () => PromiseOrValue): - Promise { - const read = this.getReadLockKey(identifier); - await this.locker.acquire(read); - try { - return await whileLocked(); - } finally { - await this.locker.release(read); - } + protected getCountKey(identifier: ResourceIdentifier): string { + return `${identifier.path}.${this.countSuffix}`; } /** @@ -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 { + protected async modifyCount(identifier: ResourceIdentifier, mod: number): Promise { const countKey = this.getCountKey(identifier); let number = await this.storage.get(countKey) ?? 0; number += mod; diff --git a/src/util/locking/PartialReadWriteLocker.ts b/src/util/locking/PartialReadWriteLocker.ts new file mode 100644 index 000000000..d42455b5d --- /dev/null +++ b/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; + + 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; + } +} diff --git a/test/integration/Identity.test.ts b/test/integration/Identity.test.ts index 02897881d..8450e204f 100644 --- a/test/integration/Identity.test.ts +++ b/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 => removeFolder(rootFilePath), + }], +]; + // Don't send actual e-mails jest.mock('nodemailer'); @@ -37,7 +49,7 @@ async function postForm(url: string, formBody: string): Promise { // 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'), - getDefaultVariables(port, baseUrl), + getTestConfigPath(config), + { + ...getDefaultVariables(port, baseUrl), + 'urn:solid-server:default:variable:rootFilePath': rootFilePath, + }, ) as Record; ({ 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 => { + await teardown(); await app.stop(); }); diff --git a/test/unit/util/locking/BaseReadWriteLocker.test.ts b/test/unit/util/locking/BaseReadWriteLocker.test.ts new file mode 100644 index 000000000..b3aeb9a5f --- /dev/null +++ b/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 void)[]>; + + public constructor() { + this.locks = {}; + } + + public async acquire(identifier: ResourceIdentifier): Promise { + 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 { + 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; + + public constructor(resourceLocker: ResourceLocker, countLocker: ResourceLocker) { + super(resourceLocker, countLocker); + this.countMap = new Map(); + } + + 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 => { + resourceLocker = new MemoryLocker(); + countLocker = new MemoryLocker(); + + locker = new SimpleReadWriteLocker(resourceLocker, countLocker); + }); + + it('does not block single read operations.', async(): Promise => { + await expect(locker.withReadLock(resourceId, (): number => 5)).resolves.toBe(5); + }); + + it('does not block single write operations.', async(): Promise => { + await expect(locker.withWriteLock(resourceId, (): number => 5)).resolves.toBe(5); + }); + + it('does not block multiple read operations.', async(): Promise => { + 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 => { + 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 => { + // 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 => { + 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 => { + // 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 => { + 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 => { + 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((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 => { + 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 => { + const order: string[] = []; + const emitter = new EventEmitter(); + + const promRead = new Promise((resolve): any => { + emitter.on('releaseRead', resolve); + }); + + const delayedLockWrite = new Promise((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 => { + 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 => { + // 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((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((resolve): void => { + emitter.on('readStarted', (): void => { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + locker.withReadLock(resourceId, async(): Promise => { + order.push('read 2 start'); + await promRead2; + order.push('read 2 finish'); + resolve(); + }); + }); + }); + + const lockRead = locker.withReadLock(resourceId, async(): Promise => { + 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 => { + // 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((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 => { + 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' ]); + }); +}); diff --git a/test/unit/util/locking/GreedyReadWriteLocker.test.ts b/test/unit/util/locking/GreedyReadWriteLocker.test.ts index bacf7f846..d8dd7c595 100644 --- a/test/unit/util/locking/GreedyReadWriteLocker.test.ts +++ b/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 void)[]>; - - public constructor() { - this.locks = {}; - } - - public async acquire(identifier: ResourceIdentifier): Promise { - 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 { - 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; + let storageMap: Map; let storage: KeyValueStorage; const resourceId = { path: 'http://test.com/resource' }; - const resource2Id = { path: 'http://test.com/resource2' }; let locker: GreedyReadWriteLocker; beforeEach(async(): Promise => { - sourceLocker = new MemoryLocker(); + resourceLocker = { + acquire: jest.fn(), + release: jest.fn(), + }; - storage = new Map() as any; + storageMap = new Map(); + storage = storageMap as any; - locker = new GreedyReadWriteLocker(sourceLocker, storage); + locker = new GreedyReadWriteLocker(resourceLocker, storage); }); - it('does not block single read operations.', async(): Promise => { - await expect(locker.withReadLock(resourceId, (): any => 5)).resolves.toBe(5); - }); - - it('does not block single write operations.', async(): Promise => { - await expect(locker.withWriteLock(resourceId, (): any => 5)).resolves.toBe(5); - }); - - it('errors when trying to writeLock a count identifier.', async(): Promise => { - await expect(locker.withWriteLock({ path: `http://test.com/foo.count` }, (): any => 5)) - .rejects.toThrow(ForbiddenHttpError); - - 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('deletes count resources once locking is finished.', async(): Promise => { + await expect(locker.withReadLock(resourceId, (): number => 5)).resolves.toBe(5); + expect(storageMap.size).toBe(0); + expect(resourceLocker.acquire).toHaveBeenCalledTimes(3); + expect(resourceLocker.acquire).toHaveBeenNthCalledWith(1, { path: `${resourceId.path}.read` }); + expect(resourceLocker.acquire).toHaveBeenNthCalledWith(2, resourceId); + expect(resourceLocker.acquire).toHaveBeenNthCalledWith(3, { path: `${resourceId.path}.read` }); + expect(resourceLocker.release).toHaveBeenCalledTimes(3); + expect(resourceLocker.release).toHaveBeenNthCalledWith(1, { path: `${resourceId.path}.read` }); + expect(resourceLocker.release).toHaveBeenNthCalledWith(2, resourceId); + expect(resourceLocker.release).toHaveBeenNthCalledWith(3, { path: `${resourceId.path}.read` }); }); it('errors if the read counter has an unexpected value.', async(): Promise => { storage.get = jest.fn().mockResolvedValue(0); - await expect(locker.withReadLock(resourceId, (): any => 5)).rejects.toThrow(InternalServerError); - }); - - it('does not block multiple read operations.', async(): Promise => { - 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 => { - 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 => { - // 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 => { - 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 => { - // 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 => { - 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 => { - 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((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 => { - 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 => { - const order: string[] = []; - const emitter = new EventEmitter(); - - const promRead = new Promise((resolve): any => { - emitter.on('releaseRead', resolve); - }); - - const delayedLockWrite = new Promise((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 => { - 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 => { - // 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((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((resolve): void => { - emitter.on('readStarted', (): void => { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - locker.withReadLock(resourceId, async(): Promise => { - order.push('read 2 start'); - await promRead2; - order.push('read 2 finish'); - resolve(); - }); - }); - }); - - const lockRead = locker.withReadLock(resourceId, async(): Promise => { - 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 => { - // 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((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 => { - 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' ]); + await expect(locker.withReadLock(resourceId, (): number => 5)).rejects.toThrow(InternalServerError); }); }); diff --git a/test/unit/util/locking/PartialReadWriteLocker.test.ts b/test/unit/util/locking/PartialReadWriteLocker.test.ts new file mode 100644 index 000000000..eb55bd3c0 --- /dev/null +++ b/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; + const resourceId = { path: 'http://test.com/resource' }; + let locker: PartialReadWriteLocker; + + beforeEach(async(): Promise => { + resourceLocker = { + acquire: jest.fn(), + release: jest.fn(), + }; + + locker = new PartialReadWriteLocker(resourceLocker); + }); + + it('can lock resources.', async(): Promise => { + 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); + }); +});