From 96be7f79460d3df70d37682816a1908310e5260a Mon Sep 17 00:00:00 2001 From: Mike Ellery Date: Mon, 8 Apr 2019 12:09:04 -0700 Subject: [PATCH 1/3] Add request timeout for ValidatorSites: Fixes: RIPD-1737 Enforce a 20s timeout for making validator list requests. --- src/ripple/app/misc/ValidatorSite.h | 15 ++- src/ripple/app/misc/impl/ValidatorSite.cpp | 85 ++++++++++--- src/test/app/ValidatorSite_test.cpp | 134 +++++++++++++++------ src/test/jtx/TrustedPublisherServer.h | 35 ++++-- 4 files changed, 202 insertions(+), 67 deletions(-) diff --git a/src/ripple/app/misc/ValidatorSite.h b/src/ripple/app/misc/ValidatorSite.h index d5a995c4e88..c3250d738e6 100644 --- a/src/ripple/app/misc/ValidatorSite.h +++ b/src/ripple/app/misc/ValidatorSite.h @@ -58,7 +58,8 @@ namespace ripple { @li @c "version": 1 - @li @c "refreshInterval" (optional) + @li @c "refreshInterval" (optional, integer minutes). + This value is clamped internally to [1,1440] (1 min - 1 day) */ class ValidatorSite { @@ -125,11 +126,15 @@ class ValidatorSite // The configured list of URIs for fetching lists std::vector sites_; + // time to allow for requests to complete + const std::chrono::seconds requestTimeout_; + public: ValidatorSite ( boost::asio::io_service& ios, ValidatorList& validators, - beast::Journal j); + beast::Journal j, + std::chrono::seconds timeout = std::chrono::seconds{20}); ~ValidatorSite (); /** Load configured site URIs. @@ -187,6 +192,12 @@ class ValidatorSite void setTimer (); + /// request took too long + void + onRequestTimeout ( + std::size_t siteIdx, + error_code const& ec); + /// Fetch site whose time has come void onTimer ( diff --git a/src/ripple/app/misc/impl/ValidatorSite.cpp b/src/ripple/app/misc/impl/ValidatorSite.cpp index 1227607a2f4..b4cd7e87a77 100644 --- a/src/ripple/app/misc/impl/ValidatorSite.cpp +++ b/src/ripple/app/misc/impl/ValidatorSite.cpp @@ -26,15 +26,15 @@ #include #include #include +#include #include #include namespace ripple { -// default site query frequency - 5 minutes -auto constexpr DEFAULT_REFRESH_INTERVAL = std::chrono::minutes{5}; -auto constexpr ERROR_RETRY_INTERVAL = std::chrono::seconds{30}; -unsigned short constexpr MAX_REDIRECTS = 3; +auto constexpr DEFAULT_REFRESH_INTERVAL = std::chrono::minutes{5}; +auto constexpr ERROR_RETRY_INTERVAL = std::chrono::seconds{30}; +unsigned short constexpr MAX_REDIRECTS = 3; ValidatorSite::Site::Resource::Resource (std::string uri_) : uri {std::move(uri_)} @@ -90,7 +90,8 @@ ValidatorSite::Site::Site (std::string uri) ValidatorSite::ValidatorSite ( boost::asio::io_service& ios, ValidatorList& validators, - beast::Journal j) + beast::Journal j, + std::chrono::seconds timeout) : ios_ (ios) , validators_ (validators) , j_ (j) @@ -98,6 +99,7 @@ ValidatorSite::ValidatorSite ( , fetching_ (false) , pending_ (false) , stopping_ (false) + , requestTimeout_ (timeout) { } @@ -168,10 +170,12 @@ ValidatorSite::stop() { std::unique_lock lock{state_mutex_}; stopping_ = true; - cv_.wait(lock, [&]{ return ! fetching_; }); - + // work::cancel() must be called before the + // cv wait in order to kick any asio async operations + // that might be pending. if(auto sp = work_.lock()) sp->cancel(); + cv_.wait(lock, [&]{ return ! fetching_; }); error_code ec; timer_.cancel(ec); @@ -210,17 +214,30 @@ ValidatorSite::makeRequest ( fetching_ = true; sites_[siteIdx].activeResource = resource; std::shared_ptr sp; + auto timeoutCancel = + [this] () + { + std::unique_lock lock_state{state_mutex_}; + error_code ec; + timer_.cancel_one(ec); + }; auto onFetch = - [this, siteIdx] (error_code const& err, detail::response_type&& resp) + [this, siteIdx, timeoutCancel] ( + error_code const& err, detail::response_type&& resp) { + timeoutCancel (); onSiteFetch (err, std::move(resp), siteIdx); }; auto onFetchFile = - [this, siteIdx] (error_code const& err, std::string const& resp) - { - onTextFetch (err, resp, siteIdx); - }; + [this, siteIdx, timeoutCancel] ( + error_code const& err, std::string const& resp) + { + timeoutCancel (); + onTextFetch (err, resp, siteIdx); + }; + + JLOG (j_.debug()) << "Starting request for " << resource->uri; if (resource->pUrl.scheme == "https") { @@ -252,6 +269,32 @@ ValidatorSite::makeRequest ( work_ = sp; sp->run (); + // start a timer for the request, which shouldn't take more + // than requestTimeout_ to complete + std::unique_lock lock_state{state_mutex_}; + timer_.expires_at ( clock_type::now() + requestTimeout_); + timer_.async_wait ( std::bind ( + &ValidatorSite::onRequestTimeout, this, siteIdx, std::placeholders::_1)); +} + +void +ValidatorSite::onRequestTimeout ( + std::size_t siteIdx, + error_code const& ec) +{ + if (ec) + return; + + { + std::unique_lock lock_site{sites_mutex_}; + JLOG (j_.warn()) << + "Request for " << sites_[siteIdx].activeResource->uri << + " took too long"; + } + + std::unique_lock lock_state{state_mutex_}; + if(auto sp = work_.lock()) + sp->cancel(); } void @@ -370,10 +413,14 @@ ValidatorSite::parseJsonResponse ( if (body.isMember ("refresh_interval") && body["refresh_interval"].isNumeric ()) { - // TODO: should we sanity check/clamp this value - // to something reasonable? - sites_[siteIdx].refreshInterval = - std::chrono::minutes{body["refresh_interval"].asUInt ()}; + auto refresh = + boost::algorithm::clamp( + body["refresh_interval"].asUInt (), + 1u, + 60u*24); + sites_[siteIdx].refreshInterval = std::chrono::minutes{refresh}; + sites_[siteIdx].nextRefresh = + clock_type::now() + sites_[siteIdx].refreshInterval; } } @@ -435,6 +482,8 @@ ValidatorSite::onSiteFetch( { { std::lock_guard lock_sites{sites_mutex_}; + JLOG (j_.debug()) << "Got completion for " + << sites_[siteIdx].activeResource->uri; auto onError = [&](std::string const& errMsg, bool retry) { sites_[siteIdx].lastRefreshStatus.emplace( @@ -503,7 +552,7 @@ ValidatorSite::onSiteFetch( sites_[siteIdx].activeResource.reset(); } - std::lock_guard lock_state{state_mutex_}; + std::unique_lock lock_state{state_mutex_}; fetching_ = false; if (! stopping_) setTimer (); @@ -544,7 +593,7 @@ ValidatorSite::onTextFetch( sites_[siteIdx].activeResource.reset(); } - std::lock_guard lock_state{state_mutex_}; + std::unique_lock lock_state{state_mutex_}; fetching_ = false; if (! stopping_) setTimer (); diff --git a/src/test/app/ValidatorSite_test.cpp b/src/test/app/ValidatorSite_test.cpp index 37d8a330e71..ef714a54e57 100644 --- a/src/test/app/ValidatorSite_test.cpp +++ b/src/test/app/ValidatorSite_test.cpp @@ -30,8 +30,10 @@ #include #include #include +#include #include #include +#include #include namespace ripple { @@ -185,12 +187,24 @@ class ValidatorSite_test : public beast::unit_test::suite } }; + struct fetchListConfig + { + std::string path_; + std::string msg_; + bool failFetch_ = false; + bool failApply_ = false; + int serverVersion_ = 1; + std::chrono::seconds expiresFromNow_ = std::chrono::seconds{3600}; + }; void - testFetchList ( - std::vector> const& paths) + testFetchList (std::vector const& paths) { - testcase << "Fetch list - " << paths[0].first << - (paths.size() > 1 ? ", " + paths[1].first : ""); + using M = std::decay::type; + testcase << "Fetch list - " << + boost::algorithm::join (paths | + boost::adaptors::transformed( + [](M::value_type t){ return t.path_; }), + ", "); using namespace jtx; @@ -208,16 +222,14 @@ class ValidatorSite_test : public beast::unit_test::suite std::vector list; std::string uri; std::string expectMsg; - bool shouldFail; + bool failFetch; + bool failApply; bool isRetry; }; std::vector servers; auto const sequence = 1; - auto const version = 1; using namespace std::chrono_literals; - NetClock::time_point const expiration = - env.timeKeeper().now() + 3600s; auto constexpr listSize = 20; std::vector cfgPublishers; @@ -235,9 +247,10 @@ class ValidatorSite_test : public beast::unit_test::suite servers.push_back({}); auto& item = servers.back(); - item.shouldFail = ! cfg.second.empty(); - item.isRetry = cfg.first == "/bad-resource"; - item.expectMsg = cfg.second; + item.failFetch = cfg.failFetch_; + item.failApply = cfg.failApply_; + item.isRetry = cfg.path_ == "/bad-resource"; + item.expectMsg = cfg.msg_; item.list.reserve (listSize); while (item.list.size () < listSize) item.list.push_back (randomValidator()); @@ -247,12 +260,12 @@ class ValidatorSite_test : public beast::unit_test::suite pubSigningKeys, manifest, sequence, - expiration, - version, + env.timeKeeper().now() + cfg.expiresFromNow_, + cfg.serverVersion_, item.list); std::stringstream uri; - uri << "http://" << item.server->local_endpoint() << cfg.first; + uri << "http://" << item.server->local_endpoint() << cfg.path_; item.uri = uri.str(); } @@ -260,7 +273,10 @@ class ValidatorSite_test : public beast::unit_test::suite emptyLocalKey, emptyCfgKeys, cfgPublishers)); auto sites = std::make_unique ( - env.app().getIOService(), env.app().validators(), journal); + env.app().getIOService(), + env.app().validators(), + journal, + 2s); std::vector uris; for (auto const& u : servers) @@ -274,9 +290,9 @@ class ValidatorSite_test : public beast::unit_test::suite for (auto const& val : u.list) { BEAST_EXPECT( - trustedKeys.listed (val.masterPublic) != u.shouldFail); + trustedKeys.listed (val.masterPublic) != u.failApply); BEAST_EXPECT( - trustedKeys.listed (val.signingPublic) != u.shouldFail); + trustedKeys.listed (val.signingPublic) != u.failApply); } auto const jv = sites->getJson(); @@ -286,13 +302,19 @@ class ValidatorSite_test : public beast::unit_test::suite myStatus = vs; BEAST_EXPECTS( myStatus[jss::last_refresh_message].asString().empty() - != u.shouldFail, to_string(myStatus)); - if (u.shouldFail) + != u.failFetch, + to_string(myStatus) + "\n" + sink.strm_.str()); + + if (! u.expectMsg.empty()) { - using namespace std::chrono; BEAST_EXPECTS( sink.strm_.str().find(u.expectMsg) != std::string::npos, sink.strm_.str()); + } + + if (u.failFetch) + { + using namespace std::chrono; log << " -- Msg: " << myStatus[jss::last_refresh_message].asString() << std::endl; std::stringstream nextRefreshStr @@ -412,40 +434,78 @@ class ValidatorSite_test : public beast::unit_test::suite testConfigLoad (); // fetch single site - testFetchList ({{"/validators",""}}); + testFetchList ({{"/validators", ""}}); // fetch multiple sites - testFetchList ({{"/validators",""}, {"/validators",""}}); + testFetchList ({{"/validators", ""}, {"/validators", ""}}); // fetch single site with single redirects - testFetchList ({{"/redirect_once/301",""}}); - testFetchList ({{"/redirect_once/302",""}}); - testFetchList ({{"/redirect_once/307",""}}); - testFetchList ({{"/redirect_once/308",""}}); + testFetchList ({{"/redirect_once/301", ""}}); + testFetchList ({{"/redirect_once/302", ""}}); + testFetchList ({{"/redirect_once/307", ""}}); + testFetchList ({{"/redirect_once/308", ""}}); // one redirect, one not - testFetchList ({{"/validators",""}, {"/redirect_once/302",""}}); + testFetchList ({{"/validators", ""}, {"/redirect_once/302", ""}}); // fetch single site with undending redirect (fails to load) - testFetchList ({{"/redirect_forever/301", "Exceeded max redirects"}}); + testFetchList ({ + {"/redirect_forever/301", "Exceeded max redirects", true, true}}); // two that redirect forever testFetchList ({ - {"/redirect_forever/307","Exceeded max redirects"}, - {"/redirect_forever/308","Exceeded max redirects"}}); + {"/redirect_forever/307", "Exceeded max redirects", true, true}, + {"/redirect_forever/308", "Exceeded max redirects", true, true}}); // one undending redirect, one not testFetchList ( - {{"/validators",""}, - {"/redirect_forever/302","Exceeded max redirects"}}); + {{"/validators", ""}, + {"/redirect_forever/302", "Exceeded max redirects", true, true}}); // invalid redir Location testFetchList ({ {"/redirect_to/ftp://invalid-url/302", - "Invalid redirect location"}}); + "Invalid redirect location", + true, + true}}); + testFetchList ({ + {"/redirect_to/file://invalid-url/302", + "Invalid redirect location", + true, + true}}); // invalid json - testFetchList ({{"/validators/bad", "Unable to parse JSON response"}}); + testFetchList ({ + {"/validators/bad", "Unable to parse JSON response", true, true}}); // error status returned - testFetchList ({{"/bad-resource", "returned bad status"}}); + testFetchList ({ + {"/bad-resource", "returned bad status", true, true}}); // location field missing testFetchList ({ - {"/redirect_nolo/308", "returned a redirect with no Location"}}); + {"/redirect_nolo/308", + "returned a redirect with no Location", + true, + true}}); // json fields missing testFetchList ({ - {"/validators/missing", "Missing fields in JSON response"}}); + {"/validators/missing", + "Missing fields in JSON response", + true, + true}}); + // timeout + testFetchList ({ + {"/sleep/3", "took too long", true, true}}); + // bad manifest version + testFetchList ({ + {"/validators", "Unsupported version", false, true, 4}}); + // get old validator list + testFetchList ({ + {"/validators", + "Stale validator list", + false, + true, + 1, + std::chrono::seconds{0}}}); + // force an out-of-range expiration value + testFetchList ({ + {"/validators", + "Invalid validator list", + false, + true, + 1, + std::chrono::seconds{Json::Value::maxInt + 1}}}); testFileURLs(); } }; diff --git a/src/test/jtx/TrustedPublisherServer.h b/src/test/jtx/TrustedPublisherServer.h index cd47cbd3685..784de5e3c11 100644 --- a/src/test/jtx/TrustedPublisherServer.h +++ b/src/test/jtx/TrustedPublisherServer.h @@ -28,6 +28,8 @@ #include #include #include +#include +#include namespace ripple { namespace test { @@ -44,8 +46,7 @@ class TrustedPublisherServer socket_type sock_; boost::asio::ip::tcp::acceptor acceptor_; - - std::string list_; + std::function getList_; public: @@ -82,14 +83,16 @@ class TrustedPublisherServer data.pop_back(); data += "]}"; std::string blob = base64_encode(data); - - list_ = "{\"blob\":\"" + blob + "\""; - auto const sig = sign(keys.first, keys.second, makeSlice(data)); - - list_ += ",\"signature\":\"" + strHex(sig) + "\""; - list_ += ",\"manifest\":\"" + manifest + "\""; - list_ += ",\"version\":" + std::to_string(version) + '}'; + getList_ = [blob, sig, manifest, version](int interval) { + std::stringstream l; + l << "{\"blob\":\"" << blob << "\"" << + ",\"signature\":\"" << strHex(sig) << "\"" << + ",\"manifest\":\"" << manifest << "\"" << + ",\"refresh_interval\": " << interval << + ",\"version\":" << version << '}'; + return l.str(); + }; acceptor_.open(ep.protocol()); error_code ec; @@ -181,7 +184,19 @@ class TrustedPublisherServer else if (path == "/validators/missing") res.body() = "{\"version\": 1}"; else - res.body() = list_; + { + int refresh = 5; + if (boost::starts_with(path, "/validators/refresh")) + refresh = + boost::lexical_cast(path.substr(20)); + res.body() = getList_(refresh); + } + } + else if (boost::starts_with(path, "/sleep/")) + { + auto sleep_sec = + boost::lexical_cast(path.substr(7)); + std::this_thread::sleep_for(std::chrono::seconds{sleep_sec}); } else if (boost::starts_with(path, "/redirect")) { From 5e2c8ff0e5c7a0209e972ffc7ce721bde307ef86 Mon Sep 17 00:00:00 2001 From: Mike Ellery Date: Sun, 14 Apr 2019 07:03:21 -0700 Subject: [PATCH 2/3] [FOLD] Review fixes --- src/ripple/app/misc/ValidatorSite.h | 3 +- src/ripple/app/misc/impl/ValidatorSite.cpp | 80 +++++++------- src/test/app/ValidatorSite_test.cpp | 99 ++++++++++------- src/test/jtx/TrustedPublisherServer.h | 118 +++++++++++---------- 4 files changed, 166 insertions(+), 134 deletions(-) diff --git a/src/ripple/app/misc/ValidatorSite.h b/src/ripple/app/misc/ValidatorSite.h index c3250d738e6..86fa7b3edb0 100644 --- a/src/ripple/app/misc/ValidatorSite.h +++ b/src/ripple/app/misc/ValidatorSite.h @@ -189,8 +189,9 @@ class ValidatorSite private: /// Queue next site to be fetched + /// lock over state_mutex_ required void - setTimer (); + setTimer (std::lock_guard&); /// request took too long void diff --git a/src/ripple/app/misc/impl/ValidatorSite.cpp b/src/ripple/app/misc/impl/ValidatorSite.cpp index b4cd7e87a77..8910d7ccbc3 100644 --- a/src/ripple/app/misc/impl/ValidatorSite.cpp +++ b/src/ripple/app/misc/impl/ValidatorSite.cpp @@ -32,9 +32,9 @@ namespace ripple { -auto constexpr DEFAULT_REFRESH_INTERVAL = std::chrono::minutes{5}; -auto constexpr ERROR_RETRY_INTERVAL = std::chrono::seconds{30}; -unsigned short constexpr MAX_REDIRECTS = 3; +auto constexpr default_refresh_interval = std::chrono::minutes{5}; +auto constexpr error_retry_interval = std::chrono::seconds{30}; +unsigned short constexpr max_redirects = 3; ValidatorSite::Site::Resource::Resource (std::string uri_) : uri {std::move(uri_)} @@ -82,7 +82,7 @@ ValidatorSite::Site::Site (std::string uri) : loadedResource {std::make_shared(std::move(uri))} , startingResource {loadedResource} , redirCount {0} - , refreshInterval {DEFAULT_REFRESH_INTERVAL} + , refreshInterval {default_refresh_interval} , nextRefresh {clock_type::now()} { } @@ -155,7 +155,7 @@ ValidatorSite::start () { std::lock_guard lock{state_mutex_}; if (timer_.expires_at() == clock_type::time_point{}) - setTimer (); + setTimer (lock); } void @@ -177,15 +177,14 @@ ValidatorSite::stop() sp->cancel(); cv_.wait(lock, [&]{ return ! fetching_; }); - error_code ec; - timer_.cancel(ec); + timer_.cancel(); stopping_ = false; pending_ = false; cv_.notify_all(); } void -ValidatorSite::setTimer () +ValidatorSite::setTimer (std::lock_guard& state_lock) { std::lock_guard lock{sites_mutex_}; @@ -200,8 +199,11 @@ ValidatorSite::setTimer () pending_ = next->nextRefresh <= clock_type::now(); cv_.notify_all(); timer_.expires_at (next->nextRefresh); - timer_.async_wait (std::bind (&ValidatorSite::onTimer, this, - std::distance (sites_.begin (), next), std::placeholders::_1)); + auto idx = std::distance (sites_.begin (), next); + timer_.async_wait ([this, idx] (boost::system::error_code const& ec) + { + this->onTimer (idx, ec); + }); } } @@ -209,7 +211,7 @@ void ValidatorSite::makeRequest ( std::shared_ptr resource, std::size_t siteIdx, - std::lock_guard& lock) + std::lock_guard& sites_lock) { fetching_ = true; sites_[siteIdx].activeResource = resource; @@ -217,9 +219,8 @@ ValidatorSite::makeRequest ( auto timeoutCancel = [this] () { - std::unique_lock lock_state{state_mutex_}; - error_code ec; - timer_.cancel_one(ec); + std::lock_guard lock_state{state_mutex_}; + timer_.cancel_one(); }; auto onFetch = [this, siteIdx, timeoutCancel] ( @@ -271,10 +272,12 @@ ValidatorSite::makeRequest ( sp->run (); // start a timer for the request, which shouldn't take more // than requestTimeout_ to complete - std::unique_lock lock_state{state_mutex_}; - timer_.expires_at ( clock_type::now() + requestTimeout_); - timer_.async_wait ( std::bind ( - &ValidatorSite::onRequestTimeout, this, siteIdx, std::placeholders::_1)); + std::lock_guard lock_state{state_mutex_}; + timer_.expires_after (requestTimeout_); + timer_.async_wait ([this, siteIdx] (boost::system::error_code const& ec) + { + this->onRequestTimeout (siteIdx, ec); + }); } void @@ -286,13 +289,13 @@ ValidatorSite::onRequestTimeout ( return; { - std::unique_lock lock_site{sites_mutex_}; + std::lock_guard lock_site{sites_mutex_}; JLOG (j_.warn()) << "Request for " << sites_[siteIdx].activeResource->uri << " took too long"; } - std::unique_lock lock_state{state_mutex_}; + std::lock_guard lock_state{state_mutex_}; if(auto sp = work_.lock()) sp->cancel(); } @@ -311,14 +314,12 @@ ValidatorSite::onTimer ( return; } - std::lock_guard lock{sites_mutex_}; - sites_[siteIdx].nextRefresh = - clock_type::now() + sites_[siteIdx].refreshInterval; - - assert(! fetching_); - sites_[siteIdx].redirCount = 0; try { + std::lock_guard lock{sites_mutex_}; + sites_[siteIdx].nextRefresh = + clock_type::now() + sites_[siteIdx].refreshInterval; + sites_[siteIdx].redirCount = 0; // the WorkSSL client can throw if SSL init fails makeRequest(sites_[siteIdx].startingResource, siteIdx, lock); } @@ -335,7 +336,7 @@ void ValidatorSite::parseJsonResponse ( std::string const& res, std::size_t siteIdx, - std::lock_guard& lock) + std::lock_guard& sites_lock) { Json::Reader r; Json::Value body; @@ -413,12 +414,13 @@ ValidatorSite::parseJsonResponse ( if (body.isMember ("refresh_interval") && body["refresh_interval"].isNumeric ()) { - auto refresh = + using namespace std::chrono_literals; + std::chrono::minutes const refresh = boost::algorithm::clamp( - body["refresh_interval"].asUInt (), - 1u, - 60u*24); - sites_[siteIdx].refreshInterval = std::chrono::minutes{refresh}; + std::chrono::minutes {body["refresh_interval"].asUInt ()}, + 1min, + 24h); + sites_[siteIdx].refreshInterval = refresh; sites_[siteIdx].nextRefresh = clock_type::now() + sites_[siteIdx].refreshInterval; } @@ -428,7 +430,7 @@ std::shared_ptr ValidatorSite::processRedirect ( detail::response_type& res, std::size_t siteIdx, - std::lock_guard& lock) + std::lock_guard& sites_lock) { using namespace boost::beast::http; std::shared_ptr newLocation; @@ -442,7 +444,7 @@ ValidatorSite::processRedirect ( throw std::runtime_error{"missing location"}; } - if (sites_[siteIdx].redirCount == MAX_REDIRECTS) + if (sites_[siteIdx].redirCount == max_redirects) { JLOG (j_.warn()) << "Exceeded max redirects for validator list at " << @@ -492,7 +494,7 @@ ValidatorSite::onSiteFetch( errMsg}); if (retry) sites_[siteIdx].nextRefresh = - clock_type::now() + ERROR_RETRY_INTERVAL; + clock_type::now() + error_retry_interval; }; if (ec) { @@ -552,10 +554,10 @@ ValidatorSite::onSiteFetch( sites_[siteIdx].activeResource.reset(); } - std::unique_lock lock_state{state_mutex_}; + std::lock_guard lock_state{state_mutex_}; fetching_ = false; if (! stopping_) - setTimer (); + setTimer (lock_state); cv_.notify_all(); } @@ -593,10 +595,10 @@ ValidatorSite::onTextFetch( sites_[siteIdx].activeResource.reset(); } - std::unique_lock lock_state{state_mutex_}; + std::lock_guard lock_state{state_mutex_}; fetching_ = false; if (! stopping_) - setTimer (); + setTimer (lock_state); cv_.notify_all(); } diff --git a/src/test/app/ValidatorSite_test.cpp b/src/test/app/ValidatorSite_test.cpp index ef714a54e57..8083e4ad09a 100644 --- a/src/test/app/ValidatorSite_test.cpp +++ b/src/test/app/ValidatorSite_test.cpp @@ -50,6 +50,8 @@ constexpr const char* realValidatorContents() } )vl"; } + +auto constexpr default_expires = std::chrono::seconds{3600}; } class ValidatorSite_test : public beast::unit_test::suite @@ -187,25 +189,24 @@ class ValidatorSite_test : public beast::unit_test::suite } }; - struct fetchListConfig + struct FetchListConfig { - std::string path_; - std::string msg_; - bool failFetch_ = false; - bool failApply_ = false; - int serverVersion_ = 1; - std::chrono::seconds expiresFromNow_ = std::chrono::seconds{3600}; + std::string path; + std::string msg; + bool failFetch = false; + bool failApply = false; + int serverVersion = 1; + std::chrono::seconds expiresFromNow = detail::default_expires; + int expectedRefreshMin = 0; }; void - testFetchList (std::vector const& paths) + testFetchList (std::vector const& paths) { - using M = std::decay::type; testcase << "Fetch list - " << boost::algorithm::join (paths | boost::adaptors::transformed( - [](M::value_type t){ return t.path_; }), + [](FetchListConfig const& cfg){ return cfg.path; }), ", "); - using namespace jtx; Env env (*this); @@ -218,18 +219,16 @@ class ValidatorSite_test : public beast::unit_test::suite std::vector emptyCfgKeys; struct publisher { + publisher(FetchListConfig const& c) : cfg{c} {} std::unique_ptr server; std::vector list; std::string uri; - std::string expectMsg; - bool failFetch; - bool failApply; + FetchListConfig const& cfg; bool isRetry; }; std::vector servers; auto const sequence = 1; - using namespace std::chrono_literals; auto constexpr listSize = 20; std::vector cfgPublishers; @@ -245,12 +244,9 @@ class ValidatorSite_test : public beast::unit_test::suite publisherPublic, publisherSecret, pubSigningKeys.first, pubSigningKeys.second, 1); - servers.push_back({}); + servers.push_back(cfg); auto& item = servers.back(); - item.failFetch = cfg.failFetch_; - item.failApply = cfg.failApply_; - item.isRetry = cfg.path_ == "/bad-resource"; - item.expectMsg = cfg.msg_; + item.isRetry = cfg.path == "/bad-resource"; item.list.reserve (listSize); while (item.list.size () < listSize) item.list.push_back (randomValidator()); @@ -260,18 +256,19 @@ class ValidatorSite_test : public beast::unit_test::suite pubSigningKeys, manifest, sequence, - env.timeKeeper().now() + cfg.expiresFromNow_, - cfg.serverVersion_, + env.timeKeeper().now() + cfg.expiresFromNow, + cfg.serverVersion, item.list); std::stringstream uri; - uri << "http://" << item.server->local_endpoint() << cfg.path_; + uri << "http://" << item.server->local_endpoint() << cfg.path; item.uri = uri.str(); } BEAST_EXPECT(trustedKeys.load ( emptyLocalKey, emptyCfgKeys, cfgPublishers)); + using namespace std::chrono_literals; auto sites = std::make_unique ( env.app().getIOService(), env.app().validators(), @@ -285,34 +282,43 @@ class ValidatorSite_test : public beast::unit_test::suite sites->start(); sites->join(); + auto const jv = sites->getJson(); for (auto const& u : servers) { for (auto const& val : u.list) { BEAST_EXPECT( - trustedKeys.listed (val.masterPublic) != u.failApply); + trustedKeys.listed (val.masterPublic) != u.cfg.failApply); BEAST_EXPECT( - trustedKeys.listed (val.signingPublic) != u.failApply); + trustedKeys.listed (val.signingPublic) != u.cfg.failApply); } - auto const jv = sites->getJson(); Json::Value myStatus; for (auto const& vs : jv[jss::validator_sites]) if (vs[jss::uri].asString().find(u.uri) != std::string::npos) myStatus = vs; BEAST_EXPECTS( myStatus[jss::last_refresh_message].asString().empty() - != u.failFetch, + != u.cfg.failFetch, to_string(myStatus) + "\n" + sink.strm_.str()); - if (! u.expectMsg.empty()) + if (! u.cfg.msg.empty()) { BEAST_EXPECTS( - sink.strm_.str().find(u.expectMsg) != std::string::npos, + sink.strm_.str().find(u.cfg.msg) != std::string::npos, sink.strm_.str()); } - if (u.failFetch) + + if (u.cfg.expectedRefreshMin) + { + BEAST_EXPECTS( + myStatus[jss::refresh_interval_min].asInt() + == u.cfg.expectedRefreshMin, + to_string(myStatus)); + } + + if (u.cfg.failFetch) { using namespace std::chrono; log << " -- Msg: " << @@ -490,14 +496,10 @@ class ValidatorSite_test : public beast::unit_test::suite // bad manifest version testFetchList ({ {"/validators", "Unsupported version", false, true, 4}}); + using namespace std::chrono_literals; // get old validator list testFetchList ({ - {"/validators", - "Stale validator list", - false, - true, - 1, - std::chrono::seconds{0}}}); + {"/validators", "Stale validator list", false, true, 1, 0s}}); // force an out-of-range expiration value testFetchList ({ {"/validators", @@ -506,6 +508,31 @@ class ValidatorSite_test : public beast::unit_test::suite true, 1, std::chrono::seconds{Json::Value::maxInt + 1}}}); + // verify refresh intervals are properly clamped + testFetchList ({ + {"/validators/refresh/0", + "", + false, + false, + 1, + detail::default_expires, + 1}}); // minimum of 1 minute + testFetchList ({ + {"/validators/refresh/10", + "", + false, + false, + 1, + detail::default_expires, + 10}}); // 10 minutes is fine + testFetchList ({ + {"/validators/refresh/2000", + "", + false, + false, + 1, + detail::default_expires, + 60*24}}); // max of 24 hours testFileURLs(); } }; diff --git a/src/test/jtx/TrustedPublisherServer.h b/src/test/jtx/TrustedPublisherServer.h index 784de5e3c11..11f548ad200 100644 --- a/src/test/jtx/TrustedPublisherServer.h +++ b/src/test/jtx/TrustedPublisherServer.h @@ -166,73 +166,75 @@ class TrustedPublisherServer error_code ec; for (;;) { - req_type req; - http::read(sock, sb, req, ec); - if (ec) - break; - auto path = req.target().to_string(); resp_type res; - res.insert("Server", "TrustedPublisherServer"); - res.version(req.version()); - - if (boost::starts_with(path, "/validators")) + req_type req; + try { - res.result(http::status::ok); - res.insert("Content-Type", "application/json"); - if (path == "/validators/bad") - res.body() = "{ 'bad': \"1']" ; - else if (path == "/validators/missing") - res.body() = "{\"version\": 1}"; - else + http::read(sock, sb, req, ec); + if (ec) + break; + auto path = req.target().to_string(); + res.insert("Server", "TrustedPublisherServer"); + res.version(req.version()); + + if (boost::starts_with(path, "/validators")) { - int refresh = 5; - if (boost::starts_with(path, "/validators/refresh")) - refresh = - boost::lexical_cast(path.substr(20)); - res.body() = getList_(refresh); + res.result(http::status::ok); + res.insert("Content-Type", "application/json"); + if (path == "/validators/bad") + res.body() = "{ 'bad': \"1']" ; + else if (path == "/validators/missing") + res.body() = "{\"version\": 1}"; + else + { + int refresh = 5; + if (boost::starts_with(path, "/validators/refresh")) + refresh = + boost::lexical_cast( + path.substr(20)); + res.body() = getList_(refresh); + } } - } - else if (boost::starts_with(path, "/sleep/")) - { - auto sleep_sec = - boost::lexical_cast(path.substr(7)); - std::this_thread::sleep_for(std::chrono::seconds{sleep_sec}); - } - else if (boost::starts_with(path, "/redirect")) - { - if (boost::ends_with(path, "/301")) - res.result(http::status::moved_permanently); - else if (boost::ends_with(path, "/302")) - res.result(http::status::found); - else if (boost::ends_with(path, "/307")) - res.result(http::status::temporary_redirect); - else if (boost::ends_with(path, "/308")) - res.result(http::status::permanent_redirect); - - std::stringstream location; - if (boost::starts_with(path, "/redirect_to/")) + else if (boost::starts_with(path, "/sleep/")) { - location << path.substr(13); + auto const sleep_sec = + boost::lexical_cast(path.substr(7)); + std::this_thread::sleep_for( + std::chrono::seconds{sleep_sec}); } - else if (! boost::starts_with(path, "/redirect_nolo")) + else if (boost::starts_with(path, "/redirect")) { - location << "http://" << local_endpoint() << - (boost::starts_with(path, "/redirect_forever/") ? - path : "/validators"); + if (boost::ends_with(path, "/301")) + res.result(http::status::moved_permanently); + else if (boost::ends_with(path, "/302")) + res.result(http::status::found); + else if (boost::ends_with(path, "/307")) + res.result(http::status::temporary_redirect); + else if (boost::ends_with(path, "/308")) + res.result(http::status::permanent_redirect); + + std::stringstream location; + if (boost::starts_with(path, "/redirect_to/")) + { + location << path.substr(13); + } + else if (! boost::starts_with(path, "/redirect_nolo")) + { + location << "http://" << local_endpoint() << + (boost::starts_with(path, "/redirect_forever/") ? + path : "/validators"); + } + if (! location.str().empty()) + res.insert("Location", location.str()); + } + else + { + // unknown request + res.result(boost::beast::http::status::not_found); + res.insert("Content-Type", "text/html"); + res.body() = "The file '" + path + "' was not found"; } - if (! location.str().empty()) - res.insert("Location", location.str()); - } - else - { - // unknown request - res.result(boost::beast::http::status::not_found); - res.insert("Content-Type", "text/html"); - res.body() = "The file '" + path + "' was not found"; - } - try - { res.prepare_payload(); } catch (std::exception const& e) From 88e41a69f2c5d6231bac2665aee4335f810c5fbb Mon Sep 17 00:00:00 2001 From: Mike Ellery Date: Mon, 15 Apr 2019 09:38:12 -0700 Subject: [PATCH 3/3] [FOLD] catch/ignore ex from timer cancel methods --- src/ripple/app/misc/impl/ValidatorSite.cpp | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/ripple/app/misc/impl/ValidatorSite.cpp b/src/ripple/app/misc/impl/ValidatorSite.cpp index 8910d7ccbc3..4ae94199ae7 100644 --- a/src/ripple/app/misc/impl/ValidatorSite.cpp +++ b/src/ripple/app/misc/impl/ValidatorSite.cpp @@ -177,7 +177,15 @@ ValidatorSite::stop() sp->cancel(); cv_.wait(lock, [&]{ return ! fetching_; }); - timer_.cancel(); + // docs indicate cancel() can throw, but this should be + // reconsidered if it changes to noexcept + try + { + timer_.cancel(); + } + catch (boost::system::system_error const&) + { + } stopping_ = false; pending_ = false; cv_.notify_all(); @@ -220,7 +228,15 @@ ValidatorSite::makeRequest ( [this] () { std::lock_guard lock_state{state_mutex_}; - timer_.cancel_one(); + // docs indicate cancel_one() can throw, but this + // should be reconsidered if it changes to noexcept + try + { + timer_.cancel_one(); + } + catch (boost::system::system_error const&) + { + } }; auto onFetch = [this, siteIdx, timeoutCancel] (