From 1c65b06392da60ad5178c7db86c1bd4d6d83f800 Mon Sep 17 00:00:00 2001 From: Wannes Kerckhove Date: Mon, 30 May 2022 15:49:22 +0200 Subject: [PATCH] fix: Introducing initializers for handling lock cleanup on start --- RELEASE_NOTES.md | 18 +++- config/app/init/base/init.json | 8 ++ config/app/main/default.json | 30 +++++-- .../handler/account-store/default.json | 9 +- config/identity/ownership/token.json | 9 +- .../readers/access-checkers/agent-group.json | 9 +- config/util/resource-locker/file.json | 24 ++++-- config/util/resource-locker/redis.json | 22 +++-- package.json | 1 + src/index.ts | 5 +- src/init/App.ts | 8 +- src/init/Initializable.ts | 8 ++ src/init/InitializableHandler.ts | 18 ++++ src/init/Initializer.ts | 3 + src/init/final/Finalizable.ts | 2 + src/init/final/FinalizableHandler.ts | 18 ++++ src/init/final/Finalizer.ts | 6 ++ src/init/final/ParallelFinalizer.ts | 16 ---- src/util/locking/FileSystemResourceLocker.ts | 55 ++++++++---- src/util/locking/RedisLocker.ts | 45 ++++++---- test/integration/RedisLocker.test.ts | 2 +- test/integration/ResourceLockCleanup.test.ts | 84 +++++++++++++++++++ test/integration/SparqlStorage.test.ts | 2 +- test/integration/config/server-file.json | 12 ++- .../integration/config/server-redis-lock.json | 4 + test/unit/init/App.test.ts | 8 +- test/unit/init/InitializableHandler.test.ts | 11 +++ .../init/final/FinalizableHandler.test.ts | 11 +++ .../unit/init/final/ParallelFinalizer.test.ts | 30 ------- .../locking/FileSystemResourceLocker.test.ts | 17 ++-- test/unit/util/locking/RedisLocker.test.ts | 57 +++++++------ test/util/Util.ts | 7 +- 32 files changed, 415 insertions(+), 144 deletions(-) create mode 100644 src/init/Initializable.ts create mode 100644 src/init/InitializableHandler.ts create mode 100644 src/init/final/FinalizableHandler.ts create mode 100644 src/init/final/Finalizer.ts delete mode 100644 src/init/final/ParallelFinalizer.ts create mode 100644 test/integration/ResourceLockCleanup.test.ts create mode 100644 test/unit/init/InitializableHandler.test.ts create mode 100644 test/unit/init/final/FinalizableHandler.test.ts delete mode 100644 test/unit/init/final/ParallelFinalizer.test.ts diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index e1f5b44b8..a3bbb02fb 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -39,7 +39,17 @@ The following changes are relevant for v4 custom configs that replaced certain f - The `IdentityProviderFactory` inputs have been extended. - `/identity/handler/provider-factory/identity.json` - LDP components have slightly changed so the preference parser is in a separate config file. - - `/config/ldp/handler/*` + - `/ldp/handler/*` +- Restructured the init configs. + - `/app/init/base/init.json` + - `/app/main/default.json` +- Added lock cleanup on server start (and updated existing finalization). + - `/util/resource-locker/file.json` + - `/util/resource-locker/redis.json` +- Updated finalizers. + - `/app/identity/handler/account-store/default.json` + - `/identity/ownership/token.json` + - `/ldp/authorization/readers/access-checkers/agent-group.json` ### Interface changes These changes are relevant if you wrote custom modules for the server that depend on existing interfaces. @@ -49,6 +59,12 @@ These changes are relevant if you wrote custom modules for the server that depen - Both `TemplateEngine` implementations now take a `baseUrl` parameter as input. - The `IdentityProviderFactory` and `ConvertingErrorHandler` now additionally take a `PreferenceParser` as input. - Error handlers now take the incoming HttpRequest as input instead of just the preferences. +- Extended the initialization/finalization system: + * Introduced `Initializable` interface and `InitializableHandler` wrapper class. + * Introduced `Finalizer` abstract class and `FinalizableHandler` wrapper class. + * Changed type for `finalizer` attribute in `App` from `Finalizable` to `Finalizer` and updated the calling code in `App.stop()`. + * Removed the now obsolete `ParallelFinalizer` util class. +- Added a lock cleanup on initialize for lock implementations `RedisLocker` and `FileSystemResourceLocker`. A new interface `SingleThreaded` has been added. This empty interface can be implemented to mark a component as not-threadsafe. When the CSS starts in multithreaded mode, it will error and halt if any SingleThreaded components are instantiated. diff --git a/config/app/init/base/init.json b/config/app/init/base/init.json index a0d04f30d..a5698a7d0 100644 --- a/config/app/init/base/init.json +++ b/config/app/init/base/init.json @@ -30,6 +30,7 @@ "@id": "urn:solid-server:default:PrimarySequenceInitializer", "@type":"SequenceHandler", "handlers": [ + { "@id": "urn:solid-server:default:CleanupInitializer"}, { "@id": "urn:solid-server:default:BaseUrlVerifier" }, { "@id": "urn:solid-server:default:PrimaryParallelInitializer" }, { "@id": "urn:solid-server:default:SeededPodInitializer" }, @@ -53,6 +54,13 @@ { "@id": "urn:solid-server:default:ServerInitializer" } ] } + }, + { + "comment": "Initializers that need to cleanup or do anything else before something writes to the backend should be added here.", + "@id": "urn:solid-server:default:CleanupInitializer", + "@type":"SequenceHandler", + "handlers": [ + ] } ] } diff --git a/config/app/main/default.json b/config/app/main/default.json index ac846c05b..194735fe4 100644 --- a/config/app/main/default.json +++ b/config/app/main/default.json @@ -7,18 +7,36 @@ "@type": "App", "initializer": { "@id": "urn:solid-server:default:Initializer" }, "finalizer": { - "comment": "This is going to contain the list of finalizers that need to be called. These should be added in the configs where such classes are configured.", - "@id": "urn:solid-server:default:Finalizer", - "@type": "ParallelFinalizer", - "finalizers": [ - { "@id": "urn:solid-server:default:ServerInitializer" } + "comment": "Is executed when the server is stopped.", + "@type": "SequenceHandler", + "handlers": [ + { "@id": "urn:solid-server:default:Finalizer" }, + { "@id": "urn:solid-server:default:CleanupFinalizer" } ] }, - "clusterManager": { + "clusterManager": { "@id": "urn:solid-server:default:ClusterManager", "@type": "ClusterManager", "workers": { "@id": "urn:solid-server:default:variable:workers" } } + }, + { + "comment": "This is going to contain the list of finalizers that need to be called. These should be added in the configs where such classes are configured.", + "@id": "urn:solid-server:default:Finalizer", + "@type": "ParallelHandler", + "handlers": [ + { + "@type": "FinalizableHandler", + "finalizable": { "@id": "urn:solid-server:default:ServerInitializer" } + } + ] + }, + { + "comment": "Finalizers that need to cleanup once no more data will be written to the backend should be added here.", + "@id": "urn:solid-server:default:CleanupFinalizer", + "@type":"SequenceHandler", + "handlers": [ + ] } ] } diff --git a/config/identity/handler/account-store/default.json b/config/identity/handler/account-store/default.json index 9d1a78193..c547e117e 100644 --- a/config/identity/handler/account-store/default.json +++ b/config/identity/handler/account-store/default.json @@ -28,8 +28,13 @@ { "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.", "@id": "urn:solid-server:default:Finalizer", - "@type": "ParallelFinalizer", - "finalizers": [ { "@id": "urn:solid-server:default:ExpiringForgotPasswordStorage" } ] + "@type": "ParallelHandler", + "handlers": [ + { + "@type": "FinalizableHandler", + "finalizable": { "@id": "urn:solid-server:default:ExpiringForgotPasswordStorage" } + } + ] } ] } diff --git a/config/identity/ownership/token.json b/config/identity/ownership/token.json index f8abe8a36..551d33467 100644 --- a/config/identity/ownership/token.json +++ b/config/identity/ownership/token.json @@ -21,8 +21,13 @@ { "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.", "@id": "urn:solid-server:default:Finalizer", - "@type": "ParallelFinalizer", - "finalizers": [ { "@id": "urn:solid-server:default:ExpiringTokenStorage" } ] + "@type": "ParallelHandler", + "handlers": [ + { + "@type": "FinalizableHandler", + "finalizable": { "@id": "urn:solid-server:default:ExpiringTokenStorage" } + } + ] } ] } diff --git a/config/ldp/authorization/readers/access-checkers/agent-group.json b/config/ldp/authorization/readers/access-checkers/agent-group.json index 911142344..994bd8e92 100644 --- a/config/ldp/authorization/readers/access-checkers/agent-group.json +++ b/config/ldp/authorization/readers/access-checkers/agent-group.json @@ -14,8 +14,13 @@ { "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.", "@id": "urn:solid-server:default:Finalizer", - "@type": "ParallelFinalizer", - "finalizers": [ { "@id": "urn:solid-server:default:ExpiringAclCache" } ] + "@type": "ParallelHandler", + "handlers": [ + { + "@type": "FinalizableHandler", + "finalizable": { "@id": "urn:solid-server:default:ExpiringAclCache" } + } + ] } ] } diff --git a/config/util/resource-locker/file.json b/config/util/resource-locker/file.json index e4e68865e..fcbeb3a62 100644 --- a/config/util/resource-locker/file.json +++ b/config/util/resource-locker/file.json @@ -22,11 +22,25 @@ "expiration": 3000 }, { - "comment": "Makes sure the lock folder is cleared and delete when the application needs to stop.", - "@id": "urn:solid-server:default:Finalizer", - "@type": "ParallelFinalizer", - "finalizers": [ - { "@id": "urn:solid-server:default:FileSystemResourceLocker" } + "@id": "urn:solid-server:default:CleanupInitializer", + "@type": "SequenceHandler", + "handlers": [ + { + "comment": "Makes sure the FileSystemResourceLocker starts with a clean slate when the application is started.", + "@type": "InitializableHandler", + "initializable": { "@id": "urn:solid-server:default:FileSystemResourceLocker" } + } + ] + }, + { + "@id": "urn:solid-server:default:CleanupFinalizer", + "@type": "SequenceHandler", + "handlers": [ + { + "comment": "Makes sure the lock folder is removed when the application stops.", + "@type": "FinalizableHandler", + "finalizable": { "@id": "urn:solid-server:default:FileSystemResourceLocker" } + } ] } ] diff --git a/config/util/resource-locker/redis.json b/config/util/resource-locker/redis.json index f45e4c012..e6ff6def9 100644 --- a/config/util/resource-locker/redis.json +++ b/config/util/resource-locker/redis.json @@ -12,12 +12,24 @@ "expiration": 3000 }, { - "comment": "Makes sure the redis connection is closed when the application needs to stop. Also deletes still-existing locks and counters.", - "@id": "urn:solid-server:default:Finalizer", - "@type": "ParallelFinalizer", - "finalizers": [ + "@id": "urn:solid-server:default:CleanupInitializer", + "@type": "SequenceHandler", + "handlers": [ { - "@id": "urn:solid-server:default:RedisLocker" + "comment": "Makes sure the RedisLocker starts with a clean slate when the application is started.", + "@type": "InitializableHandler", + "initializable": { "@id": "urn:solid-server:default:RedisLocker" } + } + ] + }, + { + "@id": "urn:solid-server:default:CleanupFinalizer", + "@type": "SequenceHandler", + "handlers": [ + { + "comment": "Makes sure the redis connection is closed when the application needs to stop. Also deletes still-existing locks and counters.", + "@type": "FinalizableHandler", + "finalizable": { "@id": "urn:solid-server:default:RedisLocker" } } ] } diff --git a/package.json b/package.json index 71eb54e4b..e10be8fc7 100644 --- a/package.json +++ b/package.json @@ -57,6 +57,7 @@ "release": "standard-version", "start": "node ./bin/server.js", "start:file": "node ./bin/server.js -c config/file.json -f ./data", + "start:file-no-setup": "node ./bin/server.js -c config/file-no-setup.json -f ./data", "test": "npm run test:ts && npm run jest", "test:deploy": "test/deploy/validate-configs.sh", "test:ts": "tsc -p test --noEmit", diff --git a/src/index.ts b/src/index.ts index 55f26fa3c..aec9ff246 100644 --- a/src/index.ts +++ b/src/index.ts @@ -194,7 +194,8 @@ export * from './init/cluster/WorkerManager'; // Init/Final export * from './init/final/Finalizable'; -export * from './init/final/ParallelFinalizer'; +export * from './init/final/FinalizableHandler'; +export * from './init/final/Finalizer'; // Init/Setup export * from './init/setup/SetupHandler'; @@ -221,6 +222,8 @@ export * from './init/BaseUrlVerifier'; export * from './init/CliResolver'; export * from './init/ConfigPodInitializer'; export * from './init/ContainerInitializer'; +export * from './init/Initializable'; +export * from './init/InitializableHandler'; export * from './init/Initializer'; export * from './init/LoggerInitializer'; export * from './init/ModuleVersionVerifier'; diff --git a/src/init/App.ts b/src/init/App.ts index 3929a00fb..0ee31035a 100644 --- a/src/init/App.ts +++ b/src/init/App.ts @@ -1,5 +1,5 @@ import type { ClusterManager } from './cluster/ClusterManager'; -import type { Finalizable } from './final/Finalizable'; +import type { Finalizer } from './final/Finalizer'; import type { Initializer } from './Initializer'; /** @@ -7,10 +7,10 @@ import type { Initializer } from './Initializer'; */ export class App { private readonly initializer: Initializer; - private readonly finalizer: Finalizable; + private readonly finalizer: Finalizer; public readonly clusterManager: ClusterManager; - public constructor(initializer: Initializer, finalizer: Finalizable, clusterManager: ClusterManager) { + public constructor(initializer: Initializer, finalizer: Finalizer, clusterManager: ClusterManager) { this.initializer = initializer; this.finalizer = finalizer; this.clusterManager = clusterManager; @@ -27,6 +27,6 @@ export class App { * Stops the application and handles cleanup. */ public async stop(): Promise { - await this.finalizer.finalize(); + await this.finalizer.handleSafe(); } } diff --git a/src/init/Initializable.ts b/src/init/Initializable.ts new file mode 100644 index 000000000..649dbdac0 --- /dev/null +++ b/src/init/Initializable.ts @@ -0,0 +1,8 @@ +/** + * Allows for initializing state or executing logic when the application is started. + * Use this interface to add initialization logic to classes that already extend some other type. + * NOTE: classes without an existing extends-relation should extend from Initializer instead! + */ +export interface Initializable { + initialize: () => Promise; +} diff --git a/src/init/InitializableHandler.ts b/src/init/InitializableHandler.ts new file mode 100644 index 000000000..af3913f92 --- /dev/null +++ b/src/init/InitializableHandler.ts @@ -0,0 +1,18 @@ +import type { Initializable } from './Initializable'; +import { Initializer } from './Initializer'; + +/** + * Allows using an Initializable as an Initializer Handler. + */ +export class InitializableHandler extends Initializer { + protected readonly initializable: Initializable; + + public constructor(initializable: Initializable) { + super(); + this.initializable = initializable; + } + + public async handle(): Promise { + return this.initializable.initialize(); + } +} diff --git a/src/init/Initializer.ts b/src/init/Initializer.ts index 502475eba..cbdd9d999 100644 --- a/src/init/Initializer.ts +++ b/src/init/Initializer.ts @@ -1,3 +1,6 @@ import { AsyncHandler } from '../util/handlers/AsyncHandler'; +/** + * Initializer is used to indicate an AsyncHandler that performs initialization logic. + */ export abstract class Initializer extends AsyncHandler {} diff --git a/src/init/final/Finalizable.ts b/src/init/final/Finalizable.ts index a99f75bf1..bace55f2b 100644 --- a/src/init/final/Finalizable.ts +++ b/src/init/final/Finalizable.ts @@ -1,5 +1,7 @@ /** * Allows for cleaning up an object and stopping relevant loops when the application needs to be stopped. + * Use this interface to add finalization logic to classes that already extend some other type. + * NOTE: classes without an existing extends-relation should extend from Finalizer instead! */ export interface Finalizable { finalize: () => Promise; diff --git a/src/init/final/FinalizableHandler.ts b/src/init/final/FinalizableHandler.ts new file mode 100644 index 000000000..4a1d7d825 --- /dev/null +++ b/src/init/final/FinalizableHandler.ts @@ -0,0 +1,18 @@ +import type { Finalizable } from './Finalizable'; +import { Finalizer } from './Finalizer'; + +/** + * Allows using a Finalizable as a Finalizer Handler. + */ +export class FinalizableHandler extends Finalizer { + protected readonly finalizable: Finalizable; + + public constructor(finalizable: Finalizable) { + super(); + this.finalizable = finalizable; + } + + public async handle(): Promise { + return this.finalizable.finalize(); + } +} diff --git a/src/init/final/Finalizer.ts b/src/init/final/Finalizer.ts new file mode 100644 index 000000000..97208895b --- /dev/null +++ b/src/init/final/Finalizer.ts @@ -0,0 +1,6 @@ +import { AsyncHandler } from '../../util/handlers/AsyncHandler'; + +/** + * Finalizer is used to indicate an AsyncHandler that performs finalization logic. + */ +export abstract class Finalizer extends AsyncHandler {} diff --git a/src/init/final/ParallelFinalizer.ts b/src/init/final/ParallelFinalizer.ts deleted file mode 100644 index ed97d933e..000000000 --- a/src/init/final/ParallelFinalizer.ts +++ /dev/null @@ -1,16 +0,0 @@ -import type { Finalizable } from './Finalizable'; - -/** - * Finalizes all the injected Finalizable classes in parallel. - */ -export class ParallelFinalizer implements Finalizable { - private readonly finalizers: Finalizable[]; - - public constructor(finalizers: Finalizable[] = []) { - this.finalizers = finalizers; - } - - public async finalize(): Promise { - await Promise.all(this.finalizers.map(async(finalizer): Promise => finalizer.finalize())); - } -} diff --git a/src/util/locking/FileSystemResourceLocker.ts b/src/util/locking/FileSystemResourceLocker.ts index 6523e0ae8..1a9dea343 100644 --- a/src/util/locking/FileSystemResourceLocker.ts +++ b/src/util/locking/FileSystemResourceLocker.ts @@ -1,9 +1,10 @@ import { createHash } from 'crypto'; -import { ensureDirSync, pathExists, readdir, rmdir } from 'fs-extra'; +import { ensureDir, remove } from 'fs-extra'; import type { LockOptions, UnlockOptions } from 'proper-lockfile'; import { lock, unlock } from 'proper-lockfile'; import type { ResourceIdentifier } from '../../http/representation/ResourceIdentifier'; import type { Finalizable } from '../../init/final/Finalizable'; +import type { Initializable } from '../../init/Initializable'; import { getLoggerFor } from '../../logging/LogUtil'; import { createErrorMessage } from '../errors/ErrorUtil'; import { InternalServerError } from '../errors/InternalServerError'; @@ -34,9 +35,12 @@ const attemptDefaults: Required = { retryCount: -1, retryDelay: * Argument interface of the FileSystemResourceLocker constructor. */ interface FileSystemResourceLockerArgs { - /** The rootPath of the filesystem */ + /** The rootPath of the filesystem _[default is the current dir `./`]_ */ rootFilePath?: string; - /** The path to the directory where locks will be stored (appended to rootFilePath) */ + /** + * The path to the directory where locks will be stored (appended to rootFilePath) + * _[default is `/.internal/locks`]_ + */ lockDirectory?: string; /** Custom settings concerning retrying locks */ attemptSettings?: AttemptSettings; @@ -54,24 +58,22 @@ function isCodedError(err: unknown): err is { code: string } & Error { * either resolve successfully or reject immediately with the causing error. The retry function of the library * however will be ignored and replaced by our own LockUtils' {@link retryFunctionUntil} function. */ -export class FileSystemResourceLocker implements ResourceLocker, Finalizable { +export class FileSystemResourceLocker implements ResourceLocker, Initializable, Finalizable { protected readonly logger = getLoggerFor(this); private readonly attemptSettings: Required; /** Folder that stores the locks */ private readonly lockFolder: string; + private finalized = false; /** * Create a new FileSystemResourceLocker - * @param rootFilePath - The rootPath of the filesystem _[default is the current dir `./`]_ - * @param lockDirectory - The path to the directory where locks will be stored (appended to rootFilePath) - _[default is `/.internal/locks`]_ - * @param attemptSettings - Custom settings concerning retrying locks + * @param args - Configures the locker using the specified FileSystemResourceLockerArgs instance. */ public constructor(args: FileSystemResourceLockerArgs = {}) { const { rootFilePath, lockDirectory, attemptSettings } = args; + defaultLockOptions.onCompromised = this.customOnCompromised.bind(this); this.attemptSettings = { ...attemptDefaults, ...attemptSettings }; this.lockFolder = joinFilePath(rootFilePath ?? './', lockDirectory ?? '/.internal/locks'); - ensureDirSync(this.lockFolder); } /** @@ -149,13 +151,36 @@ export class FileSystemResourceLocker implements ResourceLocker, Finalizable { }; } + /** + * Initializer method to be executed on server start. This makes sure that no pre-existing (dangling) locks + * remain on disk, so that request will not be blocked because a lock was acquired in the previous server instance. + * + * NOTE: this also removes locks created by the GreedyReadWriteLocker. + * (See issue: https://github.com/CommunitySolidServer/CommunitySolidServer/issues/1358) + */ + public async initialize(): Promise { + // Remove all existing (dangling) locks so new requests are not blocked (by removing the lock folder). + await remove(this.lockFolder); + // Put the folder back since `proper-lockfile` depends on its existence. + return ensureDir(this.lockFolder); + } + public async finalize(): Promise { - // Delete lingering locks in the lockFolder. - if (await pathExists(this.lockFolder)) { - for (const dir of await readdir(this.lockFolder)) { - await rmdir(joinFilePath(this.lockFolder, dir)); - } - await rmdir(this.lockFolder); + // Register that finalize was called by setting a state variable. + this.finalized = true; + // NOTE: in contrast with initialize(), the lock folder is not cleared here, as the proper-lock library + // manages these files and will attempt to clear existing files when the process is shutdown gracefully. + } + + /** + * This function is used to override the proper-lock onCompromised function. + * Once the locker was finalized, it will log the provided error instead of throwing it + * This allows for a clean shutdown procedure. + */ + private customOnCompromised(err: any): void { + if (!this.finalized) { + throw err; } + this.logger.warn(`onCompromised was called with error: ${err.message}`); } } diff --git a/src/util/locking/RedisLocker.ts b/src/util/locking/RedisLocker.ts index f5d5507a2..d2bff1bfc 100644 --- a/src/util/locking/RedisLocker.ts +++ b/src/util/locking/RedisLocker.ts @@ -1,6 +1,7 @@ import Redis from 'ioredis'; import type { ResourceIdentifier } from '../../http/representation/ResourceIdentifier'; import type { Finalizable } from '../../init/final/Finalizable'; +import type { Initializable } from '../../init/Initializable'; import { getLoggerFor } from '../../logging/LogUtil'; import type { AttemptSettings } from '../LockUtils'; import { retryFunction } from '../LockUtils'; @@ -42,13 +43,14 @@ const PREFIX_LOCK = '__L__'; * * @see [Redis Lua scripting documentation](https://redis.io/docs/manual/programmability/) * * @see [ioredis Lua scripting API](https://github.com/luin/ioredis#lua-scripting) */ -export class RedisLocker implements ReadWriteLocker, ResourceLocker, Finalizable { +export class RedisLocker implements ReadWriteLocker, ResourceLocker, Initializable, Finalizable { protected readonly logger = getLoggerFor(this); private readonly redis: Redis; private readonly redisRw: RedisReadWriteLock; private readonly redisLock: RedisResourceLock; private readonly attemptSettings: Required; + private finalized = false; public constructor(redisClient = '127.0.0.1:6379', attemptSettings: AttemptSettings = {}) { this.redis = this.createRedisClient(redisClient); @@ -113,6 +115,9 @@ export class RedisLocker implements ReadWriteLocker, ResourceLocker, Finalizable * @param fn - The function reference to swallow false from. */ private swallowFalse(fn: () => Promise): () => Promise { + if (this.finalized) { + throw new Error('Invalid state: cannot execute Redis operation once finalize() has been called.'); + } return async(): Promise => { const result = await fromResp2ToBool(fn()); // Swallow any result resolving to `false` @@ -172,24 +177,36 @@ export class RedisLocker implements ReadWriteLocker, ResourceLocker, Finalizable ); } - /* Finalizer methods */ + /* Initializer & Finalizer methods */ + + public async initialize(): Promise { + // On server start: remove all existing (dangling) locks, so new requests are not blocked. + return this.clearLocks(); + } public async finalize(): Promise { - // This for loop is an extra failsafe, - // this extra code won't slow down anything, this function will only be called to shut down in peace + this.finalized = true; try { - // Remove any lock still open, since once closed, they should no longer be held. - const keysRw = await this.redisRw.keys(`${PREFIX_RW}*`); - if (keysRw.length > 0) { - await this.redisRw.del(...keysRw); - } - - const keysLock = await this.redisLock.keys(`${PREFIX_LOCK}*`); - if (keysLock.length > 0) { - await this.redisLock.del(...keysLock); - } + // On controlled server shutdown: clean up all existing locks. + return await this.clearLocks(); } finally { + // Always quit the redis client await this.redis.quit(); } } + + /** + * Remove any lock still open + */ + private async clearLocks(): Promise { + const keysRw = await this.redisRw.keys(`${PREFIX_RW}*`); + if (keysRw.length > 0) { + await this.redisRw.del(...keysRw); + } + + const keysLock = await this.redisLock.keys(`${PREFIX_LOCK}*`); + if (keysLock.length > 0) { + await this.redisLock.del(...keysLock); + } + } } diff --git a/test/integration/RedisLocker.test.ts b/test/integration/RedisLocker.test.ts index d8d59ba4c..21b8bd2b4 100644 --- a/test/integration/RedisLocker.test.ts +++ b/test/integration/RedisLocker.test.ts @@ -10,7 +10,7 @@ import { getDefaultVariables, getTestConfigPath, instantiateFromConfig } from '. /** * Test the general functionality of the server using a RedisLocker with Read-Write strategy. */ -describeIf('docker', 'A server with a RedisLocker', (): void => { +describeIf('docker')('A server with a RedisLocker', (): void => { const port = getPort('RedisLocker'); const baseUrl = `http://localhost:${port}/`; let app: App; diff --git a/test/integration/ResourceLockCleanup.test.ts b/test/integration/ResourceLockCleanup.test.ts new file mode 100644 index 000000000..b6ba484f8 --- /dev/null +++ b/test/integration/ResourceLockCleanup.test.ts @@ -0,0 +1,84 @@ +import fetch from 'cross-fetch'; +import type { App, DataAccessorBasedStore, Initializable, ResourceLocker } from '../../src'; +import { readableToString, BasicRepresentation } from '../../src'; +import { describeIf, getPort } from '../util/Util'; +import { getDefaultVariables, getTestConfigPath, getTestFolder, instantiateFromConfig, removeFolder } from './Config'; + +const port = getPort('ResourceLockCleanup'); +const baseUrl = `http://localhost:${port}/`; +const rootFilePath = getTestFolder(`resource-lock-cleanup`); +const resourceIdentifier = { path: `${baseUrl}container1/test.txt` }; + +const configs: [string, any][] = [ + [ + 'file-based', { + config: 'server-file.json', + init: async(initializable: Initializable): Promise => initializable.initialize(), + teardown: async(): Promise => removeFolder(rootFilePath), + }], + [ + 'redis-based', { + config: 'server-redis-lock.json', + init: jest.fn(), + teardown: jest.fn(), + }], +]; + +describeIf('docker').each(configs)('A server using %s locking', (id, { config, init, teardown }): +void => { + let app: App; + let store: DataAccessorBasedStore; + let locker: ResourceLocker; + + beforeAll(async(): Promise => { + const variables = { + ...getDefaultVariables(port, baseUrl), + 'urn:solid-server:default:variable:rootFilePath': rootFilePath, + }; + + // Create the server + const instances = await instantiateFromConfig( + 'urn:solid-server:test:Instances', + [ + getTestConfigPath(config), + ], + variables, + ) as Record; + ({ app, store, locker } = instances); + // Create the test resource + await store.setRepresentation(resourceIdentifier, new BasicRepresentation('abc', 'text/plain')); + + // Perform additional initialization, if configured + await init(locker); + }); + + afterAll(async(): Promise => { + // Stop the server + await app.stop(); + + // Execute the configured teardown + await teardown(); + }); + + it('should not be affected by dangling locks.', async(): Promise => { + // Simulate lock existing before server startup, by creating a (write) lock directly + await locker.acquire({ path: `${resourceIdentifier.path}.write` }); + // Start the server + await app.start(); + + // Updating the resource should succeed (if the server clears dangling locks on startup). + const updatedContent = 'def'; + const result = await fetch(resourceIdentifier.path, { + method: 'PUT', + headers: { + 'content-type': 'text/plain', + }, + body: updatedContent, + }); + expect(result.status).toBe(205); + // Check if the resource was updated: + const representation = await store.getRepresentation(resourceIdentifier); + const data = await readableToString(representation.data); + expect(data).toEqual(updatedContent); + }); +}); diff --git a/test/integration/SparqlStorage.test.ts b/test/integration/SparqlStorage.test.ts index b3dc87f90..46d7ed485 100644 --- a/test/integration/SparqlStorage.test.ts +++ b/test/integration/SparqlStorage.test.ts @@ -8,7 +8,7 @@ import { getDefaultVariables, getPresetConfigPath, getTestConfigPath, instantiat const port = getPort('SparqlStorage'); const baseUrl = `http://localhost:${port}/`; -describeIf('docker', 'A server with a SPARQL endpoint as storage', (): void => { +describeIf('docker')('A server with a SPARQL endpoint as storage', (): void => { let app: App; beforeAll(async(): Promise => { diff --git a/test/integration/config/server-file.json b/test/integration/config/server-file.json index 90eb3c60f..48ac02443 100644 --- a/test/integration/config/server-file.json +++ b/test/integration/config/server-file.json @@ -27,17 +27,25 @@ "css:config/util/index/default.json", "css:config/util/logging/winston.json", "css:config/util/representation-conversion/default.json", - "css:config/util/resource-locker/memory.json", + "css:config/util/resource-locker/file.json", "css:config/util/variables/default.json" ], "@graph": [ { "@id": "urn:solid-server:test:Instances", "@type": "RecordObject", - "RecordObject:_record": [ + "record": [ { "RecordObject:_record_key": "app", "RecordObject:_record_value": { "@id": "urn:solid-server:default:App" } + }, + { + "RecordObject:_record_key": "store", + "RecordObject:_record_value": { "@id": "urn:solid-server:default:ResourceStore_Backend" } + }, + { + "RecordObject:_record_key": "locker", + "RecordObject:_record_value": { "@id": "urn:solid-server:default:FileSystemResourceLocker" } } ] }, diff --git a/test/integration/config/server-redis-lock.json b/test/integration/config/server-redis-lock.json index 2e433e1f7..898ba631e 100644 --- a/test/integration/config/server-redis-lock.json +++ b/test/integration/config/server-redis-lock.json @@ -38,6 +38,10 @@ "RecordObject:_record_key": "app", "RecordObject:_record_value": { "@id": "urn:solid-server:default:App" } }, + { + "RecordObject:_record_key": "store", + "RecordObject:_record_value": { "@id": "urn:solid-server:default:ResourceStore_Backend" } + }, { "RecordObject:_record_key": "locker", "RecordObject:_record_value": { "@id": "urn:solid-server:default:RedisLocker" } diff --git a/test/unit/init/App.test.ts b/test/unit/init/App.test.ts index b251b57d8..107b572ea 100644 --- a/test/unit/init/App.test.ts +++ b/test/unit/init/App.test.ts @@ -1,17 +1,17 @@ import type { ClusterManager } from '../../../src'; import { App } from '../../../src/init/App'; -import type { Finalizable } from '../../../src/init/final/Finalizable'; +import type { Finalizer } from '../../../src/init/final/Finalizer'; import type { Initializer } from '../../../src/init/Initializer'; describe('An App', (): void => { let initializer: Initializer; - let finalizer: Finalizable; + let finalizer: Finalizer; let clusterManager: ClusterManager; let app: App; beforeEach(async(): Promise => { initializer = { handleSafe: jest.fn() } as any; - finalizer = { finalize: jest.fn() }; + finalizer = { handleSafe: jest.fn() } as any; clusterManager = {} as any; app = new App(initializer, finalizer, clusterManager); }); @@ -23,7 +23,7 @@ describe('An App', (): void => { it('can stop with the finalizer.', async(): Promise => { await expect(app.stop()).resolves.toBeUndefined(); - expect(finalizer.finalize).toHaveBeenCalledTimes(1); + expect(finalizer.handleSafe).toHaveBeenCalledTimes(1); }); it('can check its clusterManager for the threading mode.', async(): Promise => { diff --git a/test/unit/init/InitializableHandler.test.ts b/test/unit/init/InitializableHandler.test.ts new file mode 100644 index 000000000..1075f254b --- /dev/null +++ b/test/unit/init/InitializableHandler.test.ts @@ -0,0 +1,11 @@ +import { InitializableHandler } from '../../../src'; + +describe('InitializableHandler', (): void => { + const initializable = { initialize: jest.fn() }; + const initializer = new InitializableHandler(initializable); + + it('redirects handle towards initialize.', async(): Promise => { + await initializer.handleSafe(); + expect(initializable.initialize).toHaveBeenCalledTimes(1); + }); +}); diff --git a/test/unit/init/final/FinalizableHandler.test.ts b/test/unit/init/final/FinalizableHandler.test.ts new file mode 100644 index 000000000..a0b610e3f --- /dev/null +++ b/test/unit/init/final/FinalizableHandler.test.ts @@ -0,0 +1,11 @@ +import { FinalizableHandler } from '../../../../src'; + +describe('FinalizableHandler', (): void => { + const finalizable = { finalize: jest.fn() }; + const finalizer = new FinalizableHandler(finalizable); + + it('redirects handle towards finalize.', async(): Promise => { + await finalizer.handleSafe(); + expect(finalizable.finalize).toHaveBeenCalledTimes(1); + }); +}); diff --git a/test/unit/init/final/ParallelFinalizer.test.ts b/test/unit/init/final/ParallelFinalizer.test.ts deleted file mode 100644 index 71dc8201f..000000000 --- a/test/unit/init/final/ParallelFinalizer.test.ts +++ /dev/null @@ -1,30 +0,0 @@ -import type { Finalizable } from '../../../../src/init/final/Finalizable'; -import { ParallelFinalizer } from '../../../../src/init/final/ParallelFinalizer'; - -describe('A ParallelFinalizer', (): void => { - let finalizers: Finalizable[]; - let finalizer: ParallelFinalizer; - let results: number[]; - - beforeEach(async(): Promise => { - results = []; - finalizers = [ - { finalize: jest.fn((): any => results.push(0)) }, - { finalize: jest.fn((): any => results.push(1)) }, - ]; - - finalizer = new ParallelFinalizer(finalizers); - }); - - it('is finished when all finalizers are finished.', async(): Promise => { - await expect(finalizer.finalize()).resolves.toBeUndefined(); - expect(finalizers[0].finalize).toHaveBeenCalledTimes(1); - expect(finalizers[1].finalize).toHaveBeenCalledTimes(1); - expect(results).toEqual([ 0, 1 ]); - }); - - it('works if there are no input finalizers.', async(): Promise => { - finalizer = new ParallelFinalizer(); - await expect(finalizer.finalize()).resolves.toBeUndefined(); - }); -}); diff --git a/test/unit/util/locking/FileSystemResourceLocker.test.ts b/test/unit/util/locking/FileSystemResourceLocker.test.ts index 14eeda6f5..ff74d8129 100644 --- a/test/unit/util/locking/FileSystemResourceLocker.test.ts +++ b/test/unit/util/locking/FileSystemResourceLocker.test.ts @@ -1,6 +1,5 @@ import { readdir } from 'fs-extra'; -import { InternalServerError } from '../../../../src/util/errors/InternalServerError'; -import { FileSystemResourceLocker } from '../../../../src/util/locking/FileSystemResourceLocker'; +import { InternalServerError, FileSystemResourceLocker } from '../../../../src'; const lockFolder = './.internal/locks/'; @@ -10,6 +9,7 @@ describe('A FileSystemResourceLocker', (): void => { beforeEach(async(): Promise => { locker = new FileSystemResourceLocker({ attemptSettings: { retryCount: 19, retryDelay: 100 }}); + await locker.initialize(); }); afterEach(async(): Promise => { @@ -109,13 +109,20 @@ describe('A FileSystemResourceLocker', (): void => { await expect(locker.acquire(identifier)).rejects.toThrow(InternalServerError); }); - it('clears the files in de lock directory after calling finalize.', async(): Promise => { + it('clears the files in de lock directory upon calling initialize.', async(): Promise => { await locker.acquire(identifier); await expect(readdir(lockFolder)).resolves.toHaveLength(1); - await locker.finalize(); - await expect(readdir(lockFolder)).rejects.toThrow(); + await locker.initialize(); + await expect(readdir(lockFolder)).resolves.toHaveLength(0); }); + it('stops proper-lock from throwing errors onCompromise after finalize was called.', + async(): Promise => { + expect((): void => (locker as any).customOnCompromised(new Error('test'))).toThrow(); + await locker.finalize(); + expect((locker as any).customOnCompromised(new Error('test'))).toBeUndefined(); + }); + it('can create a locker with default AttemptSettings.', async(): Promise => { expect((): FileSystemResourceLocker => new FileSystemResourceLocker()).not.toThrow(); }); diff --git a/test/unit/util/locking/RedisLocker.test.ts b/test/unit/util/locking/RedisLocker.test.ts index 8ad73d8fd..7df0f6a6d 100644 --- a/test/unit/util/locking/RedisLocker.test.ts +++ b/test/unit/util/locking/RedisLocker.test.ts @@ -70,8 +70,20 @@ const store = { const redis: jest.Mocked = { defineCommand: jest.fn(), quit: jest.fn(), - keys: jest.fn().mockResolvedValue([]), - del: jest.fn(), + keys: jest.fn().mockImplementation(async(pattern: string): Promise => + Object.keys(store.internal) + .filter((value: string): boolean => new RegExp(pattern, 'u').test(value))), + del: jest.fn().mockImplementation(async(...keys: string[]): Promise => { + let deletedEntries = 0; + for (const key of keys) { + if (typeof store.internal[key] !== 'undefined') { + deletedEntries += 1; + } + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete store.internal[key]; + } + return deletedEntries; + }), acquireReadLock: jest.fn().mockImplementation(async(key: string): Promise => store.acquireReadLock(key)), acquireWriteLock: jest.fn().mockImplementation(async(key: string): Promise => @@ -407,29 +419,14 @@ describe('A RedisLocker', (): void => { }); describe('finalize()', (): void => { - it('should quit when there are no more keys when finalize() is called.', async(): Promise => { - // This works since the Redis is simply a mock and quit should have cleared the internal store - await locker.withWriteLock(resource1, async(): Promise => { - await locker.finalize(); - expect(redis.quit).toHaveBeenCalledTimes(1); - }); - }); - it('should clear all lock keys when finalize() is called.', async(): Promise => { - redis.keys.mockResolvedValueOnce([ '__L__k1', '__L__k2' ]); - // This works since the Redis is simply a mock and quit should have cleared the internal store - await locker.withWriteLock(resource1, async(): Promise => { - await locker.finalize(); - expect(redis.quit).toHaveBeenCalledTimes(1); - }); - }); - - it('should clear all rw keys when finalize() is called.', async(): Promise => { - redis.keys.mockResolvedValueOnce([ '__RW__k1', '__RW__k2' ]); - // This works since the Redis is simply a mock and quit should have cleared the internal store - await locker.withWriteLock(resource1, async(): Promise => { + it('should call quit and clear Read-Write locks when finalize() is called.', async(): Promise => { + const promise = locker.withWriteLock(resource1, async(): Promise => { await locker.finalize(); + expect(Object.keys(store.internal)).toHaveLength(0); expect(redis.quit).toHaveBeenCalledTimes(1); }); + // Auto-release of Read-Write lock should result in an exception, as the Locker has been finalized. + await expect(promise).rejects.toThrow(/Invalid state/u); }); }); }); @@ -521,17 +518,27 @@ describe('A RedisLocker', (): void => { }); }); + describe('initialize()', (): void => { + it('should clear all locks when initialize() is called.', async(): Promise => { + await locker.acquire({ path: 'path1' }); + await locker.acquire({ path: 'path2' }); + await locker.initialize(); + expect(Object.keys(store.internal)).toHaveLength(0); + }); + }); + describe('finalize()', (): void => { it('should clear all locks (even when empty) when finalize() is called.', async(): Promise => { await locker.finalize(); + expect(Object.keys(store.internal)).toHaveLength(0); expect(redis.quit).toHaveBeenCalledTimes(1); }); it('should clear all locks when finalize() is called.', async(): Promise => { - redis.keys - .mockResolvedValueOnce([ '__L__k1', '__L__k2' ]) - .mockResolvedValueOnce([ '__L__k1', '__L__k2' ]); + await locker.acquire({ path: 'path1' }); + await locker.acquire({ path: 'path2' }); await locker.finalize(); + expect(Object.keys(store.internal)).toHaveLength(0); expect(redis.quit).toHaveBeenCalledTimes(1); }); }); diff --git a/test/util/Util.ts b/test/util/Util.ts index ff25bf333..bbd8ef54f 100644 --- a/test/util/Util.ts +++ b/test/util/Util.ts @@ -1,6 +1,7 @@ import type { Dirent, Stats } from 'fs'; import { PassThrough, Readable } from 'stream'; import type { SystemError } from '../../src/util/errors/SystemError'; +import Describe = jest.Describe; const portNames = [ // Integration @@ -19,6 +20,7 @@ const portNames = [ 'PodCreation', 'PodQuota', 'RedisLocker', + 'ResourceLockCleanup', 'RestrictedIdentity', 'SeedingPods', 'ServerFetch', @@ -40,11 +42,10 @@ export function getPort(name: typeof portNames[number]): number { return 6000 + idx; } -export function describeIf(envFlag: string, name: string, fn: () => void): void { +export function describeIf(envFlag: string): Describe { const flag = `TEST_${envFlag.toUpperCase()}`; const enabled = !/^(|0|false)$/iu.test(process.env[flag] ?? ''); - // eslint-disable-next-line jest/valid-describe-callback, jest/valid-title, jest/no-disabled-tests - return enabled ? describe(name, fn) : describe.skip(name, fn); + return enabled ? describe : describe.skip; } /**