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

Fix Potential crash on db close #72

Merged
merged 12 commits into from
Jan 27, 2025
Merged

Fix Potential crash on db close #72

merged 12 commits into from
Jan 27, 2025

Conversation

stevensJourney
Copy link
Collaborator

@stevensJourney stevensJourney commented Jan 23, 2025

Overview

This adds extra checks and mutex locks to the .close method. This should prevent some cases where closing a database might crash the application.

This PR includes 2 fixes which should help with crashes.

The first fix involves clearing callbacks for the SQLite commit and table change hooks on close. These hooks are handled asynchronously and are called via the React native invoker. A race condition could occur where an async callback is executed after the connection has closed and memory has been freed. The callback handlers now use safe shared pointer which is used to check if the connection is closed - if closed the callback execution is ended.

The second fix involves improving the waitForFinished logic has which occurs on close. This should prevent SQLite operations on a closed database. The JS write and read locks will now also reject any pending requests if the database has been closed.

Units tests have been added to ensure the close method works correctly.

Tests pass on Android and iOS locally.

TODOs

  • The iOS test app seems to be broken.

@stevensJourney stevensJourney marked this pull request as ready for review January 27, 2025 13:52
Copy link
Contributor

@rkistner rkistner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but did not check all the concurrency logic in detail.

@stevensJourney stevensJourney merged commit 43598db into main Jan 27, 2025
4 checks passed
@stevensJourney stevensJourney deleted the fix/close-multiple branch January 27, 2025 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants