Concurrent `setItem` and `getItem` can lead into an unahdled `does not look like a valid storage file`

[tndev]

Concureent setItem and getItem can lead into an unahdled does not look like a valid storage file:

parse error:  {} for:
     Error: [node-persist][readFile] .node-persist/storage/37b51d194a7513e45b56f6524f2d51f2 does not look like a validstorage file!
      at fs.readFile (node_modules/node-persist/src/local-storage.js:278:66)
      at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:525:3)

The code used to reproduce create that error was:

const localdb = require('node-persist')
const storage = localdb.create({ ttl: true, logging: true })

async function test2 () {
  await storage.setItem('bar', { 'foo': 'Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.' })
  await storage.getItem('bar')
  await storage.setItem('bar', { 'foo': 'Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.' })
  await storage.getItem('bar')
  await storage.setItem('bar', { 'foo': 'Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. At vero eos et accusam et justo duo dolores et ea rebum. Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.' })
  await storage.getItem('bar')
}

async function test3 () {
  await storage.getItem('bar')
  await storage.getItem('bar')
  await storage.getItem('bar')
  await storage.getItem('bar')
  await storage.getItem('bar')
}

async function main () {
  await storage.init()

  try {
    await Promise.all([
      test2(),
      test3(),
      test2(),
      test3(),
      test2()
    ])
  } catch (err) {
    console.error(err)
  }
}

main().catch(err => {
  console.error(err)
})

The problem cannot be realiable be reproduces.

[akhoury]

we could use file locking i guess, maybe try https://github.com/moxystudio/node-proper-lockfile

[tndev]

Yes that might be a solution. I honestly don't use your library so I don't have much knowledge about how it works, and if and what kind of bottelneck that would introduce.

I just stumbled across a question on stackoverflow regarding your library, and figured out that problem while I tried to give an solution to the question.

[akhoury]

can you link the question please?

[biswanaths]

This is the SO question https://stackoverflow.com/questions/52582180/node-persist-on-heavy-node-js-app-not-returning-values/52632704#52632704

[biswanaths]

I think the better approach would be to serialise read and writes based on the queue. One approach as mentioned in my SO answer(see the gist) above is to chain promises. I think we can enhance it to simply chain promises per key rather than whole universe of read and write through the library.

Please see the basic idea here : https://gist.github.com/biswanaths/b8999c9e4e5a9e979dcde061074528f6

I believe whatever solution to this problem should live in library. The end user does not need to know and work around whether the library is storing kv per file or using a single file.

[akhoury]

well, what if it's a different process writing to the same file?

[biswanaths]

@akhoury , Oh, yes. This obviously does will not work of multiprocess I don't know where node-persist, use case falls, like single-process, multi-process or may be getting used as part of a distributed application.

Also going through the old case, I think Issue 31 more or less deals with the same issue.

[DragonEmbers]

I've also hit this problem, but in my case, I'm not performing asynchronous get/sets but a series of ~10 sets asynchronously using Promise.all(array.map([function that creates promises])). The library hits on the same file name a number of times and ends up writing n values to the same file, thus ending up with a corrupt file.

Given that asynchronicity is commonplace in nodejs, it would be a lot better if this was handled by the library using file locks.

[Weedoze]

This problem is still happening. What is the status of the development about this topic ?

[robbie-cahill]

When storage.init (storage being node-persist) encounters a corrupt file it throws this error.

I tried using try/catch and .catch() callbacks to handle this error gracefully, but neither work because the error is not bubbling up to where I can handle it.

For those looking for a drop-in replacement for this library, I've switched to https://www.npmjs.com/package/node-localstorage and it seems to be working fine. The api is basically similar with getItem() and setItem()

[ryanleesmith]

node-localstorage doesn't appear to be async, or offer an async api.

Regarding this plugin's error - I have a .catch around every line of code related to it (init, getItem, setItem, removeItem) and it's still throwing the uncaught exception. Even more confusing, it happens at a periodic interval - which would be explained by the fact that my code periodically polls and uses the plugin for reading/writing, but I actually removed all references in the code to the corrupted file in question. It's unfortunate as this is otherwise a fine plugin.

[ryanleesmith]

Ok, figured it out - the interval for expiring keys does a scan, which is where the uncaught exception is originating from, and why you can't catch it. Passing in expiredInterval: 0 as an option in the init resolves the issue for me. If I understand correctly, this means keys will not be auto-expired, but getItem will still honor the TTL. Depending on your usage, this may or may not be an issue.

[SamNChiet]

Wait, wouldn't this kind of invalidate the library for use as a server-side application that's handling requests?
Would it work to wrap getItem and setItem with something that tracks files being modified, or should I just jump to something else?