From 077f5d7069ff94108b56d4ebbfc5881a8280955c Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Wed, 20 Jan 2021 11:34:42 +0100 Subject: [PATCH] fix: Remove locking from the SparqlUpdatePatchHandler Due to not having re-entrant locks this would cause deadlocks with the LockingResourceStore or require more advanced configurations. If this is needed in the future we can potentially add a LockingPatchHandler. --- src/storage/patch/SparqlUpdatePatchHandler.ts | 12 +---- .../patch/SparqlUpdatePatchHandler.test.ts | 45 +++---------------- 2 files changed, 7 insertions(+), 50 deletions(-) diff --git a/src/storage/patch/SparqlUpdatePatchHandler.ts b/src/storage/patch/SparqlUpdatePatchHandler.ts index 53a7ce187..485f82eaa 100644 --- a/src/storage/patch/SparqlUpdatePatchHandler.ts +++ b/src/storage/patch/SparqlUpdatePatchHandler.ts @@ -11,7 +11,6 @@ import { getLoggerFor } from '../../logging/LogUtil'; import { INTERNAL_QUADS } from '../../util/ContentTypes'; import { NotFoundHttpError } from '../../util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError'; -import type { ResourceLocker } from '../../util/locking/ResourceLocker'; import type { ResourceStore } from '../ResourceStore'; import { PatchHandler } from './PatchHandler'; @@ -23,12 +22,10 @@ export class SparqlUpdatePatchHandler extends PatchHandler { protected readonly logger = getLoggerFor(this); private readonly source: ResourceStore; - private readonly locker: ResourceLocker; - public constructor(source: ResourceStore, locker: ResourceLocker) { + public constructor(source: ResourceStore) { super(); this.source = source; - this.locker = locker; } public async canHandle(input: {identifier: ResourceIdentifier; patch: SparqlUpdatePatch}): Promise { @@ -43,12 +40,7 @@ export class SparqlUpdatePatchHandler extends PatchHandler { const op = patch.algebra; this.validateUpdate(op); - const lock = await this.locker.acquire(identifier); - try { - await this.applyPatch(identifier, op); - } finally { - await lock.release(); - } + await this.applyPatch(identifier, op); } private isDeleteInsert(op: Algebra.Operation): op is Algebra.DeleteInsert { diff --git a/test/unit/storage/patch/SparqlUpdatePatchHandler.test.ts b/test/unit/storage/patch/SparqlUpdatePatchHandler.test.ts index 22f4274f3..96f5381c2 100644 --- a/test/unit/storage/patch/SparqlUpdatePatchHandler.test.ts +++ b/test/unit/storage/patch/SparqlUpdatePatchHandler.test.ts @@ -3,29 +3,21 @@ import { namedNode, quad } from '@rdfjs/data-model'; import arrayifyStream from 'arrayify-stream'; import type { Quad } from 'rdf-js'; import { translate } from 'sparqlalgebrajs'; -import streamifyArray from 'streamify-array'; import type { SparqlUpdatePatch } from '../../../../src/ldp/http/SparqlUpdatePatch'; +import { BasicRepresentation } from '../../../../src/ldp/representation/BasicRepresentation'; import { RepresentationMetadata } from '../../../../src/ldp/representation/RepresentationMetadata'; import { SparqlUpdatePatchHandler } from '../../../../src/storage/patch/SparqlUpdatePatchHandler'; import type { ResourceStore } from '../../../../src/storage/ResourceStore'; import { INTERNAL_QUADS } from '../../../../src/util/ContentTypes'; import { NotFoundHttpError } from '../../../../src/util/errors/NotFoundHttpError'; import { NotImplementedHttpError } from '../../../../src/util/errors/NotImplementedHttpError'; -import type { Lock } from '../../../../src/util/locking/Lock'; -import type { ResourceLocker } from '../../../../src/util/locking/ResourceLocker'; describe('A SparqlUpdatePatchHandler', (): void => { let handler: SparqlUpdatePatchHandler; - let locker: ResourceLocker; - let lock: Lock; - let release: () => Promise; let source: ResourceStore; let startQuads: Quad[]; - let order: string[]; beforeEach(async(): Promise => { - order = []; - startQuads = [ quad( namedNode('http://test.com/startS1'), namedNode('http://test.com/startP1'), @@ -37,32 +29,14 @@ describe('A SparqlUpdatePatchHandler', (): void => { ) ]; source = { - getRepresentation: jest.fn(async(): Promise => { - order.push('getRepresentation'); - return { - dataType: 'quads', - data: streamifyArray([ ...startQuads ]), - metadata: null, - }; - }), - setRepresentation: jest.fn(async(): Promise => { - order.push('setRepresentation'); - }), + getRepresentation: jest.fn(async(): Promise => new BasicRepresentation(startQuads, 'internal/quads', false)), + setRepresentation: jest.fn(), modifyResource: jest.fn(async(): Promise => { throw new Error('noModify'); }), } as unknown as ResourceStore; - release = jest.fn(async(): Promise => order.push('release')); - locker = { - acquire: jest.fn(async(): Promise => { - order.push('acquire'); - lock = { release }; - return lock; - }), - }; - - handler = new SparqlUpdatePatchHandler(source, locker); + handler = new SparqlUpdatePatchHandler(source); }); async function basicChecks(quads: Quad[]): Promise { @@ -71,7 +45,6 @@ describe('A SparqlUpdatePatchHandler', (): void => { { path: 'path' }, { type: { [INTERNAL_QUADS]: 1 }}, ); expect(source.setRepresentation).toHaveBeenCalledTimes(1); - expect(order).toEqual([ 'acquire', 'getRepresentation', 'setRepresentation', 'release' ]); const setParams = (source.setRepresentation as jest.Mock).mock.calls[0]; expect(setParams[0]).toEqual({ path: 'path' }); expect(setParams[1]).toEqual(expect.objectContaining({ @@ -197,7 +170,6 @@ describe('A SparqlUpdatePatchHandler', (): void => { { quads: true }, ) } as SparqlUpdatePatch }); await expect(handle).rejects.toThrow('GRAPH statements are not supported'); - expect(order).toEqual([]); }); it('rejects GRAPH deletes.', async(): Promise => { @@ -208,7 +180,6 @@ describe('A SparqlUpdatePatchHandler', (): void => { { quads: true }, ) } as SparqlUpdatePatch }); await expect(handle).rejects.toThrow('GRAPH statements are not supported'); - expect(order).toEqual([]); }); it('rejects DELETE/INSERT updates with a non-empty WHERE.', async(): Promise => { @@ -220,7 +191,6 @@ describe('A SparqlUpdatePatchHandler', (): void => { { quads: true }, ) } as SparqlUpdatePatch }); await expect(handle).rejects.toThrow('WHERE statements are not supported'); - expect(order).toEqual([]); }); it('rejects DELETE WHERE updates with variables.', async(): Promise => { @@ -230,7 +200,6 @@ describe('A SparqlUpdatePatchHandler', (): void => { { quads: true }, ) } as SparqlUpdatePatch }); await expect(handle).rejects.toThrow('WHERE statements are not supported'); - expect(order).toEqual([]); }); it('rejects non-DELETE/INSERT updates.', async(): Promise => { @@ -240,12 +209,10 @@ describe('A SparqlUpdatePatchHandler', (): void => { { quads: true }, ) } as SparqlUpdatePatch }); await expect(handle).rejects.toThrow('Only DELETE/INSERT SPARQL update operations are supported'); - expect(order).toEqual([]); }); - it('releases the lock if an error occurs while patching.', async(): Promise => { + it('throws the error returned by the store if there is one.', async(): Promise => { source.getRepresentation = jest.fn(async(): Promise => { - order.push('getRepresentation'); throw new Error('error'); }); @@ -256,14 +223,12 @@ describe('A SparqlUpdatePatchHandler', (): void => { { quads: true }, ) } as SparqlUpdatePatch }; await expect(handler.handle(input)).rejects.toThrow('error'); - expect(order).toEqual([ 'acquire', 'getRepresentation', 'release' ]); }); it('creates a new resource if it does not exist yet.', async(): Promise => { // There is no initial data startQuads = []; source.getRepresentation = jest.fn((): any => { - order.push('getRepresentation'); throw new NotFoundHttpError(); });