Skip to content

Commit 9d7a487

Browse files
authored
Merge pull request groue#1839 from groue/dev/issue-1838
Fix cancellation of async tasks that use the FTS5 full-text engine.
2 parents 24fa06d + ac2f2e8 commit 9d7a487

7 files changed

Lines changed: 178 additions & 6 deletions

File tree

GRDB.xcodeproj/project.pbxproj

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,7 @@
292292
56BB6EA91D3009B100A1CA52 /* SchedulingWatchdog.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56BB6EA81D3009B100A1CA52 /* SchedulingWatchdog.swift */; };
293293
56BCA2622E6C28F800E4F08D /* DatabaseSchemaSourceTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56BCA2612E6C28EF00E4F08D /* DatabaseSchemaSourceTests.swift */; };
294294
56BF2282241781C5003D86EB /* UtilsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56BF2281241781C5003D86EB /* UtilsTests.swift */; };
295+
56C7979B2EE99BD000AA265D /* Issue1838Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56C7979A2EE99BD000AA265D /* Issue1838Tests.swift */; };
295296
56CC922C201DFFB900CB597E /* DropWhileCursorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56CC922B201DFFB900CB597E /* DropWhileCursorTests.swift */; };
296297
56CC9243201E034D00CB597E /* PrefixWhileCursorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56CC9242201E034D00CB597E /* PrefixWhileCursorTests.swift */; };
297298
56CC9246201E058100CB597E /* DropFirstCursorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56CC9245201E058000CB597E /* DropFirstCursorTests.swift */; };
@@ -802,6 +803,7 @@
802803
56C3F7521CF9F12400F6A361 /* DatabaseSavepointTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DatabaseSavepointTests.swift; sourceTree = "<group>"; };
803804
56C48E731C9A9923005DF1D9 /* module.modulemap */ = {isa = PBXFileReference; lastKnownFileType = "sourcecode.module-map"; path = module.modulemap; sourceTree = "<group>"; };
804805
56C494401ED7255500CC72AF /* GRDBDeploymentTarget.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = GRDBDeploymentTarget.xcconfig; sourceTree = "<group>"; };
806+
56C7979A2EE99BD000AA265D /* Issue1838Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Issue1838Tests.swift; sourceTree = "<group>"; };
805807
56CC922B201DFFB900CB597E /* DropWhileCursorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DropWhileCursorTests.swift; sourceTree = "<group>"; };
806808
56CC9242201E034D00CB597E /* PrefixWhileCursorTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PrefixWhileCursorTests.swift; sourceTree = "<group>"; };
807809
56CC9245201E058000CB597E /* DropFirstCursorTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DropFirstCursorTests.swift; sourceTree = "<group>"; };
@@ -1448,6 +1450,7 @@
14481450
56B964C21DA521450002DA19 /* FTS5TableBuilderTests.swift */,
14491451
5698ACCA1DA62A2D0056AF8C /* FTS5TokenizerTests.swift */,
14501452
56ED8A7E1DAB8D6800BD0ABC /* FTS5WrapperTokenizerTests.swift */,
1453+
56C7979A2EE99BD000AA265D /* Issue1838Tests.swift */,
14511454
);
14521455
name = FTS;
14531456
sourceTree = "<group>";
@@ -2130,6 +2133,7 @@
21302133
56D496C11D81373A008276D7 /* DatabaseQueueBackupTests.swift in Sources */,
21312134
562393721DEE104400A6B01F /* MapCursorTests.swift in Sources */,
21322135
56D496571D81303E008276D7 /* FoundationDateComponentsTests.swift in Sources */,
2136+
56C7979B2EE99BD000AA265D /* Issue1838Tests.swift in Sources */,
21332137
568C3F7A2A5AB2C300A2309D /* ForeignKeyDefinitionTests.swift in Sources */,
21342138
56D496701D81309E008276D7 /* RecordPrimaryKeyRowIDTests.swift in Sources */,
21352139
5622060C1E420EB3005860AC /* DatabaseQueueConcurrencyTests.swift in Sources */,

GRDB/Core/Database+Statements.swift

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -513,6 +513,27 @@ extension Database {
513513
publicStatementArguments: configuration.publicStatementArguments)
514514
}
515515

516+
/// Always throws an error
517+
func statementCompilationDidFail(
518+
at statementStart: UnsafePointer<CChar>,
519+
withResultCode resultCode: CInt
520+
) throws -> Never {
521+
switch ResultCode(rawValue: resultCode) {
522+
case .SQLITE_INTERRUPT, .SQLITE_ABORT:
523+
// The only error that a user sees when a Task is cancelled
524+
// is CancellationError.
525+
try suspensionMutex.load().checkCancellation()
526+
default:
527+
break
528+
}
529+
530+
// Throw compilation failure
531+
throw DatabaseError(
532+
resultCode: resultCode,
533+
message: lastErrorMessage,
534+
sql: String(cString: statementStart))
535+
}
536+
516537
private func checkForAutocommitTransition() {
517538
if sqlite3_get_autocommit(sqliteConnection) == 0 {
518539
if autocommitState == .on {

GRDB/Core/Database.swift

Lines changed: 58 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -863,7 +863,16 @@ public final class Database: CustomStringConvertible, CustomDebugStringConvertib
863863
readOnlyDepth -= 1
864864
assert(readOnlyDepth >= 0, "unbalanced endReadOnly()")
865865
if readOnlyDepth == 0 {
866-
try internalCachedStatement(sql: "PRAGMA query_only = 0").execute()
866+
// We MUST ignore interruptions when we leave the read-only mode,
867+
// otherwise user could not write with this database
868+
// connection again.
869+
//
870+
// It's OK to ignore interruption, since interruption is
871+
// concurrent and we can pretend it occurred before or after
872+
// the PRAGMA.
873+
try ignoringInterruption {
874+
try internalCachedStatement(sql: "PRAGMA query_only = 0").execute()
875+
}
867876
}
868877
}
869878

@@ -1300,6 +1309,31 @@ public final class Database: CustomStringConvertible, CustomDebugStringConvertib
13001309
return try value()
13011310
}
13021311

1312+
/// Returns the result of `value`. If `value` throws an interruption
1313+
/// error, it is retried a second time.
1314+
func ignoringInterruption<T>(_ value: () throws -> T) rethrows -> T {
1315+
do {
1316+
return try value()
1317+
}
1318+
catch is CancellationError,
1319+
DatabaseError.SQLITE_INTERRUPT,
1320+
DatabaseError.SQLITE_ABORT
1321+
{
1322+
// Maybe we were unlucky, and user has interrupted the database
1323+
// during `value` execution.
1324+
//
1325+
// Another possible cause for this error is the FTS5 bug
1326+
// described at <https://sqlite.org/forum/forumpost/137c7662b3>,
1327+
// which leaves the database in a sticky interrupted state.
1328+
// To workaround this bug, we must leave the interrupted state
1329+
// before retrying:
1330+
resetAllPreparedStatements()
1331+
1332+
// Retry
1333+
return try value()
1334+
}
1335+
}
1336+
13031337
/// Support for `checkForSuspensionViolation(from:)`
13041338
private func journalMode() throws -> String {
13051339
if let journalMode = journalModeCache {
@@ -1764,7 +1798,16 @@ public final class Database: CustomStringConvertible, CustomDebugStringConvertib
17641798
// which rollback errors should be ignored, and which rollback errors
17651799
// should be exposed to the library user.
17661800
if isInsideTransaction {
1767-
try execute(sql: "ROLLBACK TRANSACTION")
1801+
// We MUST ignore interruptions during the rollback, otherwise
1802+
// we could leave a transaction open, and trigger a fatal error in
1803+
// `SerializedDatabase.preconditionNoUnsafeTransactionLeft`.
1804+
//
1805+
// It's OK to ignore interruption, since interruption is
1806+
// concurrent and we can pretend it occurred before or after
1807+
// the rollback.
1808+
try ignoringInterruption {
1809+
try execute(sql: "ROLLBACK TRANSACTION")
1810+
}
17681811
}
17691812
assert(!isInsideTransaction)
17701813
}
@@ -1779,6 +1822,19 @@ public final class Database: CustomStringConvertible, CustomDebugStringConvertib
17791822
assert(sqlite3_get_autocommit(sqliteConnection) != 0)
17801823
}
17811824

1825+
/// Resets all prepared statements.
1826+
///
1827+
/// This method helps clearing the interrupted state, as a workaround
1828+
/// for https://sqlite.org/forum/forumpost/137c7662b3, discovered
1829+
/// in https://github.com/groue/GRDB.swift/issues/1838.
1830+
func resetAllPreparedStatements() {
1831+
var stmt: SQLiteStatement? = sqlite3_next_stmt(sqliteConnection, nil)
1832+
while stmt != nil {
1833+
sqlite3_reset(stmt)
1834+
stmt = sqlite3_next_stmt(sqliteConnection, stmt)
1835+
}
1836+
}
1837+
17821838
// MARK: - Memory Management
17831839

17841840
/// Frees as much memory as possible.

GRDB/Core/Statement.swift

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -150,10 +150,7 @@ public final class Statement {
150150
&sqliteStatement, statementEnd)
151151

152152
guard code == SQLITE_OK else {
153-
throw DatabaseError(
154-
resultCode: code,
155-
message: database.lastErrorMessage,
156-
sql: String(cString: statementStart))
153+
try database.statementCompilationDidFail(at: statementStart, withResultCode: code)
157154
}
158155

159156
guard let sqliteStatement else {

GRDBCustom.xcodeproj/project.pbxproj

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,7 @@
287287
56C0539722ACEECD0029D27D /* ValueReducer.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56C0538C22ACEECD0029D27D /* ValueReducer.swift */; };
288288
56C0539B22ACEECD0029D27D /* Map.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56C0538E22ACEECD0029D27D /* Map.swift */; };
289289
56C0539D22ACEECD0029D27D /* RemoveDuplicates.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56C0538F22ACEECD0029D27D /* RemoveDuplicates.swift */; };
290+
56C7979D2EE99BE900AA265D /* Issue1838Tests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56C7979C2EE99BE900AA265D /* Issue1838Tests.swift */; };
290291
56C7A6AE1D2DFF6100EFB0C2 /* FoundationNSDateTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56F0B98E1B6001C600A2F135 /* FoundationNSDateTests.swift */; };
291292
56C9EB9B2495324700FE5D55 /* grdb_config.h in Headers */ = {isa = PBXBuildFile; fileRef = 56C9EB9A2495324700FE5D55 /* grdb_config.h */; settings = {ATTRIBUTES = (Private, ); }; };
292293
56CC9234201E009000CB597E /* DropWhileCursorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 56CC9231201DFFF600CB597E /* DropWhileCursorTests.swift */; };
@@ -814,6 +815,7 @@
814815
56C0538F22ACEECD0029D27D /* RemoveDuplicates.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = RemoveDuplicates.swift; sourceTree = "<group>"; };
815816
56C3F7521CF9F12400F6A361 /* DatabaseSavepointTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DatabaseSavepointTests.swift; sourceTree = "<group>"; };
816817
56C494421ED726C500CC72AF /* GRDBDeploymentTarget.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; name = GRDBDeploymentTarget.xcconfig; path = SQLiteCustom/GRDBDeploymentTarget.xcconfig; sourceTree = "<group>"; };
818+
56C7979C2EE99BE900AA265D /* Issue1838Tests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = Issue1838Tests.swift; sourceTree = "<group>"; };
817819
56C9EB9A2495324700FE5D55 /* grdb_config.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = grdb_config.h; path = SQLiteCustom/grdb_config.h; sourceTree = "<group>"; };
818820
56CC9231201DFFF600CB597E /* DropWhileCursorTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = DropWhileCursorTests.swift; sourceTree = "<group>"; };
819821
56CC923A201E033400CB597E /* PrefixWhileCursorTests.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = PrefixWhileCursorTests.swift; sourceTree = "<group>"; };
@@ -1441,6 +1443,7 @@
14411443
5698AC3E1DA2BEBB0056AF8C /* FTS */ = {
14421444
isa = PBXGroup;
14431445
children = (
1446+
56C7979C2EE99BE900AA265D /* Issue1838Tests.swift */,
14441447
5698AC3F1DA2BED90056AF8C /* FTS3PatternTests.swift */,
14451448
5698AC481DA2D48A0056AF8C /* FTS3RecordTests.swift */,
14461449
5698AC881DA389380056AF8C /* FTS3TableBuilderTests.swift */,
@@ -2374,6 +2377,7 @@
23742377
562205FE1E420EA2005860AC /* DatabasePoolConcurrencyTests.swift in Sources */,
23752378
565EFAF11D0436CE00A8FA9D /* NumericOverflowTests.swift in Sources */,
23762379
56DF0017228DDB8300D611F3 /* AssociationPrefetchingRowTests.swift in Sources */,
2380+
56C7979D2EE99BE900AA265D /* Issue1838Tests.swift in Sources */,
23772381
F3BA812E1CFB3064003DC1BA /* RecordPrimaryKeyMultipleTests.swift in Sources */,
23782382
F3BA81021CFB3032003DC1BA /* CGFloatTests.swift in Sources */,
23792383
5676FBAA22F5CEB9004717D9 /* ValueObservationRegionRecordingTests.swift in Sources */,

Tests/GRDBTests/DatabaseWriterTests.swift

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -896,4 +896,41 @@ class DatabaseWriterTests : GRDBTestCase {
896896
try await test(makeDatabasePool())
897897
try await test(AnyDatabaseWriter(makeDatabaseQueue()))
898898
}
899+
900+
// Regression test for https://github.com/groue/GRDB.swift/pull/1838
901+
func test_cancellation_does_not_prevent_rollback() async throws {
902+
func test(_ dbWriter: some DatabaseWriter) async throws {
903+
// Numbers that have the test fail more or less reliably
904+
// (on my machine) unless the #1838 patch is applied.
905+
// When the test fails, we have a fatal error: A transaction has been left opened at the end of a database access.
906+
let repeatCount = 400
907+
let taskCount = 50
908+
for _ in 0..<repeatCount {
909+
try await withThrowingTaskGroup(of: Void.self) { group in
910+
for _ in 0..<taskCount{
911+
let task = Task {
912+
try await dbWriter.writeWithoutTransaction { db in
913+
try db.beginTransaction()
914+
try db.rollback()
915+
}
916+
}
917+
group.addTask {
918+
Task {
919+
// For the test to fail, we need to be lucky, here:
920+
// Cancellation must occur during the compilation
921+
// or the execution of the rollback statement.
922+
task.cancel()
923+
}
924+
// Ignore expected CancellationError
925+
try? await task.value
926+
}
927+
}
928+
try await group.waitForAll()
929+
}
930+
}
931+
}
932+
try await test(makeDatabaseQueue())
933+
try await test(makeDatabasePool())
934+
try await test(AnyDatabaseWriter(makeDatabaseQueue()))
935+
}
899936
}
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
import GRDB
2+
import XCTest
3+
4+
class Issue1838Tests: GRDBTestCase {
5+
/// Regression test for <https://github.com/groue/GRDB.swift/issues/1838>.
6+
///
7+
/// This test passes since <https://github.com/groue/GRDB.swift/pull/1839>
8+
/// which workarounds the SQLite bug described at
9+
/// <https://sqlite.org/forum/forumpost/95413eb410>.
10+
func test_interrupted_rollback() throws {
11+
let dbQueue = try makeDatabaseQueue()
12+
13+
try dbQueue.inDatabase { db in
14+
try db.execute(sql: "CREATE VIRTUAL TABLE documents USING FTS5(content)")
15+
try db.inTransaction {
16+
try db.execute(sql: "INSERT INTO documents(content) VALUES ('Document')")
17+
dbQueue.interrupt()
18+
return .rollback
19+
}
20+
}
21+
22+
let documentCount = try dbQueue.read { db in
23+
try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM documents")!
24+
}
25+
XCTAssertEqual(documentCount, 0)
26+
}
27+
28+
/// Tests about how we handle the FTS5 bug <https://sqlite.org/forum/forumpost/95413eb410>
29+
func test_interrupted_commit() throws {
30+
let dbQueue = try makeDatabaseQueue()
31+
32+
try dbQueue.write { db in
33+
try db.execute(sql: "CREATE VIRTUAL TABLE documents USING FTS5(content)")
34+
}
35+
36+
do {
37+
try dbQueue.write { db in
38+
try db.execute(sql: "INSERT INTO documents(content) VALUES ('Document')")
39+
dbQueue.interrupt()
40+
}
41+
// Do not assert that the above code throws, because the
42+
// FTS5 bug might be fixed in the tested SQLite version.
43+
} catch DatabaseError.SQLITE_INTERRUPT {
44+
// Fine: That's the FTS5 bug.
45+
}
46+
47+
// Make sure the interrupted state created by the FTS5 bug is no longer active.
48+
let documentCount = try dbQueue.read { db in
49+
try Int.fetchOne(db, sql: "SELECT COUNT(*) FROM documents")!
50+
}
51+
XCTAssertEqual(documentCount, 0)
52+
}
53+
}

0 commit comments

Comments
 (0)