diff --git a/.github/scripts/levelization/results/ordering.txt b/.github/scripts/levelization/results/ordering.txt index c2000d1768..7a5be869a5 100644 --- a/.github/scripts/levelization/results/ordering.txt +++ b/.github/scripts/levelization/results/ordering.txt @@ -12,7 +12,6 @@ libxrpl.ledger > xrpl.json libxrpl.ledger > xrpl.ledger libxrpl.ledger > xrpl.nodestore libxrpl.ledger > xrpl.protocol -libxrpl.ledger > xrpl.server libxrpl.ledger > xrpl.shamap libxrpl.net > xrpl.basics libxrpl.net > xrpl.net @@ -206,7 +205,6 @@ xrpl.core > xrpl.protocol xrpl.json > xrpl.basics xrpl.ledger > xrpl.basics xrpl.ledger > xrpl.protocol -xrpl.ledger > xrpl.server xrpl.ledger > xrpl.shamap xrpl.net > xrpl.basics xrpl.nodestore > xrpl.basics diff --git a/include/xrpl/ledger/BookListeners.h b/include/xrpl/ledger/BookListeners.h deleted file mode 100644 index 3b96aca680..0000000000 --- a/include/xrpl/ledger/BookListeners.h +++ /dev/null @@ -1,49 +0,0 @@ -#pragma once - -#include -#include - -#include -#include - -namespace xrpl { - -/** Listen to public/subscribe messages from a book. */ -class BookListeners -{ -public: - using pointer = std::shared_ptr; - - BookListeners() = default; - - /** Add a new subscription for this book - */ - void - addSubscriber(InfoSub::ref sub); - - /** Stop publishing to a subscriber - */ - void - removeSubscriber(std::uint64_t sub); - - /** Publish a transaction to subscribers - - Publish a transaction to clients subscribed to changes on this book. - Uses havePublished to prevent sending duplicate transactions to clients - that have subscribed to multiple books. - - @param jvObj JSON transaction data to publish - @param havePublished InfoSub sequence numbers that have already - published this transaction. - - */ - void - publish(MultiApiJson const& jvObj, hash_set& havePublished); - -private: - std::recursive_mutex lock_; - - hash_map listeners_; -}; - -} // namespace xrpl diff --git a/include/xrpl/ledger/OrderBookDB.h b/include/xrpl/ledger/OrderBookDB.h index a0aee58e2a..a44183900c 100644 --- a/include/xrpl/ledger/OrderBookDB.h +++ b/include/xrpl/ledger/OrderBookDB.h @@ -1,11 +1,11 @@ #pragma once +#include +#include #include -#include #include #include #include -#include #include #include @@ -77,34 +77,24 @@ public: */ virtual bool isBookToXRP(Asset const& asset, std::optional const& domain = std::nullopt) = 0; - - /** - * Process a transaction for order book tracking. - * @param ledger The ledger the transaction was applied to - * @param alTx The transaction to process - * @param jvObj The JSON object of the transaction - */ - virtual void - processTxn( - std::shared_ptr const& ledger, - AcceptedLedgerTx const& alTx, - MultiApiJson const& jvObj) = 0; - - /** - * Get the book listeners for a book. - * @param book The book to get the listeners for - * @return The book listeners for the book - */ - virtual BookListeners::pointer - getBookListeners(Book const&) = 0; - - /** - * Create a new book listeners for a book. - * @param book The book to create the listeners for - * @return The new book listeners for the book - */ - virtual BookListeners::pointer - makeBookListeners(Book const&) = 0; }; +/** Extract the set of books affected by a transaction. + * + * Walks the transaction's metadata nodes and collects every order book + * whose offers were created, modified, or deleted. Used by NetworkOPs to + * fan transaction notifications out to book subscribers. + * + * @param alTx The accepted ledger transaction to inspect. + * @param j Journal used to log per-node parsing failures. Inspecting an + * offer node can throw if a required field is missing; in that + * case the bad node is skipped and a warn-level message is + * emitted via @p j. Other affected books in the same transaction + * are still returned. + * @return The set of books whose offers were created, modified, or + * deleted. May be empty for non-offer transactions. + */ +hash_set +affectedBooks(AcceptedLedgerTx const& alTx, beast::Journal const& j); + } // namespace xrpl diff --git a/include/xrpl/server/InfoSub.h b/include/xrpl/server/InfoSub.h index e93676a938..f316885fd6 100644 --- a/include/xrpl/server/InfoSub.h +++ b/include/xrpl/server/InfoSub.h @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include #include @@ -26,6 +27,19 @@ public: }; /** Manages a client's subscription to data feeds. + * + * An InfoSub holds a non-owning reference to its `Source` (typically the + * process-wide `NetworkOPsImp`). The destructor reaches back into the + * `Source` to remove this subscriber from every server-side subscription + * map. + * + * @note Lifetime contract: every `InfoSub` instance MUST be destroyed + * before the backing `Source`. NetworkOPsImp shutdown drops all + * subscriber strong refs before its own teardown to satisfy this. + * @note Thread-safety: per-instance state is guarded by `lock_`. The + * destructor reads tracking sets without taking `lock_` because + * the strong-pointer ref-count is zero at destruction time, so + * no other thread can be calling the public mutators. */ class InfoSub : public CountedObject { @@ -117,8 +131,43 @@ public: virtual bool subBook(ref ispListener, Book const&) = 0; + + /** + * Remove a book subscription for a live subscriber. + * + * Clears the book from the subscriber's own tracking set + * (InfoSub::bookSubscriptions_) and then removes the server-side + * entry from subBook_. Call this from RPC unsubscribe handlers. + * + * @param ispListener The subscriber requesting removal. + * @param book The order book to unsubscribe from. + * @return true if the entry was present and removed, false if the + * subscriber was not subscribed to @p book. + * + * @note Thread-safety: acquires subLock_ internally. + * @note Do NOT call from ~InfoSub(). Use unsubBookInternal instead + * to avoid a redundant write-back to bookSubscriptions_ on a + * partially-destroyed object. + */ virtual bool - unsubBook(std::uint64_t uListener, Book const&) = 0; + unsubBook(ref ispListener, Book const&) = 0; + + /** + * Remove a book subscription during InfoSub teardown. + * + * Removes only the server-side entry from subBook_. Does NOT touch + * InfoSub::bookSubscriptions_ because the InfoSub is being destroyed. + * Called by ~InfoSub() for each book in bookSubscriptions_. + * + * @param uListener The sequence number of the subscriber being torn down. + * @param book The order book entry to remove. + * @return true if the entry was present and removed, false otherwise + * (e.g., already removed by a concurrent RPC unsubscribe). + * + * @note Thread-safety: acquires subLock_ internally. + */ + virtual bool + unsubBookInternal(std::uint64_t uListener, Book const&) = 0; virtual bool subTransactions(ref ispListener) = 0; @@ -158,6 +207,13 @@ public: addRpcSub(std::string const& strUrl, ref rspEntry) = 0; virtual bool tryRemoveRpcSub(std::string const& strUrl) = 0; + + /** Journal used by InfoSub for diagnostics that occur after the + * owning subsystem (e.g. application-level Logs) is the only + * surviving sink — primarily destructor-time cleanup failures. + */ + [[nodiscard]] virtual beast::Journal const& + journal() const = 0; }; public: @@ -184,6 +240,31 @@ public: void deleteSubAccountInfo(AccountID const& account, bool rt); + /** Record that this subscriber is following @p book. + * + * Called by NetworkOPsImp::subBook so that ~InfoSub() can issue a + * matching unsubBook for every book this subscriber is tracking, + * keeping per-subscriber state symmetric with the server-side map. + * + * @param book The order book this subscriber has just subscribed to. + * @note Idempotent: re-inserting an already-tracked book is a no-op. + * @note Thread-safe: takes InfoSub::lock_. + */ + void + insertBookSubscription(Book const& book); + + /** Stop tracking @p book for this subscriber. + * + * Called by the unsubscribe RPC handler so that the book is not + * re-unsubscribed by ~InfoSub(). Pairs with insertBookSubscription. + * + * @param book The order book to forget. + * @note No-op if @p book was not previously inserted. + * @note Thread-safe: takes InfoSub::lock_. + */ + void + deleteBookSubscription(Book const& book); + // return false if already subscribed to this account bool insertSubAccountHistory(AccountID const& account); @@ -217,6 +298,7 @@ private: std::shared_ptr request_; std::uint64_t seq_; hash_set accountHistorySubscriptions_; + hash_set bookSubscriptions_; unsigned int apiVersion_ = 0; static int diff --git a/include/xrpl/server/NetworkOPs.h b/include/xrpl/server/NetworkOPs.h index e2aa17566e..785d808935 100644 --- a/include/xrpl/server/NetworkOPs.h +++ b/include/xrpl/server/NetworkOPs.h @@ -249,6 +249,19 @@ public: virtual void stateAccounting(json::Value& obj) = 0; + + /** Total number of (book, subscriber) entries currently tracked. + * + * Counts every weak_ptr stored across every book in subBook_, NOT the + * number of distinct subscribers and NOT the number of distinct + * books: a single subscriber following N books contributes N entries. + * + * @note Diagnostic accessor; intended for tests and operator visibility + * into per-book subscription state. The returned value is a + * snapshot under the subscription lock. + */ + virtual std::size_t + getBookSubscribersCount() = 0; }; } // namespace xrpl diff --git a/src/libxrpl/ledger/BookListeners.cpp b/src/libxrpl/ledger/BookListeners.cpp deleted file mode 100644 index d78da4c73e..0000000000 --- a/src/libxrpl/ledger/BookListeners.cpp +++ /dev/null @@ -1,55 +0,0 @@ -#include - -#include -#include -#include -#include - -#include -#include - -namespace xrpl { - -void -BookListeners::addSubscriber(InfoSub::ref sub) -{ - std::scoped_lock const sl(lock_); - listeners_[sub->getSeq()] = sub; -} - -void -BookListeners::removeSubscriber(std::uint64_t seq) -{ - std::scoped_lock const sl(lock_); - listeners_.erase(seq); -} - -void -BookListeners::publish(MultiApiJson const& jvObj, hash_set& havePublished) -{ - std::scoped_lock const sl(lock_); - auto it = listeners_.cbegin(); - - while (it != listeners_.cend()) - { - InfoSub::pointer p = it->second.lock(); - - if (p) - { - // Only publish jvObj if this is the first occurrence - if (havePublished.emplace(p->getSeq()).second) - { - jvObj.visit( - p->getApiVersion(), // - [&](json::Value const& jv) { p->send(jv, true); }); - } - ++it; - } - else - { - it = listeners_.erase(it); - } - } -} - -} // namespace xrpl diff --git a/src/libxrpl/server/InfoSub.cpp b/src/libxrpl/server/InfoSub.cpp index 87b48296a1..353c295856 100644 --- a/src/libxrpl/server/InfoSub.cpp +++ b/src/libxrpl/server/InfoSub.cpp @@ -1,15 +1,47 @@ #include +#include +#include #include #include +#include #include #include +#include #include #include namespace xrpl { +namespace { + +// Wraps a Source teardown call so that an exception from one cleanup +// step does not prevent the subsequent steps from running. Source methods +// acquire a lock and can throw std::system_error; a throw out of ~InfoSub +// during stack unwinding would terminate the process. Failures are +// reported through the Source's Journal so they reach the configured log +// sinks; JLOG itself cannot throw, so the noexcept guarantee holds. +template +void +safeUnsub(std::uint64_t seq, F&& f, beast::Journal j) noexcept +{ + try + { + f(); + } + catch (std::exception const& e) + { + JLOG(j.warn()) << "~InfoSub[seq=" << seq << "]: cleanup step failed: " << e.what(); + } + catch (...) + { + JLOG(j.warn()) << "~InfoSub[seq=" << seq << "]: cleanup step failed: unknown exception"; + } +} + +} // namespace + // This is the primary interface into the "client" portion of the program. // Code that wants to do normal operations on the network such as // creating and monitoring accounts, creating transactions, and so on @@ -32,25 +64,44 @@ InfoSub::InfoSub(Source& source, Consumer consumer) InfoSub::~InfoSub() { - source_.unsubTransactions(seq_); - source_.unsubRTTransactions(seq_); - source_.unsubLedger(seq_); - source_.unsubManifests(seq_); - source_.unsubServer(seq_); - source_.unsubValidations(seq_); - source_.unsubPeerStatus(seq_); - source_.unsubConsensus(seq_); + // Each Source teardown call below acquires a server-side lock and + // can throw. Wrap each independent call so partial failure does not + // skip the remaining teardown steps. + + auto const& j = source_.journal(); + + safeUnsub(seq_, [&] { source_.unsubTransactions(seq_); }, j); + safeUnsub(seq_, [&] { source_.unsubRTTransactions(seq_); }, j); + safeUnsub(seq_, [&] { source_.unsubLedger(seq_); }, j); + safeUnsub(seq_, [&] { source_.unsubManifests(seq_); }, j); + safeUnsub(seq_, [&] { source_.unsubServer(seq_); }, j); + safeUnsub(seq_, [&] { source_.unsubValidations(seq_); }, j); + safeUnsub(seq_, [&] { source_.unsubPeerStatus(seq_); }, j); + safeUnsub(seq_, [&] { source_.unsubConsensus(seq_); }, j); // Use the internal unsubscribe so that it won't call // back to us and modify its own parameter if (!realTimeSubscriptions_.empty()) - source_.unsubAccountInternal(seq_, realTimeSubscriptions_, true); + { + safeUnsub( + seq_, [&] { source_.unsubAccountInternal(seq_, realTimeSubscriptions_, true); }, j); + } if (!normalSubscriptions_.empty()) - source_.unsubAccountInternal(seq_, normalSubscriptions_, false); + { + safeUnsub( + seq_, [&] { source_.unsubAccountInternal(seq_, normalSubscriptions_, false); }, j); + } for (auto const& account : accountHistorySubscriptions_) - source_.unsubAccountHistoryInternal(seq_, account, false); + { + safeUnsub(seq_, [&] { source_.unsubAccountHistoryInternal(seq_, account, false); }, j); + } + + for (auto const& book : bookSubscriptions_) + { + safeUnsub(seq_, [&] { source_.unsubBookInternal(seq_, book); }, j); + } } Resource::Consumer& @@ -114,6 +165,20 @@ InfoSub::deleteSubAccountHistory(AccountID const& account) accountHistorySubscriptions_.erase(account); } +void +InfoSub::insertBookSubscription(Book const& book) +{ + std::scoped_lock const sl(lock_); + bookSubscriptions_.insert(book); +} + +void +InfoSub::deleteBookSubscription(Book const& book) +{ + std::scoped_lock const sl(lock_); + bookSubscriptions_.erase(book); +} + void InfoSub::clearRequest() { diff --git a/src/xrpld/app/ledger/OrderBookDBImpl.cpp b/src/xrpld/app/ledger/OrderBookDBImpl.cpp index 658ec4ea7a..1e474ef949 100644 --- a/src/xrpld/app/ledger/OrderBookDBImpl.cpp +++ b/src/xrpld/app/ledger/OrderBookDBImpl.cpp @@ -9,19 +9,17 @@ #include #include #include -#include #include +#include #include #include #include #include -#include #include #include #include #include -#include #include #include #include @@ -307,55 +305,10 @@ OrderBookDBImpl::isBookToXRP(Asset const& asset, std::optional const& do return xrpBooks_.contains(asset); } -BookListeners::pointer -OrderBookDBImpl::makeBookListeners(Book const& book) +hash_set +affectedBooks(AcceptedLedgerTx const& alTx, beast::Journal const& j) { - std::scoped_lock const sl(lock_); - auto ret = getBookListeners(book); - - if (!ret) - { - ret = std::make_shared(); - - listeners_[book] = ret; - XRPL_ASSERT( - getBookListeners(book) == ret, - "xrpl::OrderBookDB::makeBookListeners : result roundtrip " - "lookup"); - } - - return ret; -} - -BookListeners::pointer -OrderBookDBImpl::getBookListeners(Book const& book) -{ - BookListeners::pointer ret; - std::scoped_lock const sl(lock_); - - auto it0 = listeners_.find(book); - if (it0 != listeners_.end()) - ret = it0->second; - - return ret; -} - -// Based on the meta, send the meta to the streams that are listening. -// We need to determine which streams a given meta effects. -void -OrderBookDBImpl::processTxn( - std::shared_ptr const& ledger, - AcceptedLedgerTx const& alTx, - MultiApiJson const& jvObj) -{ - std::scoped_lock const sl(lock_); - - // For this particular transaction, maintain the set of unique - // subscriptions that have already published it. This prevents sending - // the transaction multiple times if it touches multiple ltOFFER - // entries for the same book, or if it touches multiple books and a - // single client has subscribed to those books. - hash_set havePublished; + hash_set result; for (auto const& node : alTx.getMeta().getNodes()) { @@ -363,40 +316,41 @@ OrderBookDBImpl::processTxn( { if (node.getFieldU16(sfLedgerEntryType) == ltOFFER) { - auto process = [&, this](SField const& field) { + auto extract = [&](SField const& field) { if (auto data = dynamic_cast(node.peekAtPField(field)); data && data->isFieldPresent(sfTakerPays) && data->isFieldPresent(sfTakerGets)) { - auto listeners = getBookListeners( - {data->getFieldAmount(sfTakerGets).asset(), - data->getFieldAmount(sfTakerPays).asset(), - (*data)[~sfDomainID]}); - if (listeners) - listeners->publish(jvObj, havePublished); + result.emplace( + data->getFieldAmount(sfTakerGets).asset(), + data->getFieldAmount(sfTakerPays).asset(), + (*data)[~sfDomainID]); } }; - // We need a field that contains the TakerGets and TakerPays - // parameters. if (node.getFName() == sfModifiedNode) { - process(sfPreviousFields); + extract(sfPreviousFields); } else if (node.getFName() == sfCreatedNode) { - process(sfNewFields); + extract(sfNewFields); } else if (node.getFName() == sfDeletedNode) { - process(sfFinalFields); + extract(sfFinalFields); } } } catch (std::exception const& ex) { - JLOG(j_.info()) << "processTxn: field not found (" << ex.what() << ")"; + // The bad node is skipped; other affected books in the same + // transaction are still returned. Logged at warn so a malformed + // offer node is visible to operators. + JLOG(j.warn()) << "affectedBooks: skipping malformed node (" << ex.what() << ")"; } } + + return result; } } // namespace xrpl diff --git a/src/xrpld/app/ledger/OrderBookDBImpl.h b/src/xrpld/app/ledger/OrderBookDBImpl.h index a50f512441..a68f63c043 100644 --- a/src/xrpld/app/ledger/OrderBookDBImpl.h +++ b/src/xrpld/app/ledger/OrderBookDBImpl.h @@ -1,10 +1,7 @@ #pragma once #include -#include -#include #include -#include #include #include @@ -54,18 +51,6 @@ public: void update(std::shared_ptr const& ledger); - // see if this txn effects any orderbook - void - processTxn( - std::shared_ptr const& ledger, - AcceptedLedgerTx const& alTx, - MultiApiJson const& jvObj) override; - - BookListeners::pointer - getBookListeners(Book const&) override; - BookListeners::pointer - makeBookListeners(Book const&) override; - private: std::reference_wrapper registry_; int const pathSearchMax_; @@ -84,10 +69,6 @@ private: std::recursive_mutex lock_; - using BookToListenersMap = hash_map; - - BookToListenersMap listeners_; - std::atomic seq_; beast::Journal const j_; diff --git a/src/xrpld/app/misc/NetworkOPs.cpp b/src/xrpld/app/misc/NetworkOPs.cpp index 12c79b821c..e59840befc 100644 --- a/src/xrpld/app/misc/NetworkOPs.cpp +++ b/src/xrpld/app/misc/NetworkOPs.cpp @@ -527,6 +527,8 @@ public: updateLocalTx(ReadView const& view) override; std::size_t getLocalTxCount() override; + std::size_t + getBookSubscribersCount() override; // // Monitoring: publisher side. @@ -586,7 +588,9 @@ public: bool subBook(InfoSub::ref ispListener, Book const&) override; bool - unsubBook(std::uint64_t uListener, Book const&) override; + unsubBook(InfoSub::ref ispListener, Book const&) override; + bool + unsubBookInternal(std::uint64_t uListener, Book const&) override; bool subManifests(InfoSub::ref ispListener) override; @@ -629,6 +633,12 @@ public: bool tryRemoveRpcSub(std::string const& strUrl) override; + beast::Journal const& + journal() const override + { + return journal_; + } + void stop() override { @@ -705,6 +715,32 @@ private: AcceptedLedgerTx const& transaction, bool last); + /** + * Fan transaction notifications out to all book subscribers. + * + * Extracts the set of order books affected by @p transaction, then + * delivers @p jvObj to every live subscriber of those books. + * + * Uses a two-pass design to keep subLock_ hold time short: + * 1. Under subLock_, collect strong InfoSub pointers for all live + * subscribers and prune any expired weak_ptrs encountered. + * 2. Release subLock_, then call send() on each collected pointer. + * + * @param transaction The accepted ledger transaction to inspect. + * @param jvObj JSON representation of the transaction to deliver. + * + * @note Thread-safety: acquires subLock_ for the collection pass only. + * send() is intentionally called outside the lock to avoid blocking + * all other sub/unsub/publish paths while I/O is in progress. + * @note Contention: subLock_ is shared with all other subscription types. + * On high-throughput nodes processing multi-hop payments that touch + * many offer nodes, this pass holds subLock_ longer than the old + * per-book BookListeners locks did. This is an accepted trade-off + * for lock-domain simplicity. + */ + void + pubBookTransaction(AcceptedLedgerTx const& transaction, MultiApiJson const& jvObj); + void pubProposedAccountTransaction( std::shared_ptr const& ledger, @@ -802,8 +838,19 @@ private: LedgerMaster& ledgerMaster_; + /** Maps each order book to its current set of subscribers. + * Outer key: the Book (currency pair + optional domain). + * Inner key: InfoSub::seq (unique per connection). + * Inner value: weak_ptr so that a dropped connection does not prevent + * the InfoSub from being destroyed; expired entries are pruned lazily + * by pubBookTransaction and eagerly by unsubBookInternal (~InfoSub path). + * Guarded by subLock_. + */ + using SubBookMapType = hash_map; + SubInfoMapType subAccount_; SubInfoMapType subRTAccount_; + SubBookMapType subBook_; ///< Guarded by subLock_. subRpcMapType rpcSubMap_; @@ -3191,6 +3238,16 @@ NetworkOPsImp::getLocalTxCount() return localTX_->size(); } +std::size_t +NetworkOPsImp::getBookSubscribersCount() +{ + std::scoped_lock const sl(subLock_); + std::size_t total = 0; + for (auto const& [_, subs] : subBook_) + total += subs.size(); + return total; +} + // This routine should only be used to publish accepted or validated // transactions. MultiApiJson @@ -3352,11 +3409,89 @@ NetworkOPsImp::pubValidatedTransaction( } if (transaction.getResult() == tesSUCCESS) - registry_.get().getOrderBookDB().processTxn(ledger, transaction, jvObj); + pubBookTransaction(transaction, jvObj); pubAccountTransaction(ledger, transaction, last); } +void +NetworkOPsImp::pubBookTransaction(AcceptedLedgerTx const& alTx, MultiApiJson const& jvObj) +{ + auto const books = affectedBooks(alTx, journal_); + if (books.empty()) + return; + + // Two-pass design: + // + // 1. Under subLock_, walk subBook_, collect a strong pointer for each + // unique listener (and prune any expired weak_ptrs we encounter). + // 2. Release subLock_, then send to each collected listener. + // + // Reasoning: + // * send() can be slow / blocking, so holding subLock_ across it would + // stall every other sub/unsub/pub path on this server (see the matching + // TODO above pubServer at line ~2275). + // * A strong pointer destructed while subLock_ is held risks running + // ~InfoSub() in-line, which re-enters unsubBook() and mutates the very + // subBook_/SubMapType being iterated -> dangling iterator UB. + // + // Releasing subLock_ before any InfoSub::pointer can decay solves both. + // ~InfoSub() reacquires subLock_ via unsubBook() on its own and serializes + // safely with concurrent traffic. + + std::vector listeners; + hash_set seen; + + // Sized for the common case where every affected book has at most + // one subscriber. Multi-subscriber books trigger reallocation, but + // that is rare and the upper-bound estimate (sum of per-book sizes) + // would itself require walking subBook_ twice. + listeners.reserve(books.size()); + seen.reserve(books.size()); + + { + std::scoped_lock const sl(subLock_); + + for (auto const& book : books) + { + auto it = subBook_.find(book); + if (it == subBook_.end()) + continue; + + for (auto sit = it->second.begin(); sit != it->second.end();) + { + if (auto p = sit->second.lock()) + { + // Defensive: subBook_ entries are normally cleared by + // ~InfoSub() -> unsubBook(), so we rarely see expired + // weak_ptrs here. The else branch covers the narrow race + // where the last strong ref is dropped between insertion + // and our lock() call. + if (seen.emplace(p->getSeq()).second) + listeners.emplace_back(std::move(p)); + ++sit; + } + else + { + JLOG(journal_.debug()) + << "pubBookTransaction: pruning expired weak_ptr for seq=" << sit->first; + sit = it->second.erase(sit); + } + } + + if (it->second.empty()) + subBook_.erase(it); + } + } + + for (auto const& p : listeners) + { + jvObj.visit(p->getApiVersion(), [&](json::Value const& jv) { p->send(jv, true); }); + } + // listeners destructs here, outside subLock_; ~InfoSub (if any fires) + // will reacquire subLock_ via unsubBook with no iterator hazard. +} + void NetworkOPsImp::pubAccountTransaction( std::shared_ptr const& ledger, @@ -4010,26 +4145,39 @@ NetworkOPsImp::unsubAccountHistoryInternal( bool NetworkOPsImp::subBook(InfoSub::ref isrListener, Book const& book) { - if (auto listeners = registry_.get().getOrderBookDB().makeBookListeners(book)) + // Server-side insert first, then InfoSub bookkeeping. If the InfoSub-side + // insert throws, the orphan in subBook_ is cleared by the expired-weak_ptr + // prune in pubBookTransaction. With the reverse ordering, ~InfoSub would + // call unsubBookInternal for a key that was never inserted server-side. { - listeners->addSubscriber(isrListener); - } - else - { - // LCOV_EXCL_START - UNREACHABLE("xrpl::NetworkOPsImp::subBook : null book listeners"); - // LCOV_EXCL_STOP + std::scoped_lock const sl(subLock_); + subBook_[book].try_emplace(isrListener->getSeq(), isrListener); } + isrListener->insertBookSubscription(book); return true; } bool -NetworkOPsImp::unsubBook(std::uint64_t uSeq, Book const& book) +NetworkOPsImp::unsubBook(InfoSub::ref isrListener, Book const& book) { - if (auto listeners = registry_.get().getOrderBookDB().getBookListeners(book)) - listeners->removeSubscriber(uSeq); + // Mirrors unsubAccount: clear the per-subscriber tracking set first so + // ~InfoSub does not re-issue an unsubBookInternal for a book the caller + // already removed, then erase the server-side entry. + isrListener->deleteBookSubscription(book); + return unsubBookInternal(isrListener->getSeq(), book); +} - return true; +bool +NetworkOPsImp::unsubBookInternal(std::uint64_t uSeq, Book const& book) +{ + std::scoped_lock const sl(subLock_); + auto it = subBook_.find(book); + if (it == subBook_.end()) + return false; + bool const erased = it->second.erase(uSeq) != 0u; + if (it->second.empty()) + subBook_.erase(it); + return erased; } std::uint32_t diff --git a/src/xrpld/rpc/handlers/subscribe/Unsubscribe.cpp b/src/xrpld/rpc/handlers/subscribe/Unsubscribe.cpp index 36dae615b3..af42af2a55 100644 --- a/src/xrpld/rpc/handlers/subscribe/Unsubscribe.cpp +++ b/src/xrpld/rpc/handlers/subscribe/Unsubscribe.cpp @@ -186,13 +186,23 @@ doUnsubscribe(RPC::JsonContext& context) book.domain = domain; } - context.netOps.unsubBook(ispSub->getSeq(), book); + if (!context.netOps.unsubBook(ispSub, book)) + { + JLOG(context.j.debug()) + << "doUnsubscribe: book not subscribed (no-op for seq=" << ispSub->getSeq() + << ")"; + } // both_sides is deprecated. if ((jv.isMember(jss::both) && jv[jss::both].asBool()) || (jv.isMember(jss::both_sides) && jv[jss::both_sides].asBool())) { - context.netOps.unsubBook(ispSub->getSeq(), reversed(book)); + if (!context.netOps.unsubBook(ispSub, reversed(book))) + { + JLOG(context.j.debug()) + << "doUnsubscribe: reversed book not subscribed (no-op for seq=" + << ispSub->getSeq() << ")"; + } } } }