Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Occasional error when reading session data from file system #4353

Open
midgleyc opened this issue Oct 11, 2022 · 7 comments
Open

Occasional error when reading session data from file system #4353

midgleyc opened this issue Oct 11, 2022 · 7 comments
Labels
bug:unverified feat:sessions Sessions related issues

Comments

@midgleyc
Copy link

What version of Remix are you using?

1.6.5

Steps to Reproduce

  1. Use file session storage (createFileSessionStorage)
  2. Create a large amount of session data (>1MB), saved and read each page load.
  3. Visit lots of pages quickly.

Expected Behavior

No errors.

Actual Behavior

Occasionally, the page fails to load:

SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)
    at readData (/srv/app/node_modules/@remix-run/node/dist/sessions/fileStorage.js:91:28)
    at getSession (/srv/app/node_modules/@remix-run/server-runtime/dist/sessions.js:113:25)
    at loader10 (/srv/app/build/index.js:2229:17)
    at Object.callRouteLoader (/srv/app/node_modules/@remix-run/server-runtime/dist/data.js:77:14)
    at handleDataRequest (/srv/app/node_modules/@remix-run/server-runtime/dist/server.js:113:18)
    at requestHandler (/srv/app/node_modules/@remix-run/server-runtime/dist/server.js:34:18)
    at /srv/app/node_modules/@remix-run/express/dist/server.js:39:22
@machour machour added the feat:sessions Sessions related issues label Oct 11, 2022
@kiliman
Copy link
Collaborator

kiliman commented Oct 11, 2022

The file session storage implementation doesn't work well since loaders can run in parallel. There is no file-locking on the current one so it's very possible to corrupt session data.

@kiliman
Copy link
Collaborator

kiliman commented Oct 11, 2022

I've got a working version with file locks, but still trying to figure out performance issues.

@raskyer
Copy link

raskyer commented Feb 28, 2023

Hello, do you know if any progress could be made around this issue ?

@dusty
Copy link

dusty commented Mar 1, 2023

I just made one to run synchronously and it doesn't corrupt - we only use this in local dev - we use redis in prod.

// https://remix.run/docs/en/v1/api/remix#createsessionstorage
import type { Cookie } from '@remix-run/node'
import { createSessionStorage } from '@remix-run/node'
import crypto from 'crypto'
import fs from 'fs'
import path from 'path'

interface FileSessionStorageOptions {
  cookie: Cookie
  dir: string
}

// sync version so the file isn't corrupted on concurrent writes
export function createFileSessionStorage({ cookie, dir }: FileSessionStorageOptions) {
  return createSessionStorage({
    cookie,
    async createData(data, expires) {
      const content = JSON.stringify({ data, expires })
      // eslint-disable-next-line no-constant-condition
      while (true) {
        const id = crypto.randomUUID({ disableEntropyCache: true })
        try {
          const file = getFile(dir, id)
          fs.mkdirSync(path.dirname(file), { recursive: true })
          fs.writeFileSync(file, content, { encoding: 'utf-8', flag: 'wx' })
          return id
        } catch (error) {
          if (error.code !== 'EEXIST') throw error
        }
      }
    },
    async readData(id) {
      try {
        const file = getFile(dir, id)
        const content = JSON.parse(fs.readFileSync(file, 'utf-8'))
        if (!isExpired(content.expires)) return content.data
        fs.unlinkSync(file)
        return null
      } catch (error) {
        if (error.code !== 'ENOENT') throw error
        return null
      }
    },
    async updateData(id, data, expires) {
      const content = JSON.stringify({ data, expires })
      const file = getFile(dir, id)
      fs.mkdirSync(path.dirname(file), { recursive: true })
      fs.writeFileSync(file, content, 'utf-8')
    },
    async deleteData(id) {
      try {
        fs.unlinkSync(getFile(dir, id))
      } catch (error) {
        if (error.code !== 'ENOENT') throw error
      }
    },
  })
}

// Divide the session id up into a directory (first 2 bytes) and filename
// (remaining 6 bytes) to reduce the chance of having very large directories,
// which should speed up file access. This is a maximum of 2^16 directories,
// each with 2^48 files.
function getFile(dir: string, id: string) {
  return path.join(dir, id.slice(0, 4), id.slice(4))
}

function isExpired(date?: string) {
  const expires = typeof date === 'string' ? new Date(date) : null
  return expires ? expires < new Date() : false
}

@brophdawg11 brophdawg11 self-assigned this Aug 3, 2023
@brophdawg11 brophdawg11 added this to v2 Aug 3, 2023
@brophdawg11 brophdawg11 assigned mjackson and unassigned brophdawg11 Aug 3, 2023
@brophdawg11 brophdawg11 moved this to Backlog in v2 Aug 3, 2023
@brophdawg11 brophdawg11 removed this from v2 Sep 5, 2023
@akomm
Copy link

akomm commented Dec 12, 2023

Oddly I get this error only when my FF dev tools are open. I assumed some side-effect of opening it. It is though not the disabled cache, I've tested it.

Using the 2.3.1 vite plugin remix version.

@devnomic
Copy link

I think i hit same issue, this may need to be documented in the docs.
Can you share @kiliman ?

@kiliman
Copy link
Collaborator

kiliman commented Jan 29, 2024

@devnomic I would probably use the solution above by @dusty. I don't use file sessions, so I never got around to fixing the performance issues on my spike.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:unverified feat:sessions Sessions related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants