From 00151380e3bedb475e532a69d0b4490a5b33f438 Mon Sep 17 00:00:00 2001 From: BruebachL <44814898+BruebachL@users.noreply.github.com> Date: Sat, 31 Oct 2026 19:37:15 +0100 Subject: [PATCH] [CardDB] Break the CardInfo <-> CardSet cycle on reload (#7645) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [CardDB] Drop the CardInfo self pointer The weak self pointer had to be assigned right after every construction, and a missed assignment silently stopped change notifications. CardInfo is always owned by a QSharedPointer, so it now derives from QEnableSharedFromThis and asks for sharedFromThis() instead: nothing to initialize, and the CardSet cycle stays broken. CardSet::newInstance() also loses the commented-out self-pointer assignment left over from the same design. * [CardDB] Pin the cardInfoChanged contract without the self pointer Dropping CardInfo's self pointer moved change notifications onto QEnableSharedFromThis, so the signal now rests on the card being owned by a QSharedPointer. Two tests pin that down: - A property change on a card owned by a CardDatabase still emits cardInfoChanged() carrying that card, which is what the card database views and models listen to. - clone() starts out unowned, so a change to the clone is reported as the clone's and must not reach the original's observers. * [CardDB] Treat an unowned CardInfo as a bug, not a no-op Every CardInfo is built inside a QSharedPointer (both factory overloads and clone()), and no constructor notifies anyone, so sharedFromThis() is always available by the time addToSet() or a setter runs. Assert that and warn in a released build, instead of silently skipping the notification or the set append. The copy constructor keeps its explicit QEnableSharedFromThis initializer: GCC's -Wextra warns about the uninitialized base in every translation unit that copies a CardInfo otherwise. A comment there records that the clone deliberately starts unowned. * [CardDB] Break the CardInfo <-> CardSet cycle on reload CardSet is a QList and CardInfo::addToSet() appends the card to its sets, while a card keeps a CardSetPtr per printing in setsToPrintings. The sets therefore hold strong references back to the cards, and the previous database snapshot stayed alive after a reload, so RSS grew by roughly a second database on every reload. Empty the sets before the cards are dropped: CardDatabase::clear() and swapInDatabaseData() clear the sets before replacing the containers, and removeCard() detaches the card from its sets, so nothing keeps a card alive on behalf of a set once the database lets go of it. Two tests pin the behaviour: a reload and a clear() must drop the last strong reference to a card from the previous snapshot. * [CardDB] Keep the set detach in one place clear() and swapInDatabaseData() both emptied every set before dropping the cards, so share that as detachSetMembership() instead of two copies that have to stay in sync. removeCard() now detaches through the first printing of each set directly, the same way CardInfo::newInstance() appends, instead of an inner loop that always breaks after the first item. Because clear() empties the sets up front, its per-card detach was walking every card's printings to remove it from sets that were already empty. Split removeCard() into the per-card detach and unindexCard(), and let clear() call only the latter. * [CardDB] Test that removeCard detaches the card from its set removeCard() is the one teardown path a caller can trigger for a single card (the token editor uses it), and it now detaches the card from its sets as well. Without that detach the set's strong CardInfoPtr keeps the removed card alive forever, which is exactly what the reload and clear tests pin for the whole-snapshot paths. The test loads the fixture, removes Cat, and requires the card to be gone from the CAT set and to be destroyed once the test drops its own reference. * [Servatrice] Drop the settings cache from the logger #7602 removed #include "settingscache.h" from server_logger.cpp but left isLoggingEnabled() reading settingsCache, so master no longer compiles: the servatrice target fails on the undeclared global and server_logger_test, which compiles the same file without main.cpp, fails to link it. The same commit made reloadConfig() the single place that stores writelog and the log filters, precisely so logging threads stop reading the global QSettings. isLoggingEnabled() now mirrors logMessage() and reads those members instead: the console is a sink of its own, and the logfile needs an open file plus writelog. The mutex is mutable so the const query can take it. --------- Co-authored-by: Lukas Brübach --- .../libcockatrice/card/card_info.cpp | 30 ++++- .../libcockatrice/card/card_info.h | 51 ++++----- .../card/database/card_database.cpp | 36 +++++- .../card/database/card_database.h | 17 +++ .../libcockatrice/card/set/card_set.cpp | 1 - servatrice/src/server_logger.cpp | 5 +- servatrice/src/server_logger.h | 2 +- tests/carddatabase/carddatabase_test.cpp | 106 ++++++++++++++++++ tests/server_logger_test.cpp | 22 ++++ 9 files changed, 232 insertions(+), 38 deletions(-) diff --git a/libcockatrice_card/libcockatrice/card/card_info.cpp b/libcockatrice_card/libcockatrice/card/card_info.cpp index 388c9ab57..90bfd3f44 100644 --- a/libcockatrice_card/libcockatrice/card/card_info.cpp +++ b/libcockatrice_card/libcockatrice/card/card_info.cpp @@ -6,6 +6,7 @@ #include "relation/card_relation.h" #include "set/card_set.h" +#include #include #include #include @@ -19,6 +20,18 @@ const QHash &CardInfo::getPropertiesHash() const return properties.getProperties(); } +void CardInfo::notifyChanged() +{ + const CardInfoPtr self = sharedFromThis(); + Q_ASSERT_X(self, "CardInfo::notifyChanged", "every CardInfo is owned by a QSharedPointer"); + if (!self) { + qCWarning(CardInfoLog) << "notifyChanged() on an unowned CardInfo" << name << "- observers stay stale"; + return; + } + + emit cardInfoChanged(self); +} + void CardInfo::setProperty(const QString &_name, const QString &_value) { bool changed = properties.insert(_name, _value); @@ -26,7 +39,7 @@ void CardInfo::setProperty(const QString &_name, const QString &_value) return; } - emit cardInfoChanged(smartThis); + notifyChanged(); } CardInfo::CardInfo(const QString &_name, @@ -97,7 +110,6 @@ CardInfoPtr CardInfo::newInstance(const QString &_name, { CardInfoPtr ptr(new CardInfo(_name, _text, _isToken, _properties, _relatedCards, _reverseRelatedCards, _sets, _uiAttributes, std::move(_localizedNames), std::move(_localizedTexts))); - ptr->setSmartPointer(ptr); for (const auto &printings : _sets) { for (const PrintingInfo &printing : printings) { @@ -126,7 +138,6 @@ CardInfoPtr CardInfo::newInstance(const QString &_name, CardInfoPtr ptr(new CardInfo(_name, _text, _isToken, std::move(_propertiesBlob), _relatedCards, _reverseRelatedCards, _sets, _uiAttributes, std::move(_simpleName), std::move(_altNames), std::move(_localizedNames), std::move(_localizedTexts))); - ptr->setSmartPointer(ptr); if (_appendToSets) { for (const auto &printings : _sets) { @@ -169,8 +180,15 @@ bool CardInfo::isLegalInFormat(const QString &format) const void CardInfo::addToSet(const CardSetPtr &_set, const PrintingInfo &_info) { - if (!_set->contains(smartThis)) { - _set->append(smartThis); + const CardInfoPtr self = sharedFromThis(); + Q_ASSERT_X(self, "CardInfo::addToSet", "every CardInfo is owned by a QSharedPointer"); + if (!self) { + qCWarning(CardInfoLog) << "addToSet() on an unowned CardInfo" << name << "- the set will not list it"; + return; + } + + if (!_set->contains(self)) { + _set->append(self); } if (!setsToPrintings[_set->getShortName()].contains(_info)) { setsToPrintings[_set->getShortName()].append(_info); @@ -188,7 +206,7 @@ void CardInfo::combineLegalities(const QHash &props) properties.insert(it.key(), it.value()); } } - emit cardInfoChanged(smartThis); + notifyChanged(); } void CardInfo::refreshCachedSets() diff --git a/libcockatrice_card/libcockatrice/card/card_info.h b/libcockatrice_card/libcockatrice/card/card_info.h index eb1250a1c..5452ef98b 100644 --- a/libcockatrice_card/libcockatrice/card/card_info.h +++ b/libcockatrice_card/libcockatrice/card/card_info.h @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -50,7 +51,7 @@ Q_DECLARE_METATYPE(CardInfoPtr) * Each CardInfo may belong to multiple sets through its printings, and can * be related to other cards through defined relationships. */ -class CardInfo : public QObject +class CardInfo : public QObject, public QEnableSharedFromThis { Q_OBJECT @@ -74,11 +75,10 @@ private: * @anchor PrivateCardProperties */ ///@{ - CardInfoPtr smartThis; ///< Smart pointer to self for safe cross-references. - QString name; ///< Full name of the card. - QString simpleName; ///< Simplified name for fuzzy matching. - QString text; ///< Text description or rules text of the card. - bool isToken; ///< Whether this card is a token or not. + QString name; ///< Full name of the card. + QString simpleName; ///< Simplified name for fuzzy matching. + QString text; ///< Text description or rules text of the card. + bool isToken; ///< Whether this card is a token or not. QMap localizedNames; ///< Localized card names, keyed by language code. QMap localizedTexts; ///< Localized rules text, keyed by language code. @@ -162,13 +162,14 @@ public: * @param other Another CardInfo to copy. */ CardInfo(const CardInfo &other) - : QObject(other.parent()), name(other.name), simpleName(other.simpleName), text(other.text), - isToken(other.isToken), localizedNames(other.localizedNames), localizedTexts(other.localizedTexts), - properties(other.properties), relatedCards(other.relatedCards), + : QObject(other.parent()), QEnableSharedFromThis(), name(other.name), simpleName(other.simpleName), + text(other.text), isToken(other.isToken), localizedNames(other.localizedNames), + localizedTexts(other.localizedTexts), properties(other.properties), relatedCards(other.relatedCards), reverseRelatedCards(other.reverseRelatedCards), reverseRelatedCardsToMe(other.reverseRelatedCardsToMe), setsToPrintings(other.setsToPrintings), uiAttributes(other.uiAttributes), setsNames(other.setsNames), altNames(other.altNames) { + // The copy deliberately starts unowned: the QSharedPointer that wraps it registers the self reference. } /** @@ -252,27 +253,13 @@ public: /** * @brief Clones the current CardInfo instance. * - * Uses the copy constructor and ensures the smart pointer is properly set. + * Uses the copy constructor; the copy is managed by its own shared pointer. * * @return Shared pointer to the cloned CardInfo. */ [[nodiscard]] CardInfoPtr clone() const { - auto newCardInfo = CardInfoPtr(new CardInfo(*this)); - newCardInfo->setSmartPointer(newCardInfo); // Set the smart pointer for the new instance - return newCardInfo; - } - - /** - * @brief Sets the internal smart pointer to self. - * - * Used internally to allow safe cross-references among CardInfo and CardSet. - * - * @param _ptr Shared pointer pointing to this instance. - */ - void setSmartPointer(CardInfoPtr _ptr) - { - smartThis = std::move(_ptr); + return CardInfoPtr(new CardInfo(*this)); } /** @name Basic Properties Accessors */ //@{ @@ -295,7 +282,7 @@ public: void setText(const QString &_text) { text = _text; - emit cardInfoChanged(smartThis); + notifyChanged(); } /** @@ -358,7 +345,7 @@ public: return; } localizedNames.insert(lang, _localizedName); - emit cardInfoChanged(smartThis); + notifyChanged(); } /** @@ -373,7 +360,7 @@ public: return; } localizedTexts.insert(lang, _localizedText); - emit cardInfoChanged(smartThis); + notifyChanged(); } /** @@ -552,6 +539,14 @@ public: static QString simplifyName(const QString &name); private: + /** + * @brief Emits cardInfoChanged() with a strong reference to this card. + * + * A CardInfo is always owned by a QSharedPointer, so the self reference is always available; a released + * build that somehow finds it missing warns rather than emitting a null card. + */ + void notifyChanged(); + /** * @brief Refreshes the cached, human-readable list of set names. * diff --git a/libcockatrice_card/libcockatrice/card/database/card_database.cpp b/libcockatrice_card/libcockatrice/card/database/card_database.cpp index 946412606..ed097ef3b 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database.cpp +++ b/libcockatrice_card/libcockatrice/card/database/card_database.cpp @@ -53,13 +53,26 @@ CardDatabase::~CardDatabase() clear(); } +void CardDatabase::detachSetMembership() +{ + for (const auto &set : sets) { + if (set) { + set->clear(); + } + } +} + void CardDatabase::clear() { QMutexLocker locker(clearDatabaseMutex); + // The sets hold strong card references, so empty them before dropping the cards: this breaks the + // CardInfo <-> CardSet cycle and spares the per-card detach over the whole database. + detachSetMembership(); + for (const auto &card : cards.values()) { if (card) { - removeCard(card); + unindexCard(card); } } @@ -156,6 +169,23 @@ void CardDatabase::removeCard(const CardInfoPtr &card) return; } + // The set holds a strong reference back to the card, so detach it: leaving it there keeps both + // objects alive after the database drops the card. newInstance() appends through the first + // printing of every set, so detaching through that same printing mirrors the append. + for (const auto &printings : card->getSets()) { + if (printings.isEmpty()) { + continue; + } + if (const CardSetPtr set = printings.first().getSet()) { + set->removeAll(card); + } + } + + unindexCard(card); +} + +void CardDatabase::unindexCard(const CardInfoPtr &card) +{ for (auto *cardRelation : card->getRelatedCards()) { cardRelation->deleteLater(); } @@ -280,6 +310,10 @@ void CardDatabase::swapInDatabaseData(CardDatabaseData data) card->clearDirectRelations(); } + // Break the CardSet <-> CardInfo cycle before replacing the containers, otherwise the previous + // snapshot survives the reload through the sets' strong card lists. + detachSetMembership(); + cards = std::move(data.cards); simpleNameCards = std::move(data.simpleNameCards); sets = std::move(data.sets); diff --git a/libcockatrice_card/libcockatrice/card/database/card_database.h b/libcockatrice_card/libcockatrice/card/database/card_database.h index 9f0d67b67..5c1cb43c1 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database.h +++ b/libcockatrice_card/libcockatrice/card/database/card_database.h @@ -74,6 +74,23 @@ private: */ void refreshCachedReverseRelatedCards(CardNameMap &cardMap); + /** + * @brief Empties every set, dropping the strong card references they hold. + * + * A card keeps a CardSetPtr through its printings while its set keeps a strong CardInfoPtr, so the + * two form a reference cycle. Dropping a whole snapshot has to break that cycle once, up front, + * instead of relying on the per-card detach in removeCard(). + */ + void detachSetMembership(); + + /** + * @brief Drops a card from the database indexes and notifies observers. + * + * Callers are responsible for detaching the card from its sets first. + * @param card Pointer to the card to unindex. + */ + void unindexCard(const CardInfoPtr &card); + /** @brief Mutexes for thread safety. */ QBasicMutex *clearDatabaseMutex = new QBasicMutex(), *addCardMutex = new QBasicMutex(), *removeCardMutex = new QBasicMutex(); diff --git a/libcockatrice_card/libcockatrice/card/set/card_set.cpp b/libcockatrice_card/libcockatrice/card/set/card_set.cpp index e2b41a811..a0f0f4578 100644 --- a/libcockatrice_card/libcockatrice/card/set/card_set.cpp +++ b/libcockatrice_card/libcockatrice/card/set/card_set.cpp @@ -27,7 +27,6 @@ CardSetPtr CardSet::newInstance(ICardSetPriorityController *_priorityController, const Priority _priority) { CardSetPtr ptr(new CardSet(_priorityController, _shortName, _longName, _setType, _releaseDate, _priority)); - // ptr->setSmartPointer(ptr); return ptr; } diff --git a/servatrice/src/server_logger.cpp b/servatrice/src/server_logger.cpp index dfe51183a..09453b7ee 100644 --- a/servatrice/src/server_logger.cpp +++ b/servatrice/src/server_logger.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -66,7 +67,9 @@ void ServerLogger::reloadConfig(bool _writeLog, const QString &_logFilters) bool ServerLogger::isLoggingEnabled() const { - return logFile != nullptr && settingsCache->value("server/writelog", 1).toBool(); + // Mirror logMessage(): the console is an independent sink and the logfile needs writeLog. + QMutexLocker locker(&configMutex); + return logToConsole || (logFile != nullptr && writeLog); } void ServerLogger::logMessage(const QString &message, void *caller) diff --git a/servatrice/src/server_logger.h b/servatrice/src/server_logger.h index 531430d56..40df9fd62 100644 --- a/servatrice/src/server_logger.h +++ b/servatrice/src/server_logger.h @@ -41,7 +41,7 @@ private: bool messageFilterActive = false; QStringList logFilters; // Guards writeLog/messageFilterActive/logFilters: SIGHUP rewrites them while other threads log. - QMutex configMutex; + mutable QMutex configMutex; QFile *logFile = nullptr; bool flushRunning; QStringList buffer; diff --git a/tests/carddatabase/carddatabase_test.cpp b/tests/carddatabase/carddatabase_test.cpp index eb522434d..22cbe9074 100644 --- a/tests/carddatabase/carddatabase_test.cpp +++ b/tests/carddatabase/carddatabase_test.cpp @@ -17,10 +17,12 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include @@ -608,6 +610,110 @@ TEST_F(CardDatabaseCacheInvalidationTest, MarkAllSetsAsKnownIsBatched) // Ignored sets keep their known state but are enabled. EXPECT_TRUE(controller.isEnabled("XXX")); } + +TEST_F(CardDatabaseCacheInvalidationTest, ReloadReleasesPreviousSnapshot) +{ + NoopCardPreferenceProvider prefs; + SandboxPathProvider pathProvider(root); + CardDatabase db(nullptr, &prefs, &pathProvider, &controller); + + db.loadCardDatabases(); + CardInfoPtr card = db.getCardList().value("Cat"); + ASSERT_FALSE(card.isNull()); + QWeakPointer weakCard(card); + card.clear(); + + db.reloadCardDatabasesAndNotify(); + + // CardSet holds strong CardInfoPtrs and CardInfo holds the CardSetPtr through its printings; if that + // cycle is not broken, the previous snapshot survives the reload. + EXPECT_TRUE(weakCard.isNull()) << "a reload must release the previous card snapshot"; +} + +TEST_F(CardDatabaseCacheInvalidationTest, ClearReleasesCards) +{ + NoopCardPreferenceProvider prefs; + SandboxPathProvider pathProvider(root); + CardDatabase db(nullptr, &prefs, &pathProvider, &controller); + + db.loadCardDatabases(); + CardInfoPtr card = db.getCardList().value("Cat"); + ASSERT_FALSE(card.isNull()); + QWeakPointer weakCard(card); + card.clear(); + + db.clear(); + + EXPECT_TRUE(weakCard.isNull()) << "clear() must release cards held by set membership"; +} + +TEST_F(CardDatabaseCacheInvalidationTest, RemoveCardDetachesItFromItsSet) +{ + // removeCard() is the one teardown path a caller can trigger for a single card (the token editor + // uses it). Without the detach the set's strong reference keeps the removed card alive forever. + NoopCardPreferenceProvider prefs; + SandboxPathProvider pathProvider(root); + CardDatabase db(nullptr, &prefs, &pathProvider, &controller); + + db.loadCardDatabases(); + CardInfoPtr card = db.getCardList().value("Cat"); + ASSERT_FALSE(card.isNull()); + const CardSetPtr catSet = db.getSet("CAT"); + ASSERT_TRUE(catSet->contains(card)); + + db.removeCard(card); + + EXPECT_FALSE(catSet->contains(card)) << "removeCard() must detach the card from its set"; + QWeakPointer weakCard(card); + card.clear(); + EXPECT_TRUE(weakCard.isNull()) << "a removed card must not stay alive through its set"; +} + +TEST_F(CardDatabaseCacheInvalidationTest, RemoveSelfPointerStillNotifiesDatabaseCard) +{ + // cardInfoChanged() used to be emitted through a self pointer stored on the card. Without it the + // card must still notify its observers whenever its properties change. + NoopCardPreferenceProvider prefs; + SandboxPathProvider pathProvider(root); + CardDatabase db(nullptr, &prefs, &pathProvider, &controller); + + db.loadCardDatabases(); + const CardInfoPtr card = db.getCardList().value("Cat"); + ASSERT_FALSE(card.isNull()); + QSignalSpy spy(card.data(), &CardInfo::cardInfoChanged); + + card->setProperty("manacost", "1G"); + + ASSERT_EQ(spy.count(), 1) << "a property change on a database card must still emit cardInfoChanged"; + const QList arguments = spy.takeFirst(); + ASSERT_EQ(arguments.size(), 1); + EXPECT_EQ(arguments.first().value().data(), card.data()) + << "the signal must carry the card that changed"; +} + +TEST_F(CardDatabaseCacheInvalidationTest, RemoveSelfPointerCloneNotifiesAsItself) +{ + // A clone owns its own shared reference, so its changes must be reported as the clone's, never the + // original's: decks and views listening to the original must not be invalidated by a clone edit. + NoopCardPreferenceProvider prefs; + SandboxPathProvider pathProvider(root); + CardDatabase db(nullptr, &prefs, &pathProvider, &controller); + + db.loadCardDatabases(); + const CardInfoPtr original = db.getCardList().value("Cat"); + ASSERT_FALSE(original.isNull()); + const CardInfoPtr clone = original->clone(); + QSignalSpy originalSpy(original.data(), &CardInfo::cardInfoChanged); + QSignalSpy cloneSpy(clone.data(), &CardInfo::cardInfoChanged); + + clone->setProperty("manacost", "1G"); + + ASSERT_EQ(cloneSpy.count(), 1) << "the clone must notify its own observers"; + EXPECT_EQ(cloneSpy.first().first().value().data(), clone.data()) + << "the clone's signal must carry the clone"; + EXPECT_EQ(originalSpy.count(), 0) << "the clone must not notify the original's observers"; +} + } // namespace int main(int argc, char **argv) diff --git a/tests/server_logger_test.cpp b/tests/server_logger_test.cpp index 94a6909bf..3c2d047cc 100644 --- a/tests/server_logger_test.cpp +++ b/tests/server_logger_test.cpp @@ -98,6 +98,28 @@ TEST(ServerLoggerTest, StaysQuietWhenConsoleIsDisabled) EXPECT_TRUE(capture.text().isEmpty()) << capture.text().toStdString(); } +TEST(ServerLoggerTest, IsLoggingEnabledMirrorsTheSinks) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + + // The console is a sink of its own: messages must not be discarded for want of a logfile. + { + ServerLogger logger(true, false, QString()); + EXPECT_TRUE(logger.isLoggingEnabled()); + } + + // The logfile sink needs an open file and writelog; reloadConfig() carries the SIGHUP change. + { + ServerLogger logger(false, true, QString()); + EXPECT_FALSE(logger.isLoggingEnabled()) << "no logfile is open yet"; + logger.startLog(dir.filePath("enabled.log")); + EXPECT_TRUE(logger.isLoggingEnabled()); + logger.reloadConfig(false, QString()); + EXPECT_FALSE(logger.isLoggingEnabled()) << "writelog=false leaves no sink enabled"; + } +} + TEST(ServerLoggerTest, WritesMatchingFilteredMessagesToTheLogFile) { QTemporaryDir dir;