dyedgreen/deno-sqlite

Multi-instance locking

Opened this issue · 4 comments

fabiospampinato commented

Reading the code I think there might be a fairly important problem regarding locks, as I understand it POSIX advisory locks are per-process, so for example if 3 instances of sqlite from the same process request a lock, and then 1 of them releases it, the lock is released for all the instances, because there's no sort of reference counting and all 3 of those instances exist inside the same process.

This scenario doesn't seem to be handled, so it looks like it could lead to corrupted databases if two or more database instances pointing to the same database are made.

dyedgreen commented

I think this is correct. I’m not sure we can make this work correctly across workers; but we could add some bookkeeping internal to the VFS to address this issue?

fabiospampinato commented

I guess so 🤔 It seems tricky though, because it seems like sqlite wants a lock immediately.

It's unclear how js_lock and js_unlock work actually, like:

  1. is there any way to tell sqlite that a lock couldn't be acquired? And what happens if you do that?
  2. is there a way for sqlite to sleep for some time while waiting on a lock without blocking the thread? I guess not since the API is synchronous?
  3. are lock requests scoped by sqlite instance? Like can one know if the instance of sqlite asking for a lock on a fd already has one? It looks like the vfs is indeed scoped by instance, but I'm not entirely sure about this.
dyedgreen commented
  1. Yes there is; I think this results in a combination of sleeps / errors returned back to the caller
  2. There is a sleep call in the vfs, but that blocks the thread (not sure if there is a way around that with WASM?)
  3. The vfs is currently scoped per db instance, but we could maintain a global lookup table of locks
fabiospampinato commented

Since Deno provides file locking APIs that work for macos and linux this should probably be fixed with some kind of global bookkeeping. For Windows though, where such APIs don't seem to work right, potentially the same approach that I'm using for Node VFS could be used. It would have some downsides, but it should allow opening the same files with multiple instance of deno-sqlite databases safely, which currently is not the case I think.