Skip to content

Commit 4cfa692

Browse files
committed
Fix race condition regarding Task cancellation
This complements #1797 by making sure read-only transactions complete correctly, even if the task is cancelled.
1 parent 8dfbbb5 commit 4cfa692

File tree

5 files changed

+50
-15
lines changed

5 files changed

+50
-15
lines changed

GRDB/Core/Database.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1744,7 +1744,7 @@ public final class Database: CustomStringConvertible, CustomDebugStringConvertib
17441744
if isInsideTransaction {
17451745
try execute(sql: "ROLLBACK TRANSACTION")
17461746
}
1747-
assert(sqlite3_get_autocommit(sqliteConnection) != 0)
1747+
assert(!isInsideTransaction)
17481748
}
17491749

17501750
/// Commits a database transaction.

GRDB/Core/DatabasePool.swift

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -361,9 +361,13 @@ extension DatabasePool: DatabaseReader {
361361
return try await readerPool.get { reader in
362362
try await reader.execute { db in
363363
defer {
364-
#warning("TODO: when Task is cancelled, this commit fails, and we're still in a transaction")
365-
// Ignore commit error, but make sure we leave the transaction
366-
try? db.commit()
364+
// Commit or rollback, but make sure we leave the read-only transaction
365+
// (commit may fail with a CancellationError).
366+
do {
367+
try db.commit()
368+
} catch {
369+
try? db.rollback()
370+
}
367371
assert(!db.isInsideTransaction)
368372
}
369373
// The block isolation comes from the DEFERRED transaction.
@@ -388,8 +392,13 @@ extension DatabasePool: DatabaseReader {
388392
// Second async jump because that's how `Pool.async` has to be used.
389393
reader.async { db in
390394
defer {
391-
// Ignore commit error, but make sure we leave the transaction
392-
try? db.commit()
395+
// Commit or rollback, but make sure we leave the read-only transaction
396+
// (commit may fail with a CancellationError).
397+
do {
398+
try db.commit()
399+
} catch {
400+
try? db.rollback()
401+
}
393402
assert(!db.isInsideTransaction)
394403
releaseReader(.reuse)
395404
}
@@ -552,8 +561,13 @@ extension DatabasePool: DatabaseReader {
552561
let (reader, releaseReader) = try readerPool.get()
553562
reader.async { db in
554563
defer {
555-
// Ignore commit error, but make sure we leave the transaction
556-
try? db.commit()
564+
// Commit or rollback, but make sure we leave the read-only transaction
565+
// (commit may fail with a CancellationError).
566+
do {
567+
try db.commit()
568+
} catch {
569+
try? db.rollback()
570+
}
557571
assert(!db.isInsideTransaction)
558572
releaseReader(.reuse)
559573
}

GRDB/Core/DatabaseQueue.swift

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -248,8 +248,13 @@ extension DatabaseQueue: DatabaseReader {
248248
) {
249249
writer.async { db in
250250
defer {
251-
// Ignore commit error (we can not notify it), but make sure we leave the transaction
252-
try? db.commit()
251+
// Commit or rollback, but make sure we leave the read-only transaction
252+
// (commit may fail with a CancellationError).
253+
do {
254+
try db.commit()
255+
} catch {
256+
try? db.rollback()
257+
}
253258
assert(!db.isInsideTransaction)
254259
try? db.endReadOnly()
255260
}
@@ -297,8 +302,13 @@ extension DatabaseQueue: DatabaseReader {
297302
GRDBPrecondition(!db.isInsideTransaction, "must not be called from inside a transaction.")
298303

299304
defer {
300-
// Ignore commit error (we can not notify it), but make sure we leave the transaction
301-
try? db.commit()
305+
// Commit or rollback, but make sure we leave the read-only transaction
306+
// (commit may fail with a CancellationError).
307+
do {
308+
try db.commit()
309+
} catch {
310+
try? db.rollback()
311+
}
302312
assert(!db.isInsideTransaction)
303313
try? db.endReadOnly()
304314
}

GRDB/Core/DatabaseSnapshot.swift

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,8 +113,13 @@ public final class DatabaseSnapshot {
113113
deinit {
114114
// Leave snapshot isolation
115115
reader.reentrantSync { db in
116-
// Ignore commit error (we can not notify it), but make sure we leave the transaction
117-
try? db.commit()
116+
// Commit or rollback, but make sure we leave the read-only transaction
117+
// (commit may fail with a CancellationError).
118+
do {
119+
try db.commit()
120+
} catch {
121+
try? db.rollback()
122+
}
118123
assert(!db.isInsideTransaction)
119124
}
120125
}

GRDB/Core/WALSnapshotTransaction.swift

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,13 @@ final class WALSnapshotTransaction: @unchecked Sendable {
1515
// WALSnapshotTransaction may be deinitialized in the dispatch
1616
// queue of its reader: allow reentrancy.
1717
let isInsideTransaction = reader.reentrantSync(allowingLongLivedTransaction: false) { db in
18-
try? db.commit()
18+
// Commit or rollback, but try hard to leave the read-only transaction
19+
// (commit may fail with a CancellationError).
20+
do {
21+
try db.commit()
22+
} catch {
23+
try? db.rollback()
24+
}
1925
return db.isInsideTransaction
2026
}
2127
release(isInsideTransaction)

0 commit comments

Comments
 (0)