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;