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

WebLocks doesn't support multiple connections to the same database in the same context #51

Closed
rhashimoto opened this issue May 13, 2022 · 5 comments
Labels
bug wontfix

Comments

@rhashimoto
Copy link
Owner

@rhashimoto rhashimoto commented May 13, 2022

WebLocks (and WebLocksShared) associate lock state with the filename. That means it can't distinguish multiple connections to the same database using the same WebLocks instance. The connections would overwrite each other's state.

An easy fix would be to give every open file its own instance of WebLocks, but...

Concurrent access to the same database in the same SQLite context where one access is a writer doesn't work anyway because Asyncify-ed WebAssembly is not re-entrant during an asynchronous callback. It isn't possible for one database call to block and then be able to make another database call, so fixing the bug won't enable any new usages.

Note that you can access the same database in the same SQLite context as long each call into SQLite completes before the next call is made.

@rhashimoto rhashimoto added bug wontfix labels May 13, 2022
@rhashimoto rhashimoto closed this May 13, 2022
@steida
Copy link

@steida steida commented May 14, 2022

In the other words, it means the app has to enforce async serial access. For example, this code will fail with Error: cannot commit - no transaction is active.

db.sqlite3.exec(
  db.connection,
  `
  begin;
  select * from a
`
);
db.sqlite3.exec(
  db.connection,
  `
  commit;
`
);

But this code will work because it waits:

// note added `await`
await db.sqlite3.exec(
  db.connection,
  `
  begin;
  select * from a
`,
  (row) => {
    console.log(row);
  }
);
db.sqlite3.exec(
  db.connection,
  `
  commit;
`
);

And this code will not work because the first transaction has to be completed before the second is called.

await db.sqlite3.exec(
  db.connection,
  `
  begin;
  select * from a
`
);
db.sqlite3.exec(
  db.connection,
  `
  begin;
  select * from a;
`
);

So it depends on the content of an actual SQL string. As I see it, the app API needs a transaction helper function wrapper to ensure all transactions are run serially.

Sync SQLite with IDB would be great, but I understand your reasoning...

@rhashimoto
Copy link
Owner Author

@rhashimoto rhashimoto commented May 14, 2022

You're right that the app must ensure that calls to API functions that return a Promise (e.g. sqlite3.exec) are completed before the next API call is made on the same connection. But that is a constraint SQLite itself has so although your examples illustrate correct and incorrect library use, they don't apply to this bug.

This bug is about what happens with separate library connections to the same database, i.e. with two connections running concurrently, e.g.:

await Promise.all([
  sqlite3.exec(connectionA, `INSERT INTO a VALUES ('foo')`),
  sqlite3.exec(connectionB, `INSERT INTO a VALUES ('bar')`)
]);

Ideally this should work. One of the connections would grab the lock and the other would block until the lock was released. That's how it would work if you translated this code to C, using threads or coroutines. It doesn't work in wa-sqlite for two reasons: one is because the connections will clobber each others' lock state which is this bug, and the other is because you can't start a new call to an Asyncify-ed library while another call is suspended. I'm not fixing the first problem because there isn't anything I can do about the second problem.

@steida
Copy link

@steida steida commented May 14, 2022

What is the reason all API calls are not processed serially? Do you want to enable parallel reads? Or something else? Thank you.

@rhashimoto
Copy link
Owner Author

@rhashimoto rhashimoto commented May 14, 2022

I'm not sure what you mean, exactly. Some API calls return a Promise because they might call into a VFS, which might be asynchronous.

I think the only other way to go would be to make every API call asynchronous and queue them inside the library. The IDBContext helper class does something like that for IndexedDB accesses (with the primary goal of reusing existing IDBTransaction instances).

Not doing that in the main library doesn't enable any special features, so no concurrent submissions. It might be marginally faster than a Promise on every call, but I doubt that savings is significant. I think the main reason is that I wanted to keep the API close to the C API (as long as that made sense) so I made the bare minimum of calls asynchronous. I don't think the other way makes application programming any easier, but perhaps your opinion differs. It would be straightforward (although a little tedious) to write a middleware wrapper for that.

@steida
Copy link

@steida steida commented May 14, 2022

Yes, I meant queuing calls inside the wa-sqlite library. I agree it can be done in userland. If only the sync version was doable. Thank you for your work and detailed responses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug wontfix
Projects
None yet
Development

No branches or pull requests

2 participants