diff --git a/Session/Home/HomeVC.swift b/Session/Home/HomeVC.swift index aab1f1592..a573779c9 100644 --- a/Session/Home/HomeVC.swift +++ b/Session/Home/HomeVC.swift @@ -46,8 +46,8 @@ public final class HomeVC: BaseVC, LibSessionRespondingViewController, UITableVi // MARK: - UI - private var tableViewTopConstraint: NSLayoutConstraint! - private var loadingConversationsLabelTopConstraint: NSLayoutConstraint! + private var tableViewTopConstraint: NSLayoutConstraint? + private var loadingConversationsLabelTopConstraint: NSLayoutConstraint? private var navBarProfileView: ProfilePictureView? private lazy var seedReminderView: SeedReminderView = { @@ -452,8 +452,8 @@ public final class HomeVC: BaseVC, LibSessionRespondingViewController, UITableVi // Update the 'view seed' UI if updatedState.showViewedSeedBanner != self.viewModel.state.showViewedSeedBanner { - tableViewTopConstraint.isActive = false - loadingConversationsLabelTopConstraint.isActive = false + tableViewTopConstraint?.isActive = false + loadingConversationsLabelTopConstraint?.isActive = false seedReminderView.isHidden = !updatedState.showViewedSeedBanner if updatedState.showViewedSeedBanner { diff --git a/Session/Meta/AppDelegate.swift b/Session/Meta/AppDelegate.swift index 6320c9251..a670b8d60 100644 --- a/Session/Meta/AppDelegate.swift +++ b/Session/Meta/AppDelegate.swift @@ -597,7 +597,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD /// There is a warning which can happen on launch because the Database read can be blocked by another database operation /// which could result in this blocking the main thread, as a result we want to check the identity exists on a background thread /// and then return to the main thread only when required - DispatchQueue.global(qos: .default).async { [weak self, dependencies] in + DispatchQueue.global(qos: .default).asyncAfter(deadline: .now() + 0.01) { [weak self, dependencies] in guard dependencies[cache: .onboarding].state == .completed else { return } self?.enableBackgroundRefreshIfNecessary() @@ -734,7 +734,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD /// We want to start observing the changes for the 'HomeVC' and want to wait until we actually get data back before we /// continue as we don't want to show a blank home screen - DispatchQueue.global(qos: .userInitiated).async { + DispatchQueue.global(qos: .userInitiated).asyncAfter(deadline: .now() + 0.01) { viewController.startObservingChanges() { longRunningStartupTimoutCancellable.cancel() @@ -774,7 +774,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD /// On application startup the `Storage.read` can be slightly slow while GRDB spins up it's database /// read pools (up to a few seconds), since this read is blocking we want to dispatch it to run async to ensure /// we don't block user interaction while it's running - DispatchQueue.global(qos: .default).async(using: dependencies) { + DispatchQueue.global(qos: .default).asyncAfter(deadline: .now() + 0.01) { let unreadCount: Int = dependencies[singleton: .storage] .read { db in try Interaction.fetchUnreadCount(db, using: dependencies) } .defaulting(to: 0) @@ -870,7 +870,10 @@ class AppDelegate: UIResponder, UIApplicationDelegate, UNUserNotificationCenterD /// Start the pollers on a background thread so that any database queries they need to run don't /// block the main thread - DispatchQueue.global(qos: .background).async { [dependencies] in + /// + /// **Note:** We add a delay of `0.01` to prevent potential database re-entrancy if this is triggered + /// within the completion block of a database transaction, this gives it the time to complete the transaction + DispatchQueue.global(qos: .background).asyncAfter(deadline: .now() + 0.01) { [dependencies] in dependencies[singleton: .currentUserPoller].startIfNeeded() guard shouldStartGroupPollers else { return } diff --git a/Session/Meta/SessionApp.swift b/Session/Meta/SessionApp.swift index 7fcd7a8d4..4fd3a9d65 100644 --- a/Session/Meta/SessionApp.swift +++ b/Session/Meta/SessionApp.swift @@ -96,8 +96,9 @@ public class SessionApp: SessionAppType { return (SessionThread.filter(id: threadId).isNotEmpty(db), isMessageRequest) } - /// The thread should generally exist at the time of calling this method, but on the off chance it doesn't then we need to `fetchOrCreate` it and - /// should do it on a background thread just in case something is keeping the DBWrite thread busy as in the past this could cause the app to hang + /// The thread should generally exist at the time of calling this method, but on the off chance it doesn't then we need to + /// `fetchOrCreate` it and should do it on a background thread just in case something is keeping the DBWrite thread + /// busy as in the past this could cause the app to hang creatingThreadIfNeededThenRunOnMain( threadId: threadId, variant: variant, diff --git a/Session/Meta/Translations/Localizable.xcstrings b/Session/Meta/Translations/Localizable.xcstrings index c11e3b868..331eccbf4 100644 --- a/Session/Meta/Translations/Localizable.xcstrings +++ b/Session/Meta/Translations/Localizable.xcstrings @@ -304068,7 +304068,7 @@ "km" : { "stringUnit" : { "state" : "translated", - "value" : "ការបង្កើតគណនីគឺមានភ្លាមៗ, \noofសាន្ត និងមិនបង្ហាញអត្តសញ្ញាណ {emoji}" + "value" : "ការបង្កើតគណនីគឺភ្លាមៗ ឥតគិតថ្លៃ និងអនាមិក {emoji}" } }, "kn" : { diff --git a/SessionMessagingKit/Jobs/GarbageCollectionJob.swift b/SessionMessagingKit/Jobs/GarbageCollectionJob.swift index 4cae9b272..19f23f432 100644 --- a/SessionMessagingKit/Jobs/GarbageCollectionJob.swift +++ b/SessionMessagingKit/Jobs/GarbageCollectionJob.swift @@ -354,8 +354,11 @@ public enum GarbageCollectionJob: JobExecutor { } }, completion: { _, _ in - // Dispatch async so we can swap from the write queue to a read one (we are done writing) - queue.async { + // Dispatch async so we can swap from the write queue to a read one (we are done + // writing), this has to be done after a slight delay to ensure the transaction + // provided by the completion block completes first (ie. so we don't hit + // re-entrancy issues) + queue.asyncAfter(deadline: .now() + 0.01) { // Retrieve a list of all valid attachmnet and avatar file paths struct FileInfo { let attachmentLocalRelativePaths: Set diff --git a/SessionMessagingKit/Jobs/ProcessPendingGroupMemberRemovalsJob.swift b/SessionMessagingKit/Jobs/ProcessPendingGroupMemberRemovalsJob.swift index 216663a7b..cbe18eb47 100644 --- a/SessionMessagingKit/Jobs/ProcessPendingGroupMemberRemovalsJob.swift +++ b/SessionMessagingKit/Jobs/ProcessPendingGroupMemberRemovalsJob.swift @@ -273,7 +273,7 @@ public enum ProcessPendingGroupMemberRemovalsJob: JobExecutor { } }, completion: { db, result in - queue.async(using: dependencies) { + queue.asyncAfter(deadline: .now() + 0.01, using: dependencies) { switch result { case .success: success(job, false) case .failure(let error): failure(job, error, false) diff --git a/SessionMessagingKit/Sending & Receiving/MessageSender.swift b/SessionMessagingKit/Sending & Receiving/MessageSender.swift index 3cd59ff7f..d5a54c595 100644 --- a/SessionMessagingKit/Sending & Receiving/MessageSender.swift +++ b/SessionMessagingKit/Sending & Receiving/MessageSender.swift @@ -775,9 +775,10 @@ public final class MessageSender { guard !rowIds.isEmpty else { return error } - // Need to dispatch to a different thread as this function is most commonly called within a read - // thread and we want to write to the db and don't want to run into a re-entrancy error - DispatchQueue.global(qos: .background).async(using: dependencies) { + // Note: We need to dispatch this after a small 0.01 delay to prevent any potential + // re-entrancy issues since the 'asyncMigrate' returns a result containing a DB instance + // within a transaction + DispatchQueue.global(qos: .background).asyncAfter(deadline: .now() + 0.01, using: dependencies) { dependencies[singleton: .storage].write { db in switch destination { case .syncMessage: diff --git a/SessionSnodeKit/LibSession/LibSession+Networking.swift b/SessionSnodeKit/LibSession/LibSession+Networking.swift index 83cb58b21..8da9bf955 100644 --- a/SessionSnodeKit/LibSession/LibSession+Networking.swift +++ b/SessionSnodeKit/LibSession/LibSession+Networking.swift @@ -414,9 +414,12 @@ private extension LibSessionNetwork { return Log.error(.network, "CallbackWrapper called with null context.") } - // Dispatch async so we don't block libSession's internals with Swift logic (which can block other requests) + /// Dispatch async so we don't block libSession's internals with Swift logic (which can block other requests), we + /// add the `0.01` delay to ensure the closure isn't executed immediately let wrapper: CallbackWrapper = Unmanaged>.fromOpaque(ctx).takeRetainedValue() - DispatchQueue.global(qos: .default).async { [wrapper] in wrapper.resultPublisher.send(output) } + DispatchQueue.global(qos: .default).asyncAfter(deadline: .now() + 0.01) { [wrapper] in + wrapper.resultPublisher.send(output) + } } public func unsafePointer() -> UnsafeMutableRawPointer { Unmanaged.passRetained(self).toOpaque() } diff --git a/SessionUtilitiesKit/Database/Storage.swift b/SessionUtilitiesKit/Database/Storage.swift index 5cc5a1f73..c1deeb612 100644 --- a/SessionUtilitiesKit/Database/Storage.swift +++ b/SessionUtilitiesKit/Database/Storage.swift @@ -386,9 +386,10 @@ open class Storage { } }() - // Note: We need to dispatch this to the next run toop to prevent any potential re-entrancy - // issues since the 'asyncMigrate' returns a result containing a DB instance - DispatchQueue.global(qos: .userInitiated).async { + // Note: We need to dispatch this after a small 0.01 delay to prevent any potential + // re-entrancy issues since the 'asyncMigrate' returns a result containing a DB instance + // within a transaction + DispatchQueue.global(qos: .userInitiated).asyncAfter(deadline: .now() + 0.01, using: dependencies) { migrationCompleted(finalResult) } } @@ -592,8 +593,8 @@ open class Storage { case valid(DatabaseWriter) case invalid(Error) - init(_ storage: Storage) { - switch (storage.isSuspended, storage.isValid, storage.dbWriter) { + init(_ storage: Storage?) { + switch (storage?.isSuspended, storage?.isValid, storage?.dbWriter) { case (true, _, _): self = .invalid(StorageError.databaseSuspended) case (false, true, .some(let dbWriter)): self = .valid(dbWriter) default: self = .invalid(StorageError.databaseInvalid) @@ -697,7 +698,7 @@ open class Storage { ) -> AnyPublisher { switch StorageState(self) { case .invalid(let error): return StorageState.logIfNeeded(error, isWrite: true) - case .valid(let dbWriter): + case .valid: /// **Note:** GRDB does have a `writePublisher` method but it appears to asynchronously trigger /// both the `output` and `complete` closures at the same time which causes a lot of unexpected /// behaviours (this behaviour is apparently expected but still causes a number of odd behaviours in our code @@ -707,11 +708,19 @@ open class Storage { /// which behaves in a much more expected way than the GRDB `writePublisher` does let info: CallInfo = CallInfo(fileName, functionName, lineNumber, true, self) return Deferred { - Future { resolver in - do { resolver(Result.success(try dbWriter.write(Storage.perform(info: info, updates: updates)))) } - catch { - StorageState.logIfNeeded(error, isWrite: true) - resolver(Result.failure(error)) + Future { [weak self] resolver in + /// The `StorageState` may have changed between the creation of the publisher and it actually + /// being executed so we need to check again + switch StorageState(self) { + case .invalid(let error): return StorageState.logIfNeeded(error, isWrite: true) + case .valid(let dbWriter): + do { + resolver(Result.success(try dbWriter.write(Storage.perform(info: info, updates: updates)))) + } + catch { + StorageState.logIfNeeded(error, isWrite: true) + resolver(Result.failure(error)) + } } } }.eraseToAnyPublisher() @@ -741,7 +750,7 @@ open class Storage { ) -> AnyPublisher { switch StorageState(self) { case .invalid(let error): return StorageState.logIfNeeded(error, isWrite: false) - case .valid(let dbWriter): + case .valid: /// **Note:** GRDB does have a `readPublisher` method but it appears to asynchronously trigger /// both the `output` and `complete` closures at the same time which causes a lot of unexpected /// behaviours (this behaviour is apparently expected but still causes a number of odd behaviours in our code @@ -751,11 +760,19 @@ open class Storage { /// which behaves in a much more expected way than the GRDB `readPublisher` does let info: CallInfo = CallInfo(fileName, functionName, lineNumber, false, self) return Deferred { - Future { resolver in - do { resolver(Result.success(try dbWriter.read(Storage.perform(info: info, updates: value)))) } - catch { - StorageState.logIfNeeded(error, isWrite: false) - resolver(Result.failure(error)) + Future { [weak self] resolver in + /// The `StorageState` may have changed between the creation of the publisher and it actually + /// being executed so we need to check again + switch StorageState(self) { + case .invalid(let error): return StorageState.logIfNeeded(error, isWrite: false) + case .valid(let dbWriter): + do { + resolver(Result.success(try dbWriter.read(Storage.perform(info: info, updates: value)))) + } + catch { + StorageState.logIfNeeded(error, isWrite: false) + resolver(Result.failure(error)) + } } } }.eraseToAnyPublisher() diff --git a/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift b/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift index cc0f2da73..25686a987 100644 --- a/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift +++ b/SessionUtilitiesKit/Database/Types/PagedDatabaseObserver.swift @@ -164,19 +164,21 @@ public class PagedDatabaseObserver: TransactionObserver where _changesInCommit.performUpdate { $0.inserting(trackedChange) } } - /// We will process all updates which come through this method even if 'onChange' is null because if the UI stops observing and then starts - /// again later we don't want to have missed any changes which happened while the UI wasn't subscribed (and doing a full re-query seems painful...) + /// We will process all updates which come through this method even if 'onChange' is null because if the UI stops observing and + /// then starts again later we don't want to have missed any changes which happened while the UI wasn't subscribed (and doing + /// a full re-query seems painful...) /// - /// **Note:** This function is generally called within the DBWrite thread but we don't actually need write access to process the commit, in order - /// to avoid blocking the DBWrite thread we dispatch to a serial `commitProcessingQueue` to process the incoming changes (in the past not doing - /// so was resulting in hanging when there was a lot of activity happening) + /// **Note:** This function is generally called within the DBWrite thread but we don't actually need write access to process the + /// commit, in order to avoid blocking the DBWrite thread we dispatch to a serial `commitProcessingQueue` to process the + /// incoming changes (in the past not doing so was resulting in hanging when there was a lot of activity happening) public func databaseDidCommit(_ db: Database) { // If there were no pending changes in the commit then do nothing guard !self.changesInCommit.isEmpty else { return } - // Since we can't be sure the behaviours of 'databaseDidChange' and 'databaseDidCommit' won't change in - // the future we extract and clear the values in 'changesInCommit' since it's '@ThreadSafe' so will different - // threads modifying the data resulting in us missing a change + // Since we can't be sure the behaviours of 'databaseDidChange' and 'databaseDidCommit' + // won't change in the future we extract and clear the values in 'changesInCommit' since + // it's '@ThreadSafe' so will different threads modifying the data resulting in us + // missing a change var committedChanges: Set = [] self._changesInCommit.performUpdate { cachedChanges in @@ -184,7 +186,12 @@ public class PagedDatabaseObserver: TransactionObserver where return [] } - commitProcessingQueue.async { [weak self] in + // This looks odd but if we just use `commitProcessingQueue.async` then the code can + // get executed immediately wihch can result in a new transaction being started whilst + // we are still within the transaction wrapping `databaseDidCommit` (which we don't + // want), by adding this tiny 0.01 delay we should be giving it enough time to finish + // processing the current transaction + commitProcessingQueue.asyncAfter(deadline: .now() + 0.01) { [weak self] in self?.processDatabaseCommit(committedChanges: committedChanges) } } diff --git a/SessionUtilitiesKit/JobRunner/JobRunner.swift b/SessionUtilitiesKit/JobRunner/JobRunner.swift index f89734bfb..a8e0077eb 100644 --- a/SessionUtilitiesKit/JobRunner/JobRunner.swift +++ b/SessionUtilitiesKit/JobRunner/JobRunner.swift @@ -734,7 +734,9 @@ public final class JobRunner: JobRunnerType { // Don't add to the queue if the JobRunner isn't ready (it's been saved to the db so it'll be loaded // once the queue actually get started later) - guard canAddToQueue(updatedJob) || jobQueue?.isRunningInBackgroundTask == true else { return updatedJob } + guard canAddToQueue(updatedJob) || jobQueue?.isRunningInBackgroundTask == true else { + return updatedJob + } // The queue is ready or running in a background task so we can add the job jobQueue?.add(db, job: updatedJob, canStartJob: canStartJob) @@ -1337,10 +1339,14 @@ public final class JobQueue: Hashable { guard canStart?(self) == true || isRunningInBackgroundTask else { return } guard forceWhenAlreadyRunning || !isRunning || isRunningInBackgroundTask else { return } - // The JobRunner runs synchronously we need to ensure this doesn't start - // on the main thread (if it is on the main thread then swap to a different thread) + // The JobRunner runs synchronously so we need to ensure this doesn't start on the main + // thread and do so by creating a number of background queues to run the jobs on, if this + // function was called on the wrong queue then we need to dispatch to the correct one guard DispatchQueue.with(key: queueKey, matches: queueContext, using: dependencies) else { - internalQueue.async(using: dependencies) { [weak self] in + // Note: We need to dispatch this after a small 0.01 delay to prevent any potential + // re-entrancy issues since the `start` function can be called within an existing + // database transaction (eg. via `db.afterNextTransactionNestedOnce`) + internalQueue.asyncAfter(deadline: .now() + 0.01, using: dependencies) { [weak self] in self?.start(forceWhenAlreadyRunning: forceWhenAlreadyRunning) } return diff --git a/SessionUtilitiesKit/Utilities/ThreadSafe.swift b/SessionUtilitiesKit/Utilities/ThreadSafe.swift index 3d15c6329..d53c9f34e 100644 --- a/SessionUtilitiesKit/Utilities/ThreadSafe.swift +++ b/SessionUtilitiesKit/Utilities/ThreadSafe.swift @@ -18,7 +18,7 @@ import Foundation /// as `ThreadSafeType` (reference types or structs which have `mutating` functions **should not** use this mechanism /// as it cannot ensure thread safety for those types) @propertyWrapper -public struct ThreadSafe { +public class ThreadSafe { private var value: Value private let lock: ReadWriteLock = ReadWriteLock() @@ -49,17 +49,17 @@ public struct ThreadSafe { // MARK: - Functions - public mutating func performUpdateAndMap(_ closure: (Value) -> (Value, T)) -> T { + public func performUpdateAndMap(_ closure: (Value) -> (Value, T)) -> T { return try! performInternal { closure($0) } } - public mutating func performUpdateAndMap(_ closure: (Value) throws -> (Value, T)) throws -> T { + public func performUpdateAndMap(_ closure: (Value) throws -> (Value, T)) throws -> T { return try performInternal { try closure($0) } } // MARK: - Internal Functions - @discardableResult private mutating func performInternal(_ mutation: (Value) throws -> (Value, T)) throws -> T { + @discardableResult private func performInternal(_ mutation: (Value) throws -> (Value, T)) throws -> T { lock.writeLock() defer { lock.unlock() }