SIGSEGV: NativeSqliteDriver.notifyListeners() #4376
Comments
Reads are limited to a dispatcher with a parallelism of 4, which is equal to the newly set maximum connection readers on the SQLite driver. Writes are limited to a parallelism of 1, since that is what SQLDelight will do under the hood anyway. This seems to fix concurrency issues which result in crashes, such as sqldelight/sqldelight#4376.
Reads are limited to a dispatcher with a parallelism of 4, which is equal to the newly set maximum connection readers on the SQLite driver. Writes are limited to a parallelism of 1, since that is what SQLDelight will do under the hood anyway. This seems to fix concurrency issues which result in crashes, such as sqldelight/sqldelight#4376.
Reads are limited to a dispatcher with a parallelism of 4, which is equal to the newly set maximum connection readers on the SQLite driver. Writes are limited to a parallelism of 1, since that is what SQLDelight will do under the hood anyway. This seems to fix concurrency issues which result in crashes, such as sqldelight/sqldelight#4376.
|
For info: I moved all database write operations to a single parallelized coroutine dispatcher in chrisbanes/tivi#1400 which has definitely helped things. It still happens occassionally, but a whole lot less. |
|
I had the same problem, I could imagine that it is because there are zero values in the array, but I don't understand why this is so. |
This adds a potential workaround for sqldelight/sqldelight#4376
|
As linked above, I think I've found the reason this is happening, although I don't know why. For some reason null entries are being added to the Looks like a K/N bug to me somewhere, but tricky to work out exactly where this is coming from. Might be something to do with using a MutableMap + concurrency. My workaround moves the root I'll give it a few days or so to see if the workaround fixes the crashes (which are happening ~5 times per day for me). If it does, I'll send a PR. |
This adds a potential workaround for sqldelight/sqldelight#4376
|
wowowow, thank you for investigating |
|
Looking at it, the map was this: private class BasicMutableMapShared<K : Any, V : Any> : BasicMutableMap<K, V> {
private val lock = PoolLock(reentrant = true)
private val data = mutableMapOf<K, V>()
override fun getOrPut(key: K, block: () -> V): V = lock.withLock { data.getOrPut(key, block) }
override val values: Collection<V>
get() = lock.withLock { data.values }
override fun put(key: K, value: V) {
lock.withLock { data[key] = value }
}
override fun get(key: K): V? = lock.withLock { data[key] }
override fun remove(key: K) {
lock.withLock { data.remove(key) }
}
override val keys: Collection<K>
get() = lock.withLock { data.keys }
override fun cleanUp(block: (V) -> Unit) {
lock.withLock {
data.values.forEach(block)
}
}
}That was added last year to support both the strict memory model and the current one. Then this year, the driver was changed to use a regular mutable map without any locking. Right now, I don't know if that's causing issues, but if nowhere up the call chain is enforcing thread safety, it's certainly a potential problem. I want to dig through those changes to see if there are any other places where collections have been changed as well. Will also look into the workaround. Taking a step back, I'm not sure pulling out the map I had in buys much because it was written to support both memory models, but I don't know the scope of the changes yet. So, summary, non-thread-safe map in a place likely to be interacting with multiple threads. |
|
Just a note. On map vs set, it was using a map because the concurrent map implementation could implement a set without needing to actually write a different set implementation for concurrency. Or, to state differently, I took the lazy option :) Strictly speaking, a set is what should be used there. |
|
So, simple "fix", assuming it does actually fix the issue: #4567. Pretty big problem, so we should get this out there. On why the workaround potentially didn't solve the issue, it looks like there are still different threads for read/write. I'm going somewhat by memory here, but the query listener would likely be added by the read, but query listener notifications would run in the thread that updated the DB, so there's still collection access from multiple threads. A reduction in crashes, but not a completely prevention, makes sense in that context. However, race conditions are tough. I'd like to see what happens out in the wild with this change before I say it's "fixed", in case there's something else going on as well. |
|
@chrisbanes I took a look at tivi and notices the workaround above has changed some. I'm wondering about this: private class WorkaroundNativeSqliteDriver(
schema: SqlSchema<QueryResult.Value<Unit>>,
name: String,
maxReaderConnections: Int = 1,
) : SqlDriver by NativeSqliteDriver(schema, name, maxReaderConnections) {
// We can't use a mutable collection in a HashMap, as the value's hash
// will change as the collection changes, which then makes HashMap happy.
// In K/N this causes an IllegalStateException (but not on JVM)
private val listeners = mutableMapOf<String, Set<Query.Listener>>()
Is this a known issue or observed behavior? I wouldn't expect the hash of the value side to matter vs the key. I did get Example: |
|
Sorry, I missed this reply. Let me remove my workaround in Tivi and see if #4567 fixes it 🤞 @AlecKazakova any chance for a 2.1.0-alpha01 soon? |
It contains a fix for sqldelight/sqldelight#4376, so testing that out without my workaround.
It contains a fix for sqldelight/sqldelight#4376. Testing that out without my workaround to verify the fix.
|
@chrisbanes did you notice the issue go away? We just started seeing this issue too :( |
|
Yes, I'm currently using 2.1.0-SNAPSHOT and haven't seen the issue since. |
|
Looks like this is in 2.0.1? Can probably close this out? |
SQLDelight Version
2.0.0-rc02
Application Operating System
iOS
Describe the Bug
I've been seeing regular crashes in
NativeSqliteDriver. Seems to happen when there's a multiple database operations happening at the same time.I can repro this fairly easily in Tivi, but not sure how useful that is for debugging?
I'm currently running these operations on
Dispatchers.IO. I'm wondering if I should move these database writes to a more constrained thread pool.Stacktrace
The text was updated successfully, but these errors were encountered: