Skip to content

Commit

Permalink
fix: Remove locking from the SparqlUpdatePatchHandler
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@joachimvh
joachimvh committed Jan 22, 2021
1 parent b59357e commit 077f5d7
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 50 deletions.
12 changes: 2 additions & 10 deletions src/storage/patch/SparqlUpdatePatchHandler.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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<void> {
Expand All @@ -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 {
Expand Down
45 changes: 5 additions & 40 deletions test/unit/storage/patch/SparqlUpdatePatchHandler.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>;
let source: ResourceStore;
let startQuads: Quad[];
let order: string[];

beforeEach(async(): Promise<void> => {
order = [];

startQuads = [ quad(
namedNode('http://test.com/startS1'),
namedNode('http://test.com/startP1'),
Expand All @@ -37,32 +29,14 @@ describe('A SparqlUpdatePatchHandler', (): void => {
) ];

source = {
getRepresentation: jest.fn(async(): Promise<any> => {
order.push('getRepresentation');
return {
dataType: 'quads',
data: streamifyArray([ ...startQuads ]),
metadata: null,
};
}),
setRepresentation: jest.fn(async(): Promise<any> => {
order.push('setRepresentation');
}),
getRepresentation: jest.fn(async(): Promise<any> => new BasicRepresentation(startQuads, 'internal/quads', false)),
setRepresentation: jest.fn(),
modifyResource: jest.fn(async(): Promise<any> => {
throw new Error('noModify');
}),
} as unknown as ResourceStore;

release = jest.fn(async(): Promise<any> => order.push('release'));
locker = {
acquire: jest.fn(async(): Promise<any> => {
order.push('acquire');
lock = { release };
return lock;
}),
};

handler = new SparqlUpdatePatchHandler(source, locker);
handler = new SparqlUpdatePatchHandler(source);
});

async function basicChecks(quads: Quad[]): Promise<boolean> {
Expand All @@ -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({
Expand Down Expand Up @@ -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<void> => {
Expand All @@ -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<void> => {
Expand All @@ -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<void> => {
Expand All @@ -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<void> => {
Expand All @@ -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<void> => {
it('throws the error returned by the store if there is one.', async(): Promise<void> => {
source.getRepresentation = jest.fn(async(): Promise<any> => {
order.push('getRepresentation');
throw new Error('error');
});

Expand All @@ -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<void> => {
// There is no initial data
startQuads = [];
source.getRepresentation = jest.fn((): any => {
order.push('getRepresentation');
throw new NotFoundHttpError();
});

Expand Down

0 comments on commit 077f5d7

Please sign in to comment.