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

Ensure thread safety for query listener collection on native driver #4567

Merged
merged 3 commits into from
Aug 28, 2023

Conversation

kpgalligan
Copy link
Collaborator

The query listener collection is potentially accessed from multiple threads and needs to be thread safe. That is very likely the cause of: #4376

It is difficult to say for certain if that is the cause, as race condition issues don't generally produce obvious or repeatable crashes, but if you take the locks out of this PR and run the test, it'll crash hard.

For workaround in the issue, I'll comment on that in the issue in more detail. Summary, though, unless all access was in the same thread, it's still possible to have race conditions, and it looks like there are multiple threads still.

Presumably this has been an issue since this change: https://github.com/cashapp/sqldelight/pull/4148/files#diff-18eaa5efaf87c02833bc443af34a18778468fbe7e35dc9de4f01c61277593ce6R139

This is a pretty big runtime issue, so we should get a fix out, but again, it's hard to say for sure this fixes the originally observed behavior, so we'll really need to have it published and watch what happens.

@kpgalligan
Copy link
Collaborator Author

This collection was also changed in the PR that removed the old memory model.

https://github.com/cashapp/sqldelight/blob/kgalligan/2023-08-25/listener-concurrency/drivers/native-driver/src/nativeMain/kotlin/app/cash/sqldelight/driver/native/NativeSqlDatabase.kt#L346

Unlike the listeners, the statement cache should be OK as the connections are owned by threads. I wouldn't say I love having a regular mutable map here, but it should be OK, and unnecessary locking should be avoided.

@kpgalligan kpgalligan enabled auto-merge (squash) August 25, 2023 17:16
@kpgalligan kpgalligan merged commit 1840e8a into master Aug 28, 2023
7 checks passed
@kpgalligan kpgalligan deleted the kgalligan/2023-08-25/listener-concurrency branch August 28, 2023 20:54
hfhbd pushed a commit that referenced this pull request Apr 2, 2024
…4567)

* Ensure thread safety for query listener collection on native driver

* Spotless fix
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

3 participants