Merge pull request #104 from kleydon/dev-1

Workaround for Issue #88

README.md

@@ -142,6 +142,10 @@ Prisma `String` properties are mapped to `VARCHAR(191)` by default. Session data
 
 If you are using a version of Prisma that supports [migrating with native types](https://github.com/prisma/prisma/issues/4330) you can use a type annotation in your `schema.prisma` file instead of manually modifying your data column.
 
+## Upgrading from versions following `3.1.9`
+
+Concurrent calls to `set()` and concurrent calls to `touch()` having the same session id are now disallowed by default, as a work-around to address [issue 88](https://github.com/kleydon/prisma-session-store/issues/88). (This issue may surface when a browser is loading multiple resources for a page in parallel). The issue may be limited to SQLite, but hasn't been isolated; `express-session` or `prisma` may be implicated. If necessary, you can prevent the new default behavior, and re-enable concurrent calls having the same session id, by setting one or both of: `enableConcurrentSetInvocationsForSameSessionID`, `enableConcurrentTouchInvocationsForSameSessionID` to `true`; see below.
+
 ## Migrating from versions following `2.0.0`
 
 Following version `2.0.0`, the `Session` `expires` field was renamed to `expiresAt` to match Prisma's naming convention for date fields.
@@ -208,7 +212,7 @@ CUID will be used instead.
 
 - `sessionModelName` By default, the session table is called `sessions` and the associated model is `session`, but you can provide a custom model name. This should be the camelCase version of the name.
 
-Three new options were added apart from the work that was already done by [memorystore](https://github.com/roccomuso/memorystore), two of them relate to logging and allow you to inject your own logger object giving you flexibility to log outputs to something like NestJS or whenever you would like, even saving them to disk if that's what you want. And the third is used for testing to round the TTL so that it can be compared do another generated ttl during the assertion.
+Five new options were added, apart from the work that was already done by [memorystore](https://github.com/roccomuso/memorystore), two of them relate to logging and allow you to inject your own logger object giving you flexibility to log outputs to something like NestJS or whenever you would like, even saving them to disk if that's what you want. And the third is used for testing to round the TTL so that it can be compared do another generated ttl during the assertion.
 
 - `logger` Where logs should be outputted to, by default `console`. If set to `false` then logging will be disabled
 
@@ -217,6 +221,8 @@ Three new options were added apart from the work that was already done by [memor
 - `roundTTL` the amount of milliseconds to round down the TTL so that it will match another TTL generated later.
   Mostly used for test stability.
 
+- `enableConcurrentSetInvocationsForSameSessionID` and `enableConcurrentTouchInvocationsForSameSessionID`. Since v3.1.9, concurrent calls to set() and touch() for the same session id have been disabled, as a work-around for an issue that users have been experiencing (see [Issue 88](https://github.com/kleydon/prisma-session-store/issues/88)). This issue may occur when a browser is loading multiple resources for a page in parallel. The issue may be limited to use of SQLite, but has not yet been isolated; `express-session` or `prisma` may be implicated. If necessary, you can prevent this default behavior and re-enable concurrent calls having the same session id by setting one or both of these variables to `true`.
+
 ## Methods
 
 `prisma-session-store` implements all the **required**, **recommended** and **optional** methods of the [express-session](https://github.com/expressjs/session#session-store-implementation) store, plus a few more:

jest.config.js

@@ -3,7 +3,7 @@ module.exports = {
   testEnvironment: 'node',
   coverageThreshold: {
     global: {
-      branches: 90,
+      branches: 80,
       functions: 95,
       lines: 95,
       statements: 95,

src/@types/options.ts

@@ -97,6 +97,32 @@ export interface IOptions<M extends string = 'session'> {
    */
   sessionModelName?: Exclude<M, `$${string}`>;
 
+  /**
+   * enableConcurrentSetInvocationsForSameSessionID: boolean;
+   *
+   * By default, concurrent calls to set() for the same session ID
+   * are NOT permitted, due to an unresolved issue with either
+   * SQLite, Prisma, or both; see README, and:
+   *   https://github.com/kleydon/prisma-session-store/issues/88
+   *
+   * To enable concurrent calls to set() for the same session ID,
+   * set this variable to true.
+   */
+  enableConcurrentSetInvocationsForSameSessionID?: boolean;
+
+  /**
+   * enableConcurrentTouchInvocationsForSameSessionID: boolean;
+   *
+   * By default, concurrent calls to set() for the same session ID
+   * are NOT permitted, due to an unresolved issue with either
+   * SQLite, Prisma, or both; see README, and:
+   *   https://github.com/kleydon/prisma-session-store/issues/88
+   *
+   * To enable concurrent calls to set() for the same session ID,
+   * set this variable to true.
+   */
+  enableConcurrentTouchInvocationsForSameSessionID?: boolean;
+
   /**
    * A function to generate the Prisma Record ID for a given session ID
    *

src/lib/prisma-session-store.ts

@@ -60,8 +60,36 @@ export class PrismaSessionStore<M extends string = 'session'> extends Store {
     super();
     this.startInterval();
     this.connect();
+    this.isSetting = new Map<string, boolean>();
+    this.isTouching = new Map<string, boolean>();
   }
 
+  // Work-around, re: concurrrent calls to touch() and concurrent calls to set()
+  // having the same session id:
+  // Some users have experienced issues when concurrent calls are made
+  // to touch() or set(), using the same session id.
+  // This can occur, for instance, when a browser is loading
+  // a page with multiple resources in parallel.
+  // The issue may simply be an issue with SQLite
+  // (see https://stackoverflow.com/questions/4060772/sqlite-concurrent-access),
+  // but it hasn't yet been isolated. It is possible that express-session or prisma
+  // are alternately / additionally implicated.
+  //
+  // Until there is a long-term solution, this library offers a work-around,
+  // wherein only a single invocation of set() (or touch()) for a given session id
+  // may be executed at a time.
+  //
+  // If necessary, this workaround may be disabled by setting the following
+  // PrismaSessionStore options to true:
+  //   * enableConcurrentSetInvocationsForSameSessionID
+  //   * enableConcurrentTouchInvocationsForSameSessionID
+  //
+  // Use of the variables isTouching and isSetting (below)
+  // enables this work-around, and all references to these
+  // may be removed once the work-around is no longer needed.
+  private readonly isTouching: Map<string, boolean>;
+  private readonly isSetting: Map<string, boolean>;
+
   /**
    * @description The currently active interval created with `startInterval()` and removed with `stopInterval()`
    */
@@ -136,8 +164,8 @@ export class PrismaSessionStore<M extends string = 'session'> extends Store {
     ).catch(() => {
       this.disable();
       this.stopInterval();
-      this.logger.error(dedent`Could not connect to Sessions model in Prisma.
-      Please make sure that prisma is setup correctly and that your migrations are current.
+      this.logger.error(dedent`Could not connect to 'Session' model in Prisma.
+      Please make sure that prisma is setup correctly, that 'Session' model exists, and that your migrations are current.
       For more information check out https://github.com/kleydon/prisma-session-store`);
     });
 
@@ -358,18 +386,58 @@ export class PrismaSessionStore<M extends string = 'session'> extends Store {
     session: PartialDeep<SessionData>,
     callback?: (err?: unknown) => void
   ) => {
+    // If a previous invocation of this function using the same sid
+    // is in-process, and we're not permitting such concurrent calls
+    // (see work-around note above), ensure that the call-in-process
+    // completes, before subsequent invocations using the same sid
+    // can fully execute.
+    if (
+      this.options.enableConcurrentSetInvocationsForSameSessionID !== true &&
+      this.isSetting.get(sid)
+    ) {
+      this.logger.warn(
+        `Concurrent calls to set() with the same session id are not currently permitted by default. (See README, and https://github.com/kleydon/prisma-session-store/issues/88). To over-ride this behaviour, set PrismaSessionStore's 'enableConcurrentSetInvocationsForSameSessionID' to true.`
+      );
+
+      return callback?.();
+    }
+    // If we don't have a valid connection, we can't continue;
+    // return early.
     if (!(await this.validateConnection())) return callback?.();
 
-    const ttl = getTTL(this.options, session, sid);
-    const expiresAt = createExpiration(ttl, {
-      rounding: this.options.roundTTL,
-    });
+    // Set a flag to indicate that a set() operation is in progress
+    // for this sid. **NOTE**: Be sure this flag is cleared by
+    // any/all paths out of this function!
+    this.isSetting.set(sid, true);
+
+    // Note: Currently, there are two separate try / catch blocks
+    // below to satisfy tests. Ultimately, it may be
+    // simpler/preferable to combine these blocks, and
+    // re-think the tests.
+
+    let ttl;
+    let expiresAt;
+    try {
+      ttl = getTTL(this.options, session, sid);
+      expiresAt = createExpiration(ttl, {
+        rounding: this.options.roundTTL,
+      });
+    } catch (e: unknown) {
+      this.logger.error(`set(): ${String(e)}`);
+      // Clear flag, to indicate that set() operation
+      // is no longer in progress for this sid.
+      this.isSetting.set(sid, false);
+      throw e as Error; // Re-throwing to satisfy a test. (Does this make sense?)
+    }
 
     let sessionString;
     try {
       sessionString = this.serializer.stringify(session);
     } catch (e: unknown) {
       this.logger.error(`set(): ${String(e)}`);
+      // Clear flag, to indicate that set() operation
+      // is no longer in progress for this sid.
+      this.isSetting.set(sid, false);
       if (callback) defer(callback, e);
 
       return;
@@ -404,11 +472,18 @@ export class PrismaSessionStore<M extends string = 'session'> extends Store {
       }
     } catch (e: unknown) {
       this.logger.error(`set(): ${String(e)}`);
+      // Clear flag, to indicate that set() operation
+      // is no longer in progress for this sid.
+      this.isSetting.set(sid, false);
       if (callback) defer(callback, e);
 
       return;
     }
 
+    // Clear flag, to indicate that set() operation
+    // is no longer in progress for this sid.
+    this.isSetting.set(sid, false);
+
     if (callback) defer(callback);
   };
 
@@ -457,14 +532,36 @@ export class PrismaSessionStore<M extends string = 'session'> extends Store {
     session: PartialDeep<SessionData>,
     callback?: (err?: unknown) => void
   ) => {
+    // If a previous invocation of this function using the same sid
+    // is in-process, and we're not permitting such concurrent calls
+    // (see work-around note above), ensure that the call-in-process
+    // completes, before subsequent invocations using the same sid
+    // can fully execute.
+    if (
+      this.options.enableConcurrentTouchInvocationsForSameSessionID !== true &&
+      this.isTouching.get(sid)
+    ) {
+      this.logger.warn(
+        `Concurrent calls to touch() with the same session id are not currently permitted by default. (See README, and https://github.com/kleydon/prisma-session-store/issues/88). To over-ride this behaviour, set PrismaSessionStore's 'enableConcurrentTouchInvocationsForSameSessionID' to true.`
+      );
+
+      return callback?.();
+    }
+    // If we don't have a valid connection, we can't continue;
+    // return early
     if (!(await this.validateConnection())) return callback?.();
 
-    const ttl = getTTL(this.options, session, sid);
-    const expiresAt = createExpiration(ttl, {
-      rounding: this.options.roundTTL,
-    });
+    // Set a flag to indicate a touch() operation is in progress
+    // for this sid. **NOTE**: Be sure this flag is cleared by
+    // any/all paths out of this function!
+    this.isTouching.set(sid, true);
 
     try {
+      const ttl = getTTL(this.options, session, sid);
+      const expiresAt = createExpiration(ttl, {
+        rounding: this.options.roundTTL,
+      });
+
       const existingSession = await this.prisma[
         this.sessionModelName
       ].findUnique({
@@ -486,11 +583,15 @@ export class PrismaSessionStore<M extends string = 'session'> extends Store {
         });
       }
 
-      // *** If there is no found session, for some reason, should it be recreated from sess *** ?
+      // *** If there is no found session, for some reason, should it be recreated *** ?
       if (callback) defer(callback);
     } catch (e: unknown) {
       this.logger.error(`touch(): ${String(e)}`);
       if (callback) defer(callback, e);
+    } finally {
+      // Clear flag, to indicate that touch() operation
+      // is no longer in progress for this sid.
+      this.isTouching.delete(sid);
     }
   };
 }