Merge pull request groue#1213 from groue/dev/fix-1209

Fix crash when the number of active ValueObservations is high

Author: groue

CHANGELOG.md

@@ -90,6 +90,11 @@ GRDB adheres to [Semantic Versioning](https://semver.org/), with one exception:
 
 ---
 
+## Next Release
+
+- **Fixed**: [#1213](https://github.com/groue/GRDB.swift/pull/1213) by [@groue](https://github.com/groue): Fix crash when the number of active ValueObservations is high
+- **Breaking Change**: Transactions performed during a read-only database access are no longer notified to transaction observers. This is, strictly speaking, a breaking change. However it should have no impact since read-only transactions have very little interest.
+
 ## 5.23.0
 
 Released April 16, 2022 • [diff](https://github.com/groue/GRDB.swift/compare/v5.22.2...v5.23.0)

GRDB/Core/Database.swift

@@ -233,7 +233,9 @@ public final class Database: CustomStringConvertible, CustomDebugStringConvertib
         setupDefaultFunctions()
         setupDefaultCollations()
         setupAuthorizer()
-        observationBroker.installCommitAndRollbackHooks()
+        if !configuration.readonly {
+            observationBroker.installCommitAndRollbackHooks()
+        }
         try activateExtendedCodes()
 
         #if SQLITE_HAS_CODEC
@@ -546,6 +548,11 @@ public final class Database: CustomStringConvertible, CustomDebugStringConvertib
             finally: endReadOnly)
     }
 
+    /// Returns whether database connection is read-only.
+    var isReadOnly: Bool {
+        _readOnlyDepth > 0 || configuration.readonly
+    }
+    
     // MARK: - Snapshots
 
     #if SQLITE_ENABLE_SNAPSHOT
@@ -939,10 +946,16 @@ public final class Database: CustomStringConvertible, CustomDebugStringConvertib
     /// This method is not reentrant: you can't nest transactions.
     ///
     /// - parameters:
-    ///     - kind: The transaction type (default nil). If nil, the transaction
-    ///       type is configuration.defaultTransactionKind, which itself
-    ///       defaults to .deferred. See <https://www.sqlite.org/lang_transaction.html>
-    ///       for more information.
+    ///     - kind: The transaction type (default nil).
+    ///
+    ///       If nil, and the database connection is read-only, the transaction
+    ///       kind is `.deferred`.
+    ///
+    ///       If nil, and the database connection is not read-only, the
+    ///       transaction kind is `configuration.defaultTransactionKind`.
+    ///
+    ///       See <https://www.sqlite.org/lang_transaction.html> for
+    ///       more information.
     ///     - block: A block that executes SQL statements and return either
     ///       .commit or .rollback.
     /// - throws: The error thrown by the block.
@@ -1005,13 +1018,20 @@ public final class Database: CustomStringConvertible, CustomDebugStringConvertib
     /// - parameter readOnly: If true, writes are forbidden.
     func isolated<T>(readOnly: Bool = false, _ block: () throws -> T) throws -> T {
         var result: T?
-        try inSavepoint {
-            if readOnly {
-                result = try self.readOnly(block)
-            } else {
+        if readOnly {
+            // Enter read-only mode before starting a transaction, so that the
+            // transaction commit does not trigger database observation.
+            try self.readOnly {
+                try inSavepoint {
+                    result = try block()
+                    return .commit
+                }
+            }
+        } else {
+            try inSavepoint {
                 result = try block()
+                return .commit
             }
-            return .commit
         }
         return result!
     }
@@ -1055,7 +1075,7 @@ public final class Database: CustomStringConvertible, CustomDebugStringConvertib
             //
             // For those two reasons, we open a transaction instead of a
             // top-level savepoint.
-            try inTransaction(configuration.defaultTransactionKind, block)
+            try inTransaction { try block() }
             return
         }
 
@@ -1123,13 +1143,20 @@ public final class Database: CustomStringConvertible, CustomDebugStringConvertib
 
     /// Begins a database transaction.
     ///
-    /// - parameter kind: The transaction type (default nil). If nil, the
-    ///   transaction type is configuration.defaultTransactionKind, which itself
-    ///   defaults to .deferred. See <https://www.sqlite.org/lang_transaction.html>
-    ///   for more information.
-    /// - throws: The error thrown by the block.
+    /// - parameter kind: The transaction type (default nil).
+    ///
+    ///   If nil, and the database connection is read-only, the transaction kind
+    ///   is `.deferred`.
+    ///
+    ///   If nil, and the database connection is not read-only, the transaction
+    ///   kind is `configuration.defaultTransactionKind`.
+    ///
+    ///   See <https://www.sqlite.org/lang_transaction.html> for
+    ///   more information.
+    /// - throws: A DatabaseError whenever an SQLite error occurs.
     public func beginTransaction(_ kind: TransactionKind? = nil) throws {
-        let kind = kind ?? configuration.defaultTransactionKind
+        // SQLite throws an error for non-deferred transactions when read-only.
+        let kind = kind ?? (isReadOnly ? .deferred : configuration.defaultTransactionKind)
         try execute(sql: "BEGIN \(kind.rawValue) TRANSACTION")
         assert(sqlite3_get_autocommit(sqliteConnection) == 0)
     }

GRDB/Core/TransactionObserver.swift

@@ -220,8 +220,7 @@ class DatabaseObservationBroker {
 
     /// Prepares observation of changes that are about to be performed by the statement.
     func statementWillExecute(_ statement: Statement) {
-        // If any observer observes row deletions, we'll have to disable
-        if transactionObservations.isEmpty == false {
+        if !database.isReadOnly && !transactionObservations.isEmpty {
             // As statement executes, it may trigger database changes that will
             // be notified to transaction observers. As a consequence, observers
             // may disable themselves with stopObservingDatabaseChangesUntilNextTransaction()
@@ -310,7 +309,7 @@ class DatabaseObservationBroker {
             // SQLITE_CONSTRAINT error)
             databaseDidRollback(notifyTransactionObservers: false)
         case .cancelledCommit(let error):
-            databaseDidRollback(notifyTransactionObservers: true)
+            databaseDidRollback(notifyTransactionObservers: !database.isReadOnly)
             throw error
         default:
             break
@@ -382,7 +381,7 @@ class DatabaseObservationBroker {
         case .commit:
             databaseDidCommit()
         case .rollback:
-            databaseDidRollback(notifyTransactionObservers: true)
+            databaseDidRollback(notifyTransactionObservers: !database.isReadOnly)
         default:
             break
         }
@@ -391,6 +390,8 @@ class DatabaseObservationBroker {
     #if SQLITE_ENABLE_PREUPDATE_HOOK
     // Called from sqlite3_preupdate_hook
     private func databaseWillChange(with event: DatabasePreUpdateEvent) {
+        assert(!database.isReadOnly, "Read-only transactions are not notified")
+        
         if savepointStack.isEmpty {
             // Notify now
             for (observation, predicate) in statementObservations where predicate.evaluate(event) {
@@ -405,6 +406,8 @@ class DatabaseObservationBroker {
 
     // Called from sqlite3_update_hook
     private func databaseDidChange(with event: DatabaseEvent) {
+        assert(!database.isReadOnly, "Read-only transactions are not notified")
+        
         // We're about to call the databaseDidChange(with:) method of
         // transaction observers. In this method, observers may disable
         // themselves with stopObservingDatabaseChangesUntilNextTransaction()
@@ -430,18 +433,23 @@ class DatabaseObservationBroker {
     // Called from sqlite3_commit_hook and databaseDidCommitEmptyDeferredTransaction()
     private func databaseWillCommit() throws {
         notifyBufferedEvents()
-        for observation in transactionObservations {
-            try observation.databaseWillCommit()
+        if !database.isReadOnly {
+            for observation in transactionObservations {
+                try observation.databaseWillCommit()
+            }
         }
     }
 
     // Called from statementDidExecute
     private func databaseDidCommit() {
         savepointStack.clear()
 
-        for observation in transactionObservations {
-            observation.databaseDidCommit(database)
+        if !database.isReadOnly {
+            for observation in transactionObservations {
+                observation.databaseDidCommit(database)
+            }
         }
+        
         databaseDidEndTransaction()
     }
 
@@ -485,7 +493,7 @@ class DatabaseObservationBroker {
             try databaseWillCommit()
             databaseDidCommit()
         } catch {
-            databaseDidRollback(notifyTransactionObservers: true)
+            databaseDidRollback(notifyTransactionObservers: !database.isReadOnly)
             throw error
         }
     }
@@ -495,6 +503,7 @@ class DatabaseObservationBroker {
         savepointStack.clear()
 
         if notifyTransactionObservers {
+            assert(!database.isReadOnly, "Read-only transactions are not notified")
             for observation in transactionObservations {
                 observation.databaseDidRollback(database)
             }
@@ -566,6 +575,7 @@ class DatabaseObservationBroker {
         savepointStack.clear()
 
         for (event, statementObservations) in eventsBuffer {
+            assert(statementObservations.isEmpty || !database.isReadOnly, "Read-only transactions are not notified")
             for (observation, predicate) in statementObservations where predicate.evaluate(event) {
                 event.send(to: observation)
             }

README.md

@@ -6683,6 +6683,8 @@ By default, database holds weak references to its transaction observers: they ar
 **A transaction observer is notified of all database changes**: inserts, updates and deletes. This includes indirect changes triggered by ON DELETE and ON UPDATE actions associated to [foreign keys](https://www.sqlite.org/foreignkeys.html#fk_actions), and [SQL triggers](https://www.sqlite.org/lang_createtrigger.html).
 
 > :point_up: **Note**: Some changes are not notified: changes to internal system tables (such as `sqlite_master`), changes to [`WITHOUT ROWID`](https://www.sqlite.org/withoutrowid.html) tables, and the deletion of duplicate rows triggered by [`ON CONFLICT REPLACE`](https://www.sqlite.org/lang_conflict.html) clauses (this last exception might change in a future release of SQLite).
+>
+> :point_up:  **Note**: Transactions performed during read-only database accesses are not notified.
 
 Notified changes are not actually written to disk until the [transaction](#transactions-and-savepoints) commits, and the `databaseDidCommit` callback is called. On the other side, `databaseDidRollback` confirms their invalidation:
 
@@ -6794,7 +6796,7 @@ class PlayerScoreObserver: TransactionObserver {
 }
 ```
 
-When the `observes(eventsOfKind:)` method returns false for all event kinds, the observer is still notified of commits and rollbacks:
+When the `observes(eventsOfKind:)` method returns false for all event kinds, the observer is still notified of commits and rollbacks (except during read-only database accesses):
 
 ```swift
 class PureTransactionObserver: TransactionObserver {

Tests/GRDBTests/DatabaseTests.swift

@@ -566,6 +566,36 @@ class DatabaseTests : GRDBTestCase {
         }
     }
 
+    func testReadOnlyTransaction() throws {
+        // query_only pragma was added in SQLite 3.8.0 http://www.sqlite.org/changes.html#version_3_8_0
+        guard sqlite3_libversion_number() >= 3008000 else {
+            return
+        }
+        
+        dbConfiguration.defaultTransactionKind = .immediate
+        let dbQueue = try makeDatabaseQueue()
+        try dbQueue.inDatabase { db in
+            do {
+                sqlQueries.removeAll()
+                try db.inSavepoint { .commit }
+                try db.inTransaction { .commit }
+                try db.inTransaction(.immediate) { .commit }
+                XCTAssertEqual(Set(sqlQueries), ["BEGIN IMMEDIATE TRANSACTION", "COMMIT TRANSACTION"])
+            }
+            
+            try db.readOnly {
+                sqlQueries.removeAll()
+                try db.inSavepoint { .commit }
+                try db.inTransaction { .commit }
+                XCTAssertEqual(Set(sqlQueries), ["BEGIN DEFERRED TRANSACTION", "COMMIT TRANSACTION"])
+                do {
+                    try db.inTransaction(.immediate) { .commit }
+                    XCTFail("Expected error")
+                } catch DatabaseError.SQLITE_READONLY { }
+            }
+        }
+    }
+    
     func testCheckpoint() throws {
         do {
             // Not a WAL database

Tests/GRDBTests/TransactionObserverTests.swift

@@ -1,5 +1,5 @@
 import XCTest
-import GRDB
+@testable import GRDB
 
 private class Observer : TransactionObserver {
     var lastCommittedEvents: [DatabaseEvent] = []
@@ -1784,4 +1784,89 @@ class TransactionObserverTests: GRDBTestCase {
             }
         }
     }
+    
+    // MARK: - Read-Only Connection
+    
+    func testReadOnlyConnection() throws {
+        let dbQueue = try makeDatabaseQueue(filename: "database.sqlite")
+        try setupArtistDatabase(in: dbQueue)
+        
+        dbConfiguration.readonly = true
+        let readOnlyQueue = try makeDatabaseQueue(filename: "database.sqlite")
+        
+        let observer = Observer()
+        readOnlyQueue.add(transactionObserver: observer, extent: .databaseLifetime)
+        
+        try readOnlyQueue.inDatabase { db in
+            do {
+                try db.execute(sql: """
+                    BEGIN;
+                    COMMIT;
+                    """)
+                XCTAssertEqual(observer.didChangeCount, 0)
+                XCTAssertEqual(observer.willCommitCount, 0)
+                XCTAssertEqual(observer.didCommitCount, 0)
+                XCTAssertEqual(observer.didRollbackCount, 0)
+                #if SQLITE_ENABLE_PREUPDATE_HOOK
+                XCTAssertEqual(observer.willChangeCount, 0)
+                #endif
+            }
+            
+            do {
+                try db.execute(sql: """
+                    BEGIN;
+                    SELECT * FROM artists;
+                    COMMIT;
+                    """)
+                XCTAssertEqual(observer.didChangeCount, 0)
+                XCTAssertEqual(observer.willCommitCount, 0)
+                XCTAssertEqual(observer.didCommitCount, 0)
+                XCTAssertEqual(observer.didRollbackCount, 0)
+                #if SQLITE_ENABLE_PREUPDATE_HOOK
+                XCTAssertEqual(observer.willChangeCount, 0)
+                #endif
+            }
+        }
+    }
+    
+    func testReadOnlyBlock() throws {
+        let dbQueue = try makeDatabaseQueue()
+        try setupArtistDatabase(in: dbQueue)
+        
+        let observer = Observer()
+        dbQueue.add(transactionObserver: observer, extent: .databaseLifetime)
+        
+        try dbQueue.inDatabase { db in
+            try db.readOnly {
+                do {
+                    try db.execute(sql: """
+                        BEGIN;
+                        COMMIT;
+                        """)
+                    XCTAssertEqual(observer.didChangeCount, 0)
+                    XCTAssertEqual(observer.willCommitCount, 0)
+                    XCTAssertEqual(observer.didCommitCount, 0)
+                    XCTAssertEqual(observer.didRollbackCount, 0)
+                    #if SQLITE_ENABLE_PREUPDATE_HOOK
+                    XCTAssertEqual(observer.willChangeCount, 0)
+                    #endif
+                }
+                
+                do {
+                    try db.execute(sql: """
+                        BEGIN;
+                        SELECT * FROM artists;
+                        COMMIT;
+                        """)
+                    XCTAssertEqual(observer.didChangeCount, 0)
+                    XCTAssertEqual(observer.willCommitCount, 0)
+                    XCTAssertEqual(observer.didCommitCount, 0)
+                    XCTAssertEqual(observer.didRollbackCount, 0)
+                    #if SQLITE_ENABLE_PREUPDATE_HOOK
+                    XCTAssertEqual(observer.willChangeCount, 0)
+                    #endif
+                }
+            }
+        }
+    }
 }