fix(plugins): synchronize the driver adapter's shared column type cache#1864
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A user reported random crashes with several tables open: sometimes on switching table, sometimes on reload, sometimes on returning to the app, once about 30 seconds after launch. No crash log was available.
PluginDriverAdapteris the app-side bridge every query goes through, and there is exactly one instance per connection:driver(for:)returnsactiveSessions[connectionId]?.driver, so every tab, every window and the health monitor share it.It held a bare, unsynchronized dictionary:
mapQueryResultcalls it once per column, onexecute,executeParameterizedandexecuteUserQuery. Those are reached fromQueryExecutor.fetchQueryData, which isnonisolated staticand runs on the cooperative pool. The plugins' serial queues protect the C calls, but the mapping runs after theawaitresumes, back on the concurrent pool. Two queries on one connection can be mapping columns at the same time.Concurrent
Dictionarymutation corrupts the backing store. That is undefined behavior, and it shows up exactly as reported: crashes with no reproducible step, because the damage is done long before the process notices.The reason this went unnoticed is the protocol.
PluginDatabaseDriver(plugin side) is alreadyAnyObject, Sendable, but the app-sideDatabaseDriverwas onlyAnyObject. Nothing ever required a type shared across executors to be safe to share, and strict concurrency is not enabled, so the compiler never asked.Fix
statusandcolumnTypeCachemove into aStatestruct behindOSAllocatedUnfairLock, the primitive already used elsewhere in the app. Type mapping takes the lock once per result rather than once per column.DatabaseDrivergains theSendableconstraint it was missing, so the compiler enforces this from now on.PluginDriverAdapteris now genuinelySendable, not@unchecked: every stored property is Sendable (DatabaseConnectionis all value types,ColumnType's payloads areString?/[String]?,ColumnTypeClassifieris a pure struct with astatic letlookup).The only other conformer is a test mock, which gains
@unchecked Sendable.Tests
PluginDriverAdapterConcurrencyTestsdrives the interleaving that corrupts the cache today:executeUserQuerycalls through one adapter, asserting every caller gets correctly mapped column typesexecute/executeUserQuerysharing the cachestatusreads whileconnectanddisconnectrunThese pass with the lock. Without it they race, and under Thread Sanitizer they report on
columnTypeCache.Notes
This is a verified data race that is capable of producing the reported crash, but with no
.ipsfrom the reporter I cannot prove it is the one they hit. Three further defects found during the investigation are queued as separate PRs:trackOperation, soqueriesInFlightis always emptycancelCurrentQuery()dereferences the rawMYSQL*on the caller's thread, off the connection's serial queue, whiledisconnect()nils that pointer without holdingstateLock.id(tab.id), so SwiftUI reuses oneTableViewCoordinatoracross different table tabs