From e3642b92d137f2e175e4c9011c077ee2a7d934f9 Mon Sep 17 00:00:00 2001 From: William Casarin Date: Fri, 26 Jan 2024 14:03:49 -0800 Subject: [PATCH] txn: fix subtle transaction inheritence bugs This fixes subtle bugs with transaction inheritence. Since we were not passing the inherited state to moved value, we were sometimes committing transactions more than once. Changelog-Fixed: Fix many nostrdb transaction related crashes --- damus/Models/NoteContent.swift | 2 +- damus/Models/NotificationsManager.swift | 2 +- damus/Nostr/Profiles.swift | 4 +-- damus/Views/Profile/ProfilePicView.swift | 2 +- damus/Views/QRCodeView.swift | 7 ++-- damus/Views/SideMenuView.swift | 4 +-- nostrdb/Ndb.swift | 28 ++++++--------- nostrdb/NdbTxn.swift | 46 ++++++++++++++++-------- 8 files changed, 53 insertions(+), 42 deletions(-) diff --git a/damus/Models/NoteContent.swift b/damus/Models/NoteContent.swift index f0cecb19..81adde7a 100644 --- a/damus/Models/NoteContent.swift +++ b/damus/Models/NoteContent.swift @@ -183,7 +183,7 @@ func attributed_string_attach_icon(_ astr: inout AttributedString, img: UIImage) } func getDisplayName(pk: Pubkey, profiles: Profiles) -> String { - let profile_txn = profiles.lookup(id: pk) + let profile_txn = profiles.lookup(id: pk, txn_name: "getDisplayName") let profile = profile_txn?.unsafeUnownedValue return Profile.displayName(profile: profile, pubkey: pk).username.truncate(maxLength: 50) } diff --git a/damus/Models/NotificationsManager.swift b/damus/Models/NotificationsManager.swift index 6a832362..44a4aee1 100644 --- a/damus/Models/NotificationsManager.swift +++ b/damus/Models/NotificationsManager.swift @@ -77,7 +77,7 @@ func generate_local_notification_object(from ev: NostrEvent, state: HeadlessDamu } else if type == .like, state.settings.like_notification, let evid = ev.referenced_ids.last, - let txn = state.ndb.lookup_note(evid), + let txn = state.ndb.lookup_note(evid, txn_name: "local_notification_like"), let liked_event = txn.unsafeUnownedValue?.to_owned() { let content_preview = render_notification_content_preview(ev: liked_event, profiles: state.profiles, keypair: state.keypair) diff --git a/damus/Nostr/Profiles.swift b/damus/Nostr/Profiles.swift index 71c5327b..2413f31d 100644 --- a/damus/Nostr/Profiles.swift +++ b/damus/Nostr/Profiles.swift @@ -85,8 +85,8 @@ class Profiles { ndb.search_profile(query, limit: limit, txn: txn) } - func lookup(id: Pubkey) -> NdbTxn? { - guard let txn = ndb.lookup_profile(id) else { + func lookup(id: Pubkey, txn_name: String? = nil) -> NdbTxn? { + guard let txn = ndb.lookup_profile(id, txn_name: txn_name) else { return nil } return txn.map({ pr in pr?.profile }) diff --git a/damus/Views/Profile/ProfilePicView.swift b/damus/Views/Profile/ProfilePicView.swift index 20efd043..4681efed 100644 --- a/damus/Views/Profile/ProfilePicView.swift +++ b/damus/Views/Profile/ProfilePicView.swift @@ -126,7 +126,7 @@ struct ProfilePicView: View { } func get_profile_url(picture: String?, pubkey: Pubkey, profiles: Profiles) -> URL { - let pic = picture ?? profiles.lookup(id: pubkey)?.map({ $0?.picture }).value ?? robohash(pubkey) + let pic = picture ?? profiles.lookup(id: pubkey, txn_name: "get_profile_url")?.map({ $0?.picture }).value ?? robohash(pubkey) if let url = URL(string: pic) { return url } diff --git a/damus/Views/QRCodeView.swift b/damus/Views/QRCodeView.swift index 772b3756..7010d78e 100644 --- a/damus/Views/QRCodeView.swift +++ b/damus/Views/QRCodeView.swift @@ -118,10 +118,11 @@ struct QRCodeView: View { var QRView: some View { VStack(alignment: .center) { - let profile_txn = damus_state.profiles.lookup(id: pubkey) + let profile_txn = damus_state.profiles.lookup(id: pubkey, txn_name: "qrview-profile") let profile = profile_txn?.unsafeUnownedValue - let our_profile_txn = damus_state.profiles.lookup(id: damus_state.pubkey) - let our_profile = our_profile_txn?.unsafeUnownedValue + let our_profile = profile_txn.flatMap({ ptxn in + damus_state.ndb.lookup_profile_with_txn(damus_state.pubkey, txn: ptxn)?.profile + }) if our_profile?.picture != nil { ProfilePicView(pubkey: pubkey, size: 90.0, highlight: .custom(DamusColors.white, 3.0), profiles: damus_state.profiles, disable_animation: damus_state.settings.disable_animation) diff --git a/damus/Views/SideMenuView.swift b/damus/Views/SideMenuView.swift index 8a0e3cdd..dfc04313 100644 --- a/damus/Views/SideMenuView.swift +++ b/damus/Views/SideMenuView.swift @@ -89,8 +89,8 @@ struct SideMenuView: View { } var TopProfile: some View { - let profile_txn = damus_state.profiles.lookup(id: damus_state.pubkey) - let profile = profile_txn?.unsafeUnownedValue + let profile_txn = damus_state.ndb.lookup_profile(damus_state.pubkey, txn_name: "top_profile") + let profile = profile_txn?.unsafeUnownedValue?.profile return VStack(alignment: .leading, spacing: verticalSpacing) { HStack { ProfilePicView(pubkey: damus_state.pubkey, size: 60, highlight: .none, profiles: damus_state.profiles, disable_animation: damus_state.settings.disable_animation) diff --git a/nostrdb/Ndb.swift b/nostrdb/Ndb.swift index f366f700..b618c9d9 100644 --- a/nostrdb/Ndb.swift +++ b/nostrdb/Ndb.swift @@ -76,7 +76,8 @@ class Ndb { static private var db_files: [String] = ["data.mdb", "lock.mdb"] static var empty: Ndb { - Ndb(ndb: ndb_t(ndb: nil)) + print("txn: NOSTRDB EMPTY") + return Ndb(ndb: ndb_t(ndb: nil)) } static func open(path: String? = nil, owns_db_file: Bool = true) -> ndb_t? { @@ -187,6 +188,7 @@ class Ndb { self.closed = true print("txn: CLOSING NOSTRDB") ndb_destroy(self.ndb.ndb) + self.generation += 1 print("txn: NOSTRDB CLOSED") } @@ -195,8 +197,9 @@ class Ndb { let db = Self.open(path: self.path, owns_db_file: self.owns_db) else { return false } + + print("txn: NOSTRDB REOPENED (gen \(generation))") - self.generation += 1 self.closed = false self.ndb = db return true @@ -368,14 +371,14 @@ class Ndb { return txn.value } - func lookup_note(_ id: NoteId) -> NdbTxn? { - NdbTxn(ndb: self) { txn in + func lookup_note(_ id: NoteId, txn_name: String? = nil) -> NdbTxn? { + NdbTxn(ndb: self, name: txn_name) { txn in lookup_note_with_txn_inner(id: id, txn: txn) } } - func lookup_profile(_ pubkey: Pubkey) -> NdbTxn? { - NdbTxn(ndb: self) { txn in + func lookup_profile(_ pubkey: Pubkey, txn_name: String? = nil) -> NdbTxn? { + NdbTxn(ndb: self, name: txn_name) { txn in lookup_profile_with_txn_inner(pubkey: pubkey, txn: txn) } } @@ -456,17 +459,8 @@ class Ndb { } deinit { - //print("txn: Ndb de-init close") - // - // DO NOT CLOSE IN DESTRUCTOR! Destructor deinit is clearly - // not reliable, since it seems to be getting called more than - // once when we have a few references to the database. - // - // Not sure if this is indicitable of a larger problem but I've - // experienced nothing but hell when relying on de-init for - // global resources. Free them manually. - // - // EVIL --> self.close() <-- EVIL + print("txn: Ndb de-init") + self.close() } } diff --git a/nostrdb/NdbTxn.swift b/nostrdb/NdbTxn.swift index d2b44a6e..5482d91f 100644 --- a/nostrdb/NdbTxn.swift +++ b/nostrdb/NdbTxn.swift @@ -19,17 +19,16 @@ class NdbTxn { var inherited: Bool var ndb: Ndb var generation: Int + var name: String - init?(ndb: Ndb, with: (NdbTxn) -> T = { _ in () }) { - guard !ndb.closed else { return nil } + init?(ndb: Ndb, with: (NdbTxn) -> T = { _ in () }, name: String? = nil) { + guard !ndb.is_closed else { return nil } + self.name = name ?? "txn" self.ndb = ndb self.generation = ndb.generation -#if TXNDEBUG - txn_count += 1 - print("opening transaction \(txn_count)") - #endif if let active_txn = Thread.current.threadDictionary["ndb_txn"] as? ndb_txn { // some parent thread is active, use that instead + print("txn: inherited txn") self.txn = active_txn self.inherited = true self.generation = Thread.current.threadDictionary["txn_generation"] as! Int @@ -37,6 +36,9 @@ class NdbTxn { self.txn = ndb_txn() guard !ndb.is_closed else { return nil } self.generation = ndb.generation + #if TXNDEBUG + txn_count += 1 + #endif let ok = ndb_begin_query(ndb.ndb.ndb, &self.txn) != 0 if !ok { return nil @@ -46,17 +48,21 @@ class NdbTxn { Thread.current.threadDictionary["txn_generation"] = ndb.generation self.inherited = false } + #if TXNDEBUG + print("txn: open gen\(self.generation) '\(self.name)' \(txn_count)") + #endif self.moved = false self.val = with(self) } - private init(ndb: Ndb, txn: ndb_txn, val: T) { + private init(ndb: Ndb, txn: ndb_txn, val: T, generation: Int, inherited: Bool, name: String) { self.txn = txn self.val = val self.moved = false - self.inherited = false + self.inherited = inherited self.ndb = ndb - self.generation = 0 + self.generation = generation + self.name = name } /// Only access temporarily! Do not store database references for longterm use. If it's a primitive type you @@ -68,32 +74,42 @@ class NdbTxn { deinit { if self.generation != ndb.generation { - //print("txn: OLD GENERATION (\(self.generation) != \(ndb.generation)), IGNORING") + print("txn: OLD GENERATION (\(self.generation) != \(ndb.generation)), IGNORING") return } - if moved || inherited || ndb.closed { + if inherited { + print("txn: not closing. inherited ") + return + } + if moved { + //print("txn: not closing. moved") + return + } + if ndb.is_closed { + print("txn: not closing. db closed") return } #if TXNDEBUG txn_count -= 1; - print("txn: closing transaction \(txn_count)") + print("txn: close gen\(generation) '\(name)' \(txn_count)") #endif ndb_end_query(&self.txn) + //self.skip_close = true Thread.current.threadDictionary.removeObject(forKey: "ndb_txn") } // functor func map(_ transform: (T) -> Y) -> NdbTxn { self.moved = true - return .init(ndb: self.ndb, txn: self.txn, val: transform(val)) + return .init(ndb: self.ndb, txn: self.txn, val: transform(val), generation: generation, inherited: inherited, name: self.name) } // comonad!? // useful for moving ownership of a transaction to another value func extend(_ with: (NdbTxn) -> Y) -> NdbTxn { self.moved = true - return .init(ndb: self.ndb, txn: self.txn, val: with(self)) + return .init(ndb: self.ndb, txn: self.txn, val: with(self), generation: generation, inherited: inherited, name: self.name) } } @@ -116,7 +132,7 @@ extension NdbTxn where T: OptionalType { return nil } self.moved = true - return NdbTxn(ndb: self.ndb, txn: self.txn, val: unwrappedVal) + return NdbTxn(ndb: self.ndb, txn: self.txn, val: unwrappedVal, generation: generation, inherited: inherited, name: name) } }