dyedgreen / deno-sqlite

Deno SQLite module
https://deno.land/x/sqlite
MIT License
409 stars 36 forks source link

Broken locking logic #249

Open fabiospampinato opened 1 year ago

fabiospampinato commented 1 year ago

I'm not sure where the bug is exactly, but executing this code at the root of the project:

import {DB} from './mod.ts';

const db1 = new DB('foo.db');

db1.query ( `SELECT page_count * page_size as size FROM pragma_page_count(), pragma_page_size()` );

db1.close ();

And logging each time js_lock and js_unlock are called with something like this:

js_lock: (rid, exclusive) => {
  console.log ( `[js_lock] rid: ${rid}, exclusive: ${exclusive}` );
},
js_unlock: (rid) => {
  console.log ( `[js_unlock] rid: ${rid}` );
},

I get this log:

~/Downloads/deno-sqlite-master 2 ❯ deno run -A test.ts
[js_lock] rid: 3, exclusive: 0
[js_lock] rid: 3, exclusive: 0
~/Downloads/deno-sqlite-master 2 ❯ 

As we can see sqlite is requesting shared locks, but it's never releasing them. As long as there are shared locks an exclusive lock shouldn't be obtainable, so really sqlite here should be asking to unlock the database after its done with it, not asking for a shared lock twice, or it will prevent anybody else from writing to it, which can't be right.

I'm not sure what causes this bug. Maybe xCheckReservedLock can't be a sort of no-op?

This is worth investigating because without any form of locking that works I don't know how usable the approach really is.

fabiospampinato commented 1 year ago

Interestingly the equivalent code for node-sqlite3-wasm produces this output:

~/Desktop/sqlite3-wasm ❯ node test.js
[lock] rid 143528 - level 1
[unlock] rid 143528 - level 0
[lock] rid 143528 - level 1
[unlock] rid 143528 - level 0
[unlock] rid 143528 - level 0
[unlock] rid 143528 - level 0
~/Desktop/sqlite3-wasm ❯ 

So worth checking what they are doing differently, since sqlite seems to be making the right calls there.

They seem to be implementing xCheckReservedLock, I'm not sure if that's it or if there's more to the story.

dyedgreen commented 1 year ago

Yes, my guess would be that we have to implement that method. It's worth noting that the locking only works with --unstable, since Deno only provides an unstable locking API (see https://deno.land/api@v1.36.3?s=Deno.flockSync&unstable=)