sqlite: add timeout options to DatabaseSync #57752
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
| @@ -116,6 +116,8 @@ added: v22.5.0 | |||
| and the `loadExtension()` method are enabled. | |||
| You can call `enableLoadExtension(false)` later to disable this feature. | |||
| **Default:** `false`. | |||
| * `timeout` {number} Number of milliseconds to wait for the database to | |||
There was a problem hiding this comment.
I think we should define this in terms of a busy timeout and link to https://www.sqlite.org/c3ref/busy_timeout.html because I'm not sure everyone will understand what this does.
There was a problem hiding this comment.
Much better. Will do this.
geeksilva97
force-pushed
the
sqlite-dbsync-add-timeout
branch
from
47ee71b to
2913993
Compare
| }); | ||
| }); | ||
|
|
||
| test('trhows if lock is holden longer than the provided timeout', (t, done) => { |
There was a problem hiding this comment.
| test('trhows if lock is holden longer than the provided timeout', (t, done) => { | |
| test('throws if lock is held longer than the provided timeout', (t, done) => { |
| conn.exec('CREATE TABLE data (key INTEGER PRIMARY KEY, value TEXT)'); | ||
|
|
||
| // Spawns a child process to locks the database for 1 second | ||
| const child = spawn('./node', [fixturesPath('sqlite/lock-db.js'), databasePath], { |
There was a problem hiding this comment.
Does this need to happen in a child process? Would it work to create a second database connection in the same process?
|
|
||
| conn.exec('BEGIN EXCLUSIVE TRANSACTION'); | ||
| process.send('locked'); | ||
| setTimeout(() => { |
There was a problem hiding this comment.
I think we should try to avoid relying on setTimeout() as that will almost certainly make the tests prone to being flaky or inefficient in the CI. I think we could rely on message passing (or hopefully move this logic out of a child process and into the same process as the test) instead of a timeout.
No description provided.