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

Add a SqlDriver for Androidx KMP SQLiteDriver #5283

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

eygraber
Copy link
Contributor

@eygraber eygraber commented Jun 3, 2024

Androidx has a SQLiteDriver that targets Android, iOS, Mac, Linux, and JVM Desktop.

This PR provides a SqlDriver that wraps it. Currently the implementation only works for Android and JVM (it uses a ThreadLocal and I wanted to get some feedback before expanding that functionality to the native targets).

@eygraber
Copy link
Contributor Author

eygraber commented Jun 3, 2024

Once I'm here, this line from AndroidSqliteDriver tripped me up. I can't figure out in what scenario the previous connection should be closed, because if identifier != null then either there was no connection in the cache and a new one is made, or the connection was reused and removed from the cache so there's nothing to close.

@JakeWharton
Copy link
Member

Once I'm here, this line from AndroidSqliteDriver tripped me up. I can't figure out in what scenario the previous connection should be closed, because if identifier != null then either there was no connection in the cache and a new one is made, or the connection was reused and removed from the cache so there's nothing to close.

Two threads racing at the same time.

@eygraber
Copy link
Contributor Author

eygraber commented Jun 3, 2024

Two threads racing at the same time.

Should've been my first guess 😅

cache4k doesn't have the same semantics as LruCache so to handle that you need to listen for an Update event, and if the values don't match each other, close the old one.

@eygraber eygraber force-pushed the androidx-sqlite-driver branch 2 times, most recently from 68dff3e to 2dda23e Compare June 3, 2024 17:47
@eygraber
Copy link
Contributor Author

eygraber commented Jun 3, 2024

Some weird stuff going on with tests that I'm working through.

Also had an idea for how to make this work on native. Should I do that here, or have a follow up PR?

@eygraber eygraber force-pushed the androidx-sqlite-driver branch 2 times, most recently from 14cf97a to 131fab9 Compare June 4, 2024 06:16
@eygraber
Copy link
Contributor Author

eygraber commented Jun 4, 2024

Got a native implementation in. I don't have the most experience with Native APIs, but I think this should clean up after itself since in endTransaction eventually transactions.set(enclosingTransaction) will get called with a null value (I think).

I'm also good with removing that part for now. In any case this should be ready to go.

@eygraber eygraber force-pushed the androidx-sqlite-driver branch 6 times, most recently from 5e697c2 to b7669b4 Compare June 4, 2024 08:58
  - The issue was this driver executes more than the Android one because of the PRAGMA version calls
  - Solution was to increment the identifier so it doesn't clash with the previous one (using a cached query for a statement and vice versa
  - There was also a Robolectric issue with unclosed Closeables
    - I fixed that for the existing AndroidSqliteDriver tests as well
null2264 added a commit to null2264/yokai that referenced this pull request Jul 4, 2024
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.

None yet

2 participants