From b4175d73eac2b313b89d96571d64dee2323e992f Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Thu, 13 Dec 2012 17:15:41 -0800 Subject: [PATCH 1/8] Fix the shutdown deadlock and a few other problems. Don't wait for SSL shutdown in the websocketpp strand. --- src/cpp/websocketpp/src/sockets/tls.hpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/cpp/websocketpp/src/sockets/tls.hpp b/src/cpp/websocketpp/src/sockets/tls.hpp index 1cdbf40c2f..c20d8a58e2 100644 --- a/src/cpp/websocketpp/src/sockets/tls.hpp +++ b/src/cpp/websocketpp/src/sockets/tls.hpp @@ -51,7 +51,10 @@ public: boost::asio::io_service& get_io_service() { return m_io_service; } - + + static void dummy_handler(const boost::system::error_code&) { + } + // should be private friended? tls_socket::handshake_type get_handshake_type() { if (static_cast< endpoint_type* >(this)->is_server()) { @@ -137,7 +140,7 @@ public: bool shutdown() { boost::system::error_code ignored_ec; - m_socket_ptr->shutdown(ignored_ec); + m_socket_ptr->async_shutdown(dummy_handler); // don't block on SSL shutdown DJS if (ignored_ec) { return false; From a23788de778eb925d343b253f813ba100c93cf35 Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 14 Dec 2012 08:51:33 -0800 Subject: [PATCH 2/8] Fix a small memory leak. --- src/cpp/ripple/WSHandler.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/cpp/ripple/WSHandler.h b/src/cpp/ripple/WSHandler.h index e7ecee3b86..65bfa5253a 100644 --- a/src/cpp/ripple/WSHandler.h +++ b/src/cpp/ripple/WSHandler.h @@ -141,10 +141,12 @@ public: boost::shared_ptr< WSConnection > conn; { boost::mutex::scoped_lock sl(mMapLock); - conn = mMap[cpClient]; + typedef boost::shared_ptr< WSConnection > wsc_ptr; + typename boost::unordered_map::iterator it = mMap.find(cpClient); + if (it == mMap.end()) + return; + conn = it->second; } - if (!conn) - return; send(cpClient, conn->invokeCommand(jvRequest)); } } From f5e70fdcf34b6abc003906367218c395be9248db Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 14 Dec 2012 09:55:52 -0800 Subject: [PATCH 3/8] Make sure the socket doesn't go away while we're shutting it down asynchronously. --- src/cpp/websocketpp/src/sockets/tls.hpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/cpp/websocketpp/src/sockets/tls.hpp b/src/cpp/websocketpp/src/sockets/tls.hpp index c20d8a58e2..156901a4ec 100644 --- a/src/cpp/websocketpp/src/sockets/tls.hpp +++ b/src/cpp/websocketpp/src/sockets/tls.hpp @@ -52,7 +52,7 @@ public: return m_io_service; } - static void dummy_handler(const boost::system::error_code&) { + static void handle_shutdown(tls_socket_ptr, const boost::system::error_code&) { } // should be private friended? @@ -140,7 +140,13 @@ public: bool shutdown() { boost::system::error_code ignored_ec; - m_socket_ptr->async_shutdown(dummy_handler); // don't block on SSL shutdown DJS + m_socket_ptr->async_shutdown( // Don't block on connection shutdown DJS + boost::bind( + &tls::handle_shutdown, + m_socket_ptr, + boost::asio::placeholders::error + ) + ); if (ignored_ec) { return false; From bc3f64137360764a046c694a559ed4e0fcbae5c8 Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 14 Dec 2012 09:56:28 -0800 Subject: [PATCH 4/8] Track when a node was last accessed. --- src/cpp/ripple/SHAMap.cpp | 1 + src/cpp/ripple/SHAMap.h | 5 +++-- src/cpp/ripple/SHAMapNodes.cpp | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/cpp/ripple/SHAMap.cpp b/src/cpp/ripple/SHAMap.cpp index 58524afab5..bf55ea16c5 100644 --- a/src/cpp/ripple/SHAMap.cpp +++ b/src/cpp/ripple/SHAMap.cpp @@ -144,6 +144,7 @@ SHAMapTreeNode::pointer SHAMap::checkCacheNode(const SHAMapNode& iNode) boost::unordered_map::iterator it = mTNByID.find(iNode); if (it == mTNByID.end()) return SHAMapTreeNode::pointer(); + it->second->touch(mSeq); return it->second; } diff --git a/src/cpp/ripple/SHAMap.h b/src/cpp/ripple/SHAMap.h index e02627bbf2..df464bef13 100644 --- a/src/cpp/ripple/SHAMap.h +++ b/src/cpp/ripple/SHAMap.h @@ -161,7 +161,7 @@ private: uint256 mHash; uint256 mHashes[16]; SHAMapItem::pointer mItem; - uint32 mSeq; + uint32 mSeq, mAccessSeq; TNType mType; bool mFullBelow; @@ -183,7 +183,8 @@ public: // node functions uint32 getSeq() const { return mSeq; } - void setSeq(uint32 s) { mSeq = s; } + void setSeq(uint32 s) { mAccessSeq = mSeq = s; } + void touch(uint32 s) { mAccessSeq = s; } const uint256& getNodeHash() const { return mHash; } TNType getType() const { return mType; } diff --git a/src/cpp/ripple/SHAMapNodes.cpp b/src/cpp/ripple/SHAMapNodes.cpp index f4caccb8bc..a296c54f78 100644 --- a/src/cpp/ripple/SHAMapNodes.cpp +++ b/src/cpp/ripple/SHAMapNodes.cpp @@ -173,8 +173,8 @@ void SHAMapNode::dump() const Log(lsDEBUG) << getString(); } -SHAMapTreeNode::SHAMapTreeNode(uint32 seq, const SHAMapNode& nodeID) : SHAMapNode(nodeID), mHash(0), mSeq(seq), - mType(tnERROR), mFullBelow(false) +SHAMapTreeNode::SHAMapTreeNode(uint32 seq, const SHAMapNode& nodeID) : SHAMapNode(nodeID), mHash(0), + mSeq(seq), mAccessSeq(seq), mType(tnERROR), mFullBelow(false) { } From 0da30fc82e05c49dc3da2dcb641c9b0cad1dbaeb Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 14 Dec 2012 10:10:52 -0800 Subject: [PATCH 5/8] Avoid some extraneous reference count operations. --- src/cpp/ripple/SHAMap.cpp | 40 +++++++++++++++++++++------------------ src/cpp/ripple/SHAMap.h | 20 ++++++++++---------- 2 files changed, 32 insertions(+), 28 deletions(-) diff --git a/src/cpp/ripple/SHAMap.cpp b/src/cpp/ripple/SHAMap.cpp index bf55ea16c5..1ec25eff27 100644 --- a/src/cpp/ripple/SHAMap.cpp +++ b/src/cpp/ripple/SHAMap.cpp @@ -139,16 +139,18 @@ void SHAMap::dirtyUp(std::stack& stack, const uint256& } } -SHAMapTreeNode::pointer SHAMap::checkCacheNode(const SHAMapNode& iNode) +static const SHAMapTreeNode::pointer no_node; + +SHAMapTreeNode::ref SHAMap::checkCacheNode(const SHAMapNode& iNode) { boost::unordered_map::iterator it = mTNByID.find(iNode); if (it == mTNByID.end()) - return SHAMapTreeNode::pointer(); + return no_node; it->second->touch(mSeq); return it->second; } -SHAMapTreeNode::pointer SHAMap::walkTo(const uint256& id, bool modify) +SHAMapTreeNode::ref SHAMap::walkTo(const uint256& id, bool modify) { // walk down to the terminal node for this ID SHAMapTreeNode::pointer inNode = root; @@ -370,42 +372,44 @@ void SHAMap::eraseChildren(SHAMapTreeNode::pointer node) return; } -SHAMapItem::pointer SHAMap::peekFirstItem() +static const SHAMapItem::pointer no_item; + +SHAMapItem::ref SHAMap::peekFirstItem() { boost::recursive_mutex::scoped_lock sl(mLock); SHAMapTreeNode *node = firstBelow(root.get()); if (!node) - return SHAMapItem::pointer(); + return no_item; return node->peekItem(); } -SHAMapItem::pointer SHAMap::peekFirstItem(SHAMapTreeNode::TNType& type) +SHAMapItem::ref SHAMap::peekFirstItem(SHAMapTreeNode::TNType& type) { boost::recursive_mutex::scoped_lock sl(mLock); SHAMapTreeNode *node = firstBelow(root.get()); if (!node) - return SHAMapItem::pointer(); + return no_item; type = node->getType(); return node->peekItem(); } -SHAMapItem::pointer SHAMap::peekLastItem() +SHAMapItem::ref SHAMap::peekLastItem() { boost::recursive_mutex::scoped_lock sl(mLock); SHAMapTreeNode *node = lastBelow(root.get()); if (!node) - return SHAMapItem::pointer(); + return no_item; return node->peekItem(); } -SHAMapItem::pointer SHAMap::peekNextItem(const uint256& id) +SHAMapItem::ref SHAMap::peekNextItem(const uint256& id) { SHAMapTreeNode::TNType type; return peekNextItem(id, type); } -SHAMapItem::pointer SHAMap::peekNextItem(const uint256& id, SHAMapTreeNode::TNType& type) +SHAMapItem::ref SHAMap::peekNextItem(const uint256& id, SHAMapTreeNode::TNType& type) { // Get a pointer to the next item in the tree after a given item - item must be in tree boost::recursive_mutex::scoped_lock sl(mLock); @@ -438,10 +442,10 @@ SHAMapItem::pointer SHAMap::peekNextItem(const uint256& id, SHAMapTreeNode::TNTy } } // must be last item - return SHAMapItem::pointer(); + return no_item; } -SHAMapItem::pointer SHAMap::peekPrevItem(const uint256& id) +SHAMapItem::ref SHAMap::peekPrevItem(const uint256& id) { // Get a pointer to the previous item in the tree after a given item - item must be in tree boost::recursive_mutex::scoped_lock sl(mLock); @@ -467,24 +471,24 @@ SHAMapItem::pointer SHAMap::peekPrevItem(const uint256& id) } } // must be last item - return SHAMapItem::pointer(); + return no_item; } -SHAMapItem::pointer SHAMap::peekItem(const uint256& id) +SHAMapItem::ref SHAMap::peekItem(const uint256& id) { boost::recursive_mutex::scoped_lock sl(mLock); SHAMapTreeNode* leaf = walkToPointer(id); if (!leaf) - return SHAMapItem::pointer(); + return no_item; return leaf->peekItem(); } -SHAMapItem::pointer SHAMap::peekItem(const uint256& id, SHAMapTreeNode::TNType& type) +SHAMapItem::ref SHAMap::peekItem(const uint256& id, SHAMapTreeNode::TNType& type) { boost::recursive_mutex::scoped_lock sl(mLock); SHAMapTreeNode* leaf = walkToPointer(id); if (!leaf) - return SHAMapItem::pointer(); + return no_item; type = leaf->getType(); return leaf->peekItem(); } diff --git a/src/cpp/ripple/SHAMap.h b/src/cpp/ripple/SHAMap.h index df464bef13..5ee3c59a1f 100644 --- a/src/cpp/ripple/SHAMap.h +++ b/src/cpp/ripple/SHAMap.h @@ -347,9 +347,9 @@ protected: void dirtyUp(std::stack& stack, const uint256& target, uint256 prevHash); std::stack getStack(const uint256& id, bool include_nonmatching_leaf, bool partialOk); - SHAMapTreeNode::pointer walkTo(const uint256& id, bool modify); + SHAMapTreeNode::ref walkTo(const uint256& id, bool modify); SHAMapTreeNode* walkToPointer(const uint256& id); - SHAMapTreeNode::pointer checkCacheNode(const SHAMapNode&); + SHAMapTreeNode::ref checkCacheNode(const SHAMapNode&); void returnNode(SHAMapTreeNode::pointer&, bool modify); void trackNewNode(SHAMapTreeNode::pointer&); @@ -396,16 +396,16 @@ public: bool addGiveItem(SHAMapItem::ref, bool isTransaction, bool hasMeta); // save a copy if you only need a temporary - SHAMapItem::pointer peekItem(const uint256& id); - SHAMapItem::pointer peekItem(const uint256& id, SHAMapTreeNode::TNType& type); + SHAMapItem::ref peekItem(const uint256& id); + SHAMapItem::ref peekItem(const uint256& id, SHAMapTreeNode::TNType& type); // traverse functions - SHAMapItem::pointer peekFirstItem(); - SHAMapItem::pointer peekFirstItem(SHAMapTreeNode::TNType& type); - SHAMapItem::pointer peekLastItem(); - SHAMapItem::pointer peekNextItem(const uint256&); - SHAMapItem::pointer peekNextItem(const uint256&, SHAMapTreeNode::TNType& type); - SHAMapItem::pointer peekPrevItem(const uint256&); + SHAMapItem::ref peekFirstItem(); + SHAMapItem::ref peekFirstItem(SHAMapTreeNode::TNType& type); + SHAMapItem::ref peekLastItem(); + SHAMapItem::ref peekNextItem(const uint256&); + SHAMapItem::ref peekNextItem(const uint256&, SHAMapTreeNode::TNType& type); + SHAMapItem::ref peekPrevItem(const uint256&); // comparison/sync functions void getMissingNodes(std::vector& nodeIDs, std::vector& hashes, int max, From efd9e91c514a814bb282f91181b66dd35c6916e8 Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 14 Dec 2012 10:53:22 -0800 Subject: [PATCH 6/8] Cleanups. Remove some redundant checks. --- src/cpp/ripple/SHAMap.cpp | 37 +++++++++++++++---------------------- src/cpp/ripple/SHAMap.h | 2 +- 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/src/cpp/ripple/SHAMap.cpp b/src/cpp/ripple/SHAMap.cpp index 1ec25eff27..738ce4e47c 100644 --- a/src/cpp/ripple/SHAMap.cpp +++ b/src/cpp/ripple/SHAMap.cpp @@ -150,7 +150,7 @@ SHAMapTreeNode::ref SHAMap::checkCacheNode(const SHAMapNode& iNode) return it->second; } -SHAMapTreeNode::ref SHAMap::walkTo(const uint256& id, bool modify) +SHAMapTreeNode::pointer SHAMap::walkTo(const uint256& id, bool modify) { // walk down to the terminal node for this ID SHAMapTreeNode::pointer inNode = root; @@ -173,7 +173,7 @@ SHAMapTreeNode::ref SHAMap::walkTo(const uint256& id, bool modify) } } if (inNode->getTag() != id) - return SHAMapTreeNode::pointer(); + return no_node; if (modify) returnNode(inNode, true); return inNode; @@ -188,8 +188,7 @@ SHAMapTreeNode* SHAMap::walkToPointer(const uint256& id) const uint256& nextHash = inNode->getChildHash(branch); if (nextHash.isZero()) return NULL; inNode = getNodePointer(inNode->getChildNodeID(branch), nextHash); - if (!inNode) - throw SHAMapMissingNode(mType, inNode->getChildNodeID(branch), nextHash, id); + assert(inNode); } return (inNode->getTag() == id) ? inNode : NULL; } @@ -214,11 +213,7 @@ SHAMapTreeNode::pointer SHAMap::getNode(const SHAMapNode& id, const uint256& has return node; } - node = fetchNodeExternal(id, hash); - if (!mTNByID.insert(std::make_pair(id, node)).second) - assert(false); - trackNewNode(node); - return node; + return fetchNodeExternal(id, hash); } SHAMapTreeNode* SHAMap::getNodePointer(const SHAMapNode& id, const uint256& hash) @@ -227,11 +222,7 @@ SHAMapTreeNode* SHAMap::getNodePointer(const SHAMapNode& id, const uint256& hash if (it != mTNByID.end()) return it->second.get(); - SHAMapTreeNode::pointer node = fetchNodeExternal(id, hash); - if (!mTNByID.insert(std::make_pair(id, node)).second) - assert(false); - trackNewNode(node); - return node.get(); + return fetchNodeExternal(id, hash).get(); } void SHAMap::returnNode(SHAMapTreeNode::pointer& node, bool modify) @@ -432,8 +423,7 @@ SHAMapItem::ref SHAMap::peekNextItem(const uint256& id, SHAMapTreeNode::TNType& if (!node->isEmptyBranch(i)) { SHAMapTreeNode *firstNode = getNodePointer(node->getChildNodeID(i), node->getChildHash(i)); - if (!firstNode) - throw std::runtime_error("missing node"); + assert(firstNode); firstNode = firstBelow(firstNode); if (!firstNode) throw std::runtime_error("missing node"); @@ -680,7 +670,8 @@ bool SHAMap::updateGiveItem(SHAMapItem::ref item, bool isTransaction, bool hasMe assert(mState != smsImmutable); std::stack stack = getStack(tag, true, false); - if (stack.empty()) throw std::runtime_error("missing node"); + if (stack.empty()) + throw std::runtime_error("missing node"); SHAMapTreeNode::pointer node = stack.top(); stack.pop(); @@ -735,6 +726,9 @@ SHAMapTreeNode::pointer SHAMap::fetchNodeExternal(const SHAMapNode& id, const ui assert(false); return SHAMapTreeNode::pointer(); } + if (!mTNByID.insert(std::make_pair(id, ret)).second) + assert(false); + trackNewNode(ret); return ret; } catch (...) @@ -756,7 +750,6 @@ void SHAMap::fetchRoot(const uint256& hash) Log(lsTRACE) << "Fetch root SHAMap node " << hash; } root = fetchNodeExternal(SHAMapNode(), hash); - mTNByID[*root] = root; assert(root->getNodeHash() == hash); } @@ -804,7 +797,8 @@ SHAMapTreeNode::pointer SHAMap::getNode(const SHAMapNode& nodeID) boost::recursive_mutex::scoped_lock sl(mLock); SHAMapTreeNode::pointer node = checkCacheNode(nodeID); - if (node) return node; + if (node) + return node; node = root; while (nodeID != *node) @@ -815,7 +809,7 @@ SHAMapTreeNode::pointer SHAMap::getNode(const SHAMapNode& nodeID) return SHAMapTreeNode::pointer(); node = getNode(node->getChildNodeID(branch), node->getChildHash(branch), false); - if (!node) throw std::runtime_error("missing node"); + assert(node); } return node; } @@ -838,8 +832,7 @@ bool SHAMap::getPath(const uint256& index, std::vector< std::vectorisEmptyBranch(branch)) // paths leads to empty branch return false; inNode = getNodePointer(inNode->getChildNodeID(branch), inNode->getChildHash(branch)); - if (!inNode) - throw SHAMapMissingNode(mType, inNode->getChildNodeID(branch), inNode->getChildHash(branch), index); + assert(inNode); } if (inNode->getTag() != index) // path leads to different leaf diff --git a/src/cpp/ripple/SHAMap.h b/src/cpp/ripple/SHAMap.h index 5ee3c59a1f..e7d4565588 100644 --- a/src/cpp/ripple/SHAMap.h +++ b/src/cpp/ripple/SHAMap.h @@ -347,7 +347,7 @@ protected: void dirtyUp(std::stack& stack, const uint256& target, uint256 prevHash); std::stack getStack(const uint256& id, bool include_nonmatching_leaf, bool partialOk); - SHAMapTreeNode::ref walkTo(const uint256& id, bool modify); + SHAMapTreeNode::pointer walkTo(const uint256& id, bool modify); SHAMapTreeNode* walkToPointer(const uint256& id); SHAMapTreeNode::ref checkCacheNode(const SHAMapNode&); void returnNode(SHAMapTreeNode::pointer&, bool modify); From 534ce8307ff114bd5436c6d820e14b1d20ed0c70 Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 14 Dec 2012 12:07:50 -0800 Subject: [PATCH 7/8] Have to special-case the root node. --- src/cpp/ripple/SHAMap.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cpp/ripple/SHAMap.cpp b/src/cpp/ripple/SHAMap.cpp index 738ce4e47c..4c83e1f0f6 100644 --- a/src/cpp/ripple/SHAMap.cpp +++ b/src/cpp/ripple/SHAMap.cpp @@ -726,7 +726,9 @@ SHAMapTreeNode::pointer SHAMap::fetchNodeExternal(const SHAMapNode& id, const ui assert(false); return SHAMapTreeNode::pointer(); } - if (!mTNByID.insert(std::make_pair(id, ret)).second) + if (id.isRoot()) + mTNByID[id] = ret; + else if (!mTNByID.insert(std::make_pair(id, ret)).second) assert(false); trackNewNode(ret); return ret; From fa4ff04d06b6c55031643f2a4c576d2a6a1aa9dd Mon Sep 17 00:00:00 2001 From: JoelKatz Date: Fri, 14 Dec 2012 16:18:32 -0800 Subject: [PATCH 8/8] Broaden the integer multiplication testing. --- src/cpp/ripple/Amount.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/cpp/ripple/Amount.cpp b/src/cpp/ripple/Amount.cpp index d7ed98d866..c1e4416214 100644 --- a/src/cpp/ripple/Amount.cpp +++ b/src/cpp/ripple/Amount.cpp @@ -1446,7 +1446,7 @@ static void mulTest(int a, int b) if (prod1.isNative()) BOOST_FAIL("product is native"); - STAmount prod2(CURRENCY_ONE, ACCOUNT_ONE, a * b); + STAmount prod2(CURRENCY_ONE, ACCOUNT_ONE, static_cast(a) * static_cast(b)); if (prod1 != prod2) { Log(lsWARNING) << "nn(" << aa.getFullText() << " * " << bb.getFullText() << ") = " << prod1.getFullText() @@ -1489,8 +1489,8 @@ BOOST_AUTO_TEST_CASE( CurrencyMulDivTests ) roundTest(1, 3, 3); roundTest(2, 3, 9); roundTest(1, 7, 21); roundTest(1, 2, 4); roundTest(3, 9, 18); roundTest(7, 11, 44); - mulTest(100, 1000); mulTest(1, 2); mulTest(32, 15); mulTest(981, 4012); - mulTest(100, 5); + for (int i = 0; i <= 100000; ++i) + mulTest(rand() % 10000000, rand() % 10000000); } BOOST_AUTO_TEST_SUITE_END()