-
-
Notifications
You must be signed in to change notification settings - Fork 37
feat: Migrate JS-sided locks to DatabaseQueue for transactions and executeBatch commands
#227
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
Conversation
DatabaseQueue for transactions and executeBatch commands
| openDatabaseQueue(options.name) | ||
| } catch (error) { | ||
| throw NitroSQLiteError.fromError(error) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Resource leak in database open error handling
Resource leak in error handling: If HybridNitroSQLite.open() succeeds but openDatabaseQueue() throws an error (e.g., if the database queue is already open), the native database will remain open while the database queue is not created. This leaves the system in an inconsistent state where the native database is open but the queue tracking doesn't exist. The code should either close the native database in the error handler, or call openDatabaseQueue() before opening the native database.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true, but has been the behavior previously as well. I will address having multiple simultaneous database connections in a follow-up PR
| } catch (error) { | ||
| throw NitroSQLiteError.fromError(error) | ||
| } | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Resource Leaks from Failed Close Blocking Reopen
Resource leak in error handling: If HybridNitroSQLite.close() throws an error, closeDatabaseQueue() is never called, leaving the database queue entry in the databaseQueues map. This prevents the database from being reopened later since openDatabaseQueue() will throw an error saying the database is already open. The code should ensure closeDatabaseQueue() is called even if closing the native database fails, possibly by reversing the order or using a try-finally pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
Changes the current JS lock implementation in favor of a
DatabaseQueue, which is used for both transactions and batch executionsNote
Replace JS lock mechanism with a DatabaseQueue that serializes transactions and executeBatch operations, integrate it with session lifecycle, and update tests accordingly.
package/src/DatabaseQueue.tswithopenDatabaseQueue/closeDatabaseQueue,queueOperationAsync,startOperationSync, and per-DBqueue/inProgresstracking.transactionto run viaDatabaseQueue, add finalized-state guards, wrap errors withNitroSQLiteError, support optional exclusive begin, and return genericResult.executeBatch/executeBatchAsyncthroughDatabaseQueue; enforcethrowIfDatabaseIsNotOpen.open/closetoopenDatabaseQueue/closeDatabaseQueue; remove legacy lock usage.NitroSQLiteError; updateNitroSQLiteConnection.transactionsignature to be generic and return the callback result.example/src/tests/unit/specs/DatabaseQueue.spec.tsto verify queuing and error propagation for mixed operations.isNitroSQLiteError, adjust paths, and rely onresetTestDb(); exposetestDbQueue/TEST_DB_NAMEfor assertions.../packageinexample/tsconfig.json.Written by Cursor Bugbot for commit 9bcae9c. This will update automatically on new commits. Configure here.