From 061c856161cd124d7cd292ad8aaec48692aade51 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Tue, 26 Jan 2021 17:29:44 +0100 Subject: [PATCH] feat: Add auxiliary support to LockingResourceStore This way locks are always on the associated resource if there is a request on an auxiliary resource. --- config/presets/storage-wrapper.json | 4 +- src/storage/LockingResourceStore.ts | 28 ++++- test/integration/LockingResourceStore.test.ts | 11 +- .../unit/storage/LockingResourceStore.test.ts | 113 ++++++++++++++---- 4 files changed, 122 insertions(+), 34 deletions(-) diff --git a/config/presets/storage-wrapper.json b/config/presets/storage-wrapper.json index 4205515d0..4d8eb4296 100644 --- a/config/presets/storage-wrapper.json +++ b/config/presets/storage-wrapper.json @@ -38,8 +38,10 @@ }, "LockingResourceStore:_locks": { "@id": "urn:solid-server:default:ResourceLocker" + }, + "LockingResourceStore:_strategy": { + "@id": "urn:solid-server:default:AuxiliaryStrategy" } - }, { diff --git a/src/storage/LockingResourceStore.ts b/src/storage/LockingResourceStore.ts index 2dadaf61c..85b7561b3 100644 --- a/src/storage/LockingResourceStore.ts +++ b/src/storage/LockingResourceStore.ts @@ -1,6 +1,7 @@ import type { Readable } from 'stream'; import { promisify } from 'util'; import eos from 'end-of-stream'; +import type { AuxiliaryIdentifierStrategy } from '../ldp/auxiliary/AuxiliaryIdentifierStrategy'; import type { Patch } from '../ldp/http/Patch'; import { BasicRepresentation } from '../ldp/representation/BasicRepresentation'; import type { Representation } from '../ldp/representation/Representation'; @@ -17,46 +18,61 @@ const endOfStream = promisify(eos); * Store that for every call acquires a lock before executing it on the requested resource, * and releases it afterwards. * In case the request returns a Representation the lock will only be released when the data stream is finished. + * + * For auxiliary resources the lock will be applied to the associated resource. + * The actual operation is still executed on the auxiliary resource. */ export class LockingResourceStore implements AtomicResourceStore { protected readonly logger = getLoggerFor(this); private readonly source: ResourceStore; private readonly locks: ExpiringReadWriteLocker; + private readonly strategy: AuxiliaryIdentifierStrategy; - public constructor(source: ResourceStore, locks: ExpiringReadWriteLocker) { + public constructor(source: ResourceStore, locks: ExpiringReadWriteLocker, strategy: AuxiliaryIdentifierStrategy) { this.source = source; this.locks = locks; + this.strategy = strategy; } public async getRepresentation(identifier: ResourceIdentifier, preferences: RepresentationPreferences, conditions?: Conditions): Promise { - return this.lockedRepresentationRun(identifier, + return this.lockedRepresentationRun(this.getLockIdentifier(identifier), async(): Promise => this.source.getRepresentation(identifier, preferences, conditions)); } public async addResource(container: ResourceIdentifier, representation: Representation, conditions?: Conditions): Promise { - return this.locks.withWriteLock(container, + return this.locks.withWriteLock(this.getLockIdentifier(container), async(): Promise => this.source.addResource(container, representation, conditions)); } public async setRepresentation(identifier: ResourceIdentifier, representation: Representation, conditions?: Conditions): Promise { - return this.locks.withWriteLock(identifier, + return this.locks.withWriteLock(this.getLockIdentifier(identifier), async(): Promise => this.source.setRepresentation(identifier, representation, conditions)); } public async deleteResource(identifier: ResourceIdentifier, conditions?: Conditions): Promise { - return this.locks.withWriteLock(identifier, + return this.locks.withWriteLock(this.getLockIdentifier(identifier), async(): Promise => this.source.deleteResource(identifier, conditions)); } public async modifyResource(identifier: ResourceIdentifier, patch: Patch, conditions?: Conditions): Promise { - return this.locks.withWriteLock(identifier, + return this.locks.withWriteLock(this.getLockIdentifier(identifier), async(): Promise => this.source.modifyResource(identifier, patch, conditions)); } + /** + * Acquires the correct identifier to lock this resource. + * For auxiliary resources this means the associated identifier. + */ + protected getLockIdentifier(identifier: ResourceIdentifier): ResourceIdentifier { + return this.strategy.isAuxiliaryIdentifier(identifier) ? + this.strategy.getAssociatedIdentifier(identifier) : + identifier; + } + /** * Acquires a lock that is only released when all data of the resulting representation data has been read, * an error occurs, or the timeout has been triggered. diff --git a/test/integration/LockingResourceStore.test.ts b/test/integration/LockingResourceStore.test.ts index 9508f9cce..abc068169 100644 --- a/test/integration/LockingResourceStore.test.ts +++ b/test/integration/LockingResourceStore.test.ts @@ -1,4 +1,5 @@ import { RootContainerInitializer } from '../../src/init/RootContainerInitializer'; +import { RoutingAuxiliaryStrategy } from '../../src/ldp/auxiliary/RoutingAuxiliaryStrategy'; import { BasicRepresentation } from '../../src/ldp/representation/BasicRepresentation'; import type { Representation } from '../../src/ldp/representation/Representation'; import { InMemoryDataAccessor } from '../../src/storage/accessors/InMemoryDataAccessor'; @@ -29,9 +30,15 @@ describe('A LockingResourceStore', (): void => { beforeEach(async(): Promise => { jest.clearAllMocks(); + // Not relevant for these tests + const strategy = new RoutingAuxiliaryStrategy([]); + const base = 'http://test.com/'; path = `${base}path`; - source = new DataAccessorBasedStore(new InMemoryDataAccessor(base), new SingleRootIdentifierStrategy(base)); + source = new DataAccessorBasedStore( + new InMemoryDataAccessor(base), + new SingleRootIdentifierStrategy(base), + ); // Initialize store const initializer = new RootContainerInitializer({ store: source, baseUrl: BASE }); @@ -40,7 +47,7 @@ describe('A LockingResourceStore', (): void => { locker = new EqualReadWriteLocker(new SingleThreadedResourceLocker()); expiringLocker = new WrappedExpiringReadWriteLocker(locker, 1000); - store = new LockingResourceStore(source, expiringLocker); + store = new LockingResourceStore(source, expiringLocker, strategy); // Spy on a real ResourceLocker and ResourceStore instance getRepresentationSpy = jest.spyOn(source, 'getRepresentation'); diff --git a/test/unit/storage/LockingResourceStore.test.ts b/test/unit/storage/LockingResourceStore.test.ts index 642a9d1ee..837f3be8c 100644 --- a/test/unit/storage/LockingResourceStore.test.ts +++ b/test/unit/storage/LockingResourceStore.test.ts @@ -1,4 +1,5 @@ import { EventEmitter } from 'events'; +import type { AuxiliaryIdentifierStrategy } from '../../../src/ldp/auxiliary/AuxiliaryIdentifierStrategy'; import type { Patch } from '../../../src/ldp/http/Patch'; import type { Representation } from '../../../src/ldp/representation/Representation'; import type { ResourceIdentifier } from '../../../src/ldp/representation/ResourceIdentifier'; @@ -12,9 +13,13 @@ function emptyFn(): void { } describe('A LockingResourceStore', (): void => { + const auxiliaryId = { path: 'http://test.com/foo.dummy' }; + const associatedId = { path: 'http://test.com/foo' }; + const data = { data: 'data!' } as any; let store: LockingResourceStore; let locker: ExpiringReadWriteLocker; let source: ResourceStore; + let strategy: AuxiliaryIdentifierStrategy; let order: string[]; let timeoutTrigger: EventEmitter; @@ -64,7 +69,12 @@ describe('A LockingResourceStore', (): void => { }), }; - store = new LockingResourceStore(source, locker); + strategy = { + isAuxiliaryIdentifier: jest.fn((id: ResourceIdentifier): any => id.path.endsWith('.dummy')), + getAssociatedIdentifier: jest.fn((id: ResourceIdentifier): any => ({ path: id.path.slice(0, -6) })), + } as any; + + store = new LockingResourceStore(source, locker, strategy); }); function registerEventOrder(eventSource: EventEmitter, event: string): void { @@ -74,34 +84,70 @@ describe('A LockingResourceStore', (): void => { } it('acquires a lock on the container when adding a representation.', async(): Promise => { - await store.addResource({ path: 'path' }, {} as Representation); + await store.addResource(associatedId, data); expect(locker.withWriteLock).toHaveBeenCalledTimes(1); - expect((locker.withWriteLock as jest.Mock).mock.calls[0][0]).toEqual({ path: 'path' }); + expect((locker.withWriteLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); expect(source.addResource).toHaveBeenCalledTimes(1); + expect(source.addResource).toHaveBeenLastCalledWith(associatedId, data, undefined); + expect(order).toEqual([ 'lock write', 'addResource', 'unlock write' ]); + + order = []; + await expect(store.addResource(auxiliaryId, data)).resolves.toBeUndefined(); + expect(locker.withWriteLock).toHaveBeenCalledTimes(2); + expect((locker.withWriteLock as jest.Mock).mock.calls[1][0]).toEqual(associatedId); + expect(source.addResource).toHaveBeenCalledTimes(2); + expect(source.addResource).toHaveBeenLastCalledWith(auxiliaryId, data, undefined); expect(order).toEqual([ 'lock write', 'addResource', 'unlock write' ]); }); it('acquires a lock on the resource when setting its representation.', async(): Promise => { - await store.setRepresentation({ path: 'path' }, {} as Representation); + await store.setRepresentation(associatedId, data); expect(locker.withWriteLock).toHaveBeenCalledTimes(1); - expect((locker.withWriteLock as jest.Mock).mock.calls[0][0]).toEqual({ path: 'path' }); + expect((locker.withWriteLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); expect(source.setRepresentation).toHaveBeenCalledTimes(1); + expect(source.setRepresentation).toHaveBeenLastCalledWith(associatedId, data, undefined); + expect(order).toEqual([ 'lock write', 'setRepresentation', 'unlock write' ]); + + order = []; + await expect(store.setRepresentation(auxiliaryId, data)).resolves.toBeUndefined(); + expect(locker.withWriteLock).toHaveBeenCalledTimes(2); + expect((locker.withWriteLock as jest.Mock).mock.calls[1][0]).toEqual(associatedId); + expect(source.setRepresentation).toHaveBeenCalledTimes(2); + expect(source.setRepresentation).toHaveBeenLastCalledWith(auxiliaryId, data, undefined); expect(order).toEqual([ 'lock write', 'setRepresentation', 'unlock write' ]); }); it('acquires a lock on the resource when deleting it.', async(): Promise => { - await store.deleteResource({ path: 'path' }); + await store.deleteResource(associatedId); expect(locker.withWriteLock).toHaveBeenCalledTimes(1); - expect((locker.withWriteLock as jest.Mock).mock.calls[0][0]).toEqual({ path: 'path' }); + expect((locker.withWriteLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); expect(source.deleteResource).toHaveBeenCalledTimes(1); + expect(source.deleteResource).toHaveBeenLastCalledWith(associatedId, undefined); + expect(order).toEqual([ 'lock write', 'deleteResource', 'unlock write' ]); + + order = []; + await expect(store.deleteResource(auxiliaryId)).resolves.toBeUndefined(); + expect(locker.withWriteLock).toHaveBeenCalledTimes(2); + expect((locker.withWriteLock as jest.Mock).mock.calls[1][0]).toEqual(associatedId); + expect(source.deleteResource).toHaveBeenCalledTimes(2); + expect(source.deleteResource).toHaveBeenLastCalledWith(auxiliaryId, undefined); expect(order).toEqual([ 'lock write', 'deleteResource', 'unlock write' ]); }); it('acquires a lock on the resource when modifying its representation.', async(): Promise => { - await store.modifyResource({ path: 'path' }, {} as Patch); + await store.modifyResource(associatedId, data as Patch); expect(locker.withWriteLock).toHaveBeenCalledTimes(1); - expect((locker.withWriteLock as jest.Mock).mock.calls[0][0]).toEqual({ path: 'path' }); + expect((locker.withWriteLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); expect(source.modifyResource).toHaveBeenCalledTimes(1); + expect(source.modifyResource).toHaveBeenLastCalledWith(associatedId, data, undefined); + expect(order).toEqual([ 'lock write', 'modifyResource', 'unlock write' ]); + + order = []; + await expect(store.modifyResource(auxiliaryId, data as Patch)).resolves.toBeUndefined(); + expect(locker.withWriteLock).toHaveBeenCalledTimes(2); + expect((locker.withWriteLock as jest.Mock).mock.calls[1][0]).toEqual(associatedId); + expect(source.modifyResource).toHaveBeenCalledTimes(2); + expect(source.modifyResource).toHaveBeenLastCalledWith(auxiliaryId, data, undefined); expect(order).toEqual([ 'lock write', 'modifyResource', 'unlock write' ]); }); @@ -110,15 +156,15 @@ describe('A LockingResourceStore', (): void => { order.push('bad get'); throw new Error('dummy'); }; - await expect(store.getRepresentation({ path: 'path' }, {})).rejects.toThrow('dummy'); + await expect(store.getRepresentation(associatedId, {})).rejects.toThrow('dummy'); expect(locker.withReadLock).toHaveBeenCalledTimes(1); - expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual({ path: 'path' }); + expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); expect(order).toEqual([ 'lock read', 'bad get', 'unlock read' ]); }); it('releases the lock on the resource when data has been read.', async(): Promise => { // Read all data from the representation - const representation = await store.getRepresentation({ path: 'path' }, {}); + const representation = await store.getRepresentation(associatedId, {}); representation.data.on('data', (): any => true); registerEventOrder(representation.data, 'end'); @@ -127,14 +173,32 @@ describe('A LockingResourceStore', (): void => { // Verify the lock was acquired and released at the right time expect(locker.withReadLock).toHaveBeenCalledTimes(1); - expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual({ path: 'path' }); + expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); expect(source.getRepresentation).toHaveBeenCalledTimes(1); + expect(source.getRepresentation).toHaveBeenLastCalledWith(associatedId, {}, undefined); + expect(order).toEqual([ 'lock read', 'getRepresentation', 'end', 'unlock read' ]); + }); + + it('acquires the lock on the associated resource when reading an auxiliary resource.', async(): Promise => { + // Read all data from the representation + const representation = await store.getRepresentation(auxiliaryId, {}); + representation.data.on('data', (): any => true); + registerEventOrder(representation.data, 'end'); + + // Provide opportunity for async events + await new Promise(setImmediate); + + // Verify the lock was acquired and released at the right time + expect(locker.withReadLock).toHaveBeenCalledTimes(1); + expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); + expect(source.getRepresentation).toHaveBeenCalledTimes(1); + expect(source.getRepresentation).toHaveBeenLastCalledWith(auxiliaryId, {}, undefined); expect(order).toEqual([ 'lock read', 'getRepresentation', 'end', 'unlock read' ]); }); it('destroys the resource and releases the lock when the readable errors.', async(): Promise => { // Make the representation error - const representation = await store.getRepresentation({ path: 'path' }, {}); + const representation = await store.getRepresentation(associatedId, {}); setImmediate((): any => representation.data.emit('error', new Error('Error on the readable'))); registerEventOrder(representation.data, 'error'); registerEventOrder(representation.data, 'close'); @@ -144,7 +208,7 @@ describe('A LockingResourceStore', (): void => { // Verify the lock was acquired and released at the right time expect(locker.withReadLock).toHaveBeenCalledTimes(1); - expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual({ path: 'path' }); + expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); expect(source.getRepresentation).toHaveBeenCalledTimes(1); expect(representation.data.destroy).toHaveBeenCalledTimes(1); expect(order).toEqual([ 'lock read', 'getRepresentation', 'error', 'unlock read', 'close' ]); @@ -152,7 +216,7 @@ describe('A LockingResourceStore', (): void => { it('releases the lock on the resource when readable is destroyed.', async(): Promise => { // Make the representation close - const representation = await store.getRepresentation({ path: 'path' }, {}); + const representation = await store.getRepresentation(associatedId, {}); representation.data.destroy(); registerEventOrder(representation.data, 'close'); @@ -161,33 +225,32 @@ describe('A LockingResourceStore', (): void => { // Verify the lock was acquired and released at the right time expect(locker.withReadLock).toHaveBeenCalledTimes(1); - expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual({ path: 'path' }); + expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); expect(source.getRepresentation).toHaveBeenCalledTimes(1); expect(order).toEqual([ 'lock read', 'getRepresentation', 'close', 'unlock read' ]); }); it('releases the lock only once when multiple events are triggered.', async(): Promise => { // Read all data from the representation and trigger an additional close event - const representation = await store.getRepresentation({ path: 'path' }, {}); + const representation = await store.getRepresentation(associatedId, {}); representation.data.on('data', (): any => true); representation.data.prependListener('end', (): any => { order.push('end'); representation.data.destroy(); }); - registerEventOrder(representation.data, 'close'); // Provide opportunity for async events await new Promise(setImmediate); // Verify the lock was acquired and released at the right time expect(locker.withReadLock).toHaveBeenCalledTimes(1); - expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual({ path: 'path' }); + expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); expect(source.getRepresentation).toHaveBeenCalledTimes(1); - expect(order).toEqual([ 'lock read', 'getRepresentation', 'end', 'close', 'unlock read' ]); + expect(order).toEqual([ 'lock read', 'getRepresentation', 'end', 'unlock read' ]); }); it('releases the lock on the resource when readable times out.', async(): Promise => { - const representation = await store.getRepresentation({ path: 'path' }, {}); + const representation = await store.getRepresentation(associatedId, {}); registerEventOrder(representation.data, 'close'); registerEventOrder(representation.data, 'error'); @@ -198,7 +261,7 @@ describe('A LockingResourceStore', (): void => { // Verify the lock was acquired and released at the right time expect(locker.withReadLock).toHaveBeenCalledTimes(1); - expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual({ path: 'path' }); + expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); expect(source.getRepresentation).toHaveBeenCalledTimes(1); expect(representation.data.destroy).toHaveBeenCalledTimes(1); expect(representation.data.destroy).toHaveBeenLastCalledWith(new Error('timeout')); @@ -212,13 +275,13 @@ describe('A LockingResourceStore', (): void => { return new Promise(emptyFn); }); - const prom = store.getRepresentation({ path: 'path' }, {}); + const prom = store.getRepresentation(associatedId, {}); timeoutTrigger.emit('timeout'); await expect(prom).rejects.toThrow('timeout'); expect(locker.withReadLock).toHaveBeenCalledTimes(1); - expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual({ path: 'path' }); + expect((locker.withReadLock as jest.Mock).mock.calls[0][0]).toEqual(associatedId); expect(source.getRepresentation).toHaveBeenCalledTimes(1); expect(order).toEqual([ 'lock read', 'useless get', 'timeout', 'unlock read' ]); });