fix: Introducing initializers for handling lock cleanup on start

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';

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<void> {
-    await this.finalizer.finalize();
+    await this.finalizer.handleSafe();
   }
 }

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<void>;
+}

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<void> {
+    return this.initializable.initialize();
+  }
+}

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 {}

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<void>;

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<void> {
+    return this.finalizable.finalize();
+  }
+}

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 {}

src/init/final/ParallelFinalizer.ts

@@ -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<void> {
-    await Promise.all(this.finalizers.map(async(finalizer): Promise<void> => finalizer.finalize()));
-  }
-}

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<AttemptSettings> = { 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<AttemptSettings>;
   /** 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<void> {
+    // 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<void> {
-    // 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}`);
   }
 }

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<AttemptSettings>;
+  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<RedisAnswer>): () => Promise<unknown> {
+    if (this.finalized) {
+      throw new Error('Invalid state: cannot execute Redis operation once finalize() has been called.');
+    }
     return async(): Promise<unknown> => {
       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<void> {
+    // On server start: remove all existing (dangling) locks, so new requests are not blocked.
+    return this.clearLocks();
+  }
 
   public async finalize(): Promise<void> {
-    // 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<void> {
+    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);
+    }
+  }
 }