From b0781622b2a2d85cb8fd9b040eb9301548fd9c34 Mon Sep 17 00:00:00 2001 From: Scott Determan Date: Wed, 7 Jan 2015 17:35:40 -0500 Subject: [PATCH] Handle nullptr return values to InboundLedgers::findCreate In normal operation, InboundLedgers::findCreate never returns null, but during system shutdown, it will return null. Since this only happens in system shutdown, when findCreate returns null the calling function stops what it was doing and returns. During testing, an issue where destroying the application object and creating a new one caused problems with a static PathTable. This table is now cleared when re-initialized. --- src/ripple/app/ledger/LedgerMaster.cpp | 26 ++++++++++++++- src/ripple/app/main/Main.cpp | 40 +++++++++++++++++++++-- src/ripple/app/paths/Pathfinder.cpp | 5 +-- src/ripple/rpc/handlers/LedgerRequest.cpp | 27 ++++++++++----- 4 files changed, 83 insertions(+), 15 deletions(-) diff --git a/src/ripple/app/ledger/LedgerMaster.cpp b/src/ripple/app/ledger/LedgerMaster.cpp index cd5f73330..f0794ff47 100644 --- a/src/ripple/app/ledger/LedgerMaster.cpp +++ b/src/ripple/app/ledger/LedgerMaster.cpp @@ -966,6 +966,14 @@ public: getApp().getInboundLedgers().findCreate(nextLedger->getParentHash(), nextLedger->getLedgerSeq() - 1, InboundLedger::fcHISTORY); + if (!acq) + { + // On system shutdown, findCreate may return a nullptr + WriteLog (lsTRACE, LedgerMaster) + << "findCreate failed to return an inbound ledger"; + return; + } + if (acq->isComplete() && !acq->isFailed()) ledger = acq->getLedger(); else if ((missing > 40000) && getApp().getOPs().shouldFetchPack(missing)) @@ -1117,8 +1125,15 @@ public: InboundLedger::pointer acq = getApp().getInboundLedgers ().findCreate (hash, seq, InboundLedger::fcGENERIC); - if (!acq->isDone ()) + if (!acq) { + // On system shutdown, findCreate may return a nullptr + WriteLog (lsTRACE, LedgerMaster) + << "findCreate failed to return an inbound ledger"; + return {}; + } + + if (!acq->isDone()) { } else if (acq->isComplete () && !acq->isFailed ()) { @@ -1129,6 +1144,15 @@ public: WriteLog (lsWARNING, LedgerMaster) << "Failed to acquire a published ledger"; getApp().getInboundLedgers().dropLedger(hash); acq = getApp().getInboundLedgers().findCreate(hash, seq, InboundLedger::fcGENERIC); + + if (!acq) + { + // On system shutdown, findCreate may return a nullptr + WriteLog (lsTRACE, LedgerMaster) + << "findCreate failed to return an inbound ledger"; + return {}; + } + if (acq->isComplete()) { if (acq->isFailed()) diff --git a/src/ripple/app/main/Main.cpp b/src/ripple/app/main/Main.cpp index dfd0e84bf..784ca5bd0 100644 --- a/src/ripple/app/main/Main.cpp +++ b/src/ripple/app/main/Main.cpp @@ -40,6 +40,7 @@ #include #include #include +#include #if defined(BEAST_LINUX) || defined(BEAST_MAC) || defined(BEAST_BSD) #include @@ -154,9 +155,35 @@ setupConfigForUnitTests (Config* config) config->importNodeDatabase = beast::StringPairArray (); } -static -int -runUnitTests(std::string const& pattern, std::string const& argument) +static int runShutdownTests () +{ + // Shutdown tests can not be part of the normal unit tests in 'runUnitTests' + // because it needs to create and destroy an application object. + int const numShutdownIterations = 20; + // Give it enough time to sync and run a bit while synced. + std::chrono::seconds const serverUptimePerIteration (4 * 60); + for (int i = 0; i < numShutdownIterations; ++i) + { + std::cerr << "\n\nStarting server. Iteration: " << i << "\n" + << std::endl; + std::unique_ptr app (make_Application (deprecatedLogs())); + auto shutdownApp = [&app](std::chrono::seconds sleepTime, int iteration) + { + std::this_thread::sleep_for (sleepTime); + std::cerr << "\n\nStopping server. Iteration: " << iteration << "\n" + << std::endl; + app->signalStop(); + }; + std::thread shutdownThread (shutdownApp, serverUptimePerIteration, i); + setupServer(); + startServer(); + shutdownThread.join(); + } + return EXIT_SUCCESS; +} + +static int runUnitTests (std::string const& pattern, + std::string const& argument) { // Config needs to be set up before creating Application setupConfigForUnitTests (&getConfig ()); @@ -208,6 +235,7 @@ int run (int argc, char** argv) ("rpc_ip", po::value (), "Specify the IP address for RPC command. Format: [':']") ("rpc_port", po::value (), "Specify the port number for RPC command.") ("standalone,a", "Run with no peers.") + ("shutdowntest", po::value ()->implicit_value (""), "Perform shutdown tests.") ("unittest,u", po::value ()->implicit_value (""), "Perform unit tests.") ("unittest-arg", po::value ()->implicit_value (""), "Supplies argument to unit tests.") ("parameters", po::value< vector > (), "Specify comma separated parameters.") @@ -269,6 +297,7 @@ int run (int argc, char** argv) && !vm.count ("parameters") && !vm.count ("fg") && !vm.count ("standalone") + && !vm.count ("shutdowntest") && !vm.count ("unittest")) { std::string logMe = DoSustain (getConfig ().getDebugLogFile ().string()); @@ -384,6 +413,11 @@ int run (int argc, char** argv) } } + if (vm.count ("shutdowntest")) + { + return runShutdownTests (); + } + if (iResult == 0) { if (!vm.count ("parameters")) diff --git a/src/ripple/app/paths/Pathfinder.cpp b/src/ripple/app/paths/Pathfinder.cpp index 7c48c928a..a912c44a3 100644 --- a/src/ripple/app/paths/Pathfinder.cpp +++ b/src/ripple/app/paths/Pathfinder.cpp @@ -1185,8 +1185,9 @@ void fillPaths (Pathfinder::PaymentType type, PathCostList const& costs) void Pathfinder::initPathTable () { // CAUTION: Do not include rules that build default paths - fillPaths( - pt_XRP_to_XRP, {}); + + mPathTable.clear(); + fillPaths (pt_XRP_to_XRP, {}); fillPaths( pt_XRP_to_nonXRP, { diff --git a/src/ripple/rpc/handlers/LedgerRequest.cpp b/src/ripple/rpc/handlers/LedgerRequest.cpp index e1a436b3c..62747da68 100644 --- a/src/ripple/rpc/handlers/LedgerRequest.cpp +++ b/src/ripple/rpc/handlers/LedgerRequest.cpp @@ -76,13 +76,18 @@ Json::Value doLedgerRequest (RPC::Context& context) { // We don't have the ledger we need to figure out which ledger // they want. Try to get it. - Json::Value jvResult - = getApp().getInboundLedgers().findCreate ( - refHash, refIndex, InboundLedger::fcGENERIC) - ->getJson (0); - jvResult[jss::error] = "ledgerNotFound"; - return jvResult; + if (auto il = getApp().getInboundLedgers().findCreate ( + refHash, refIndex, InboundLedger::fcGENERIC)) + { + Json::Value jvResult = il->getJson (0); + + jvResult[jss::error] = "ledgerNotFound"; + return jvResult; + } + + // findCreate failed to return an inbound ledger. App is likely shutting down + return Json::Value(); } ledgerHash = ledger->getLedgerHash (ledgerIndex); @@ -102,9 +107,13 @@ Json::Value doLedgerRequest (RPC::Context& context) else { // Try to get the desired ledger - auto il = getApp().getInboundLedgers().findCreate ( - ledgerHash, 0, InboundLedger::fcGENERIC); - return il->getJson (0); + if (auto il = getApp ().getInboundLedgers ().findCreate ( + ledgerHash, 0, InboundLedger::fcGENERIC)) + { + return il->getJson (0); + } + return RPC::make_error ( + rpcNOT_READY, "findCreate failed to return an inbound ledger"); } }