diff --git a/src/rpc/Counters.cpp b/src/rpc/Counters.cpp index 38bd2522..2d65f82c 100644 --- a/src/rpc/Counters.cpp +++ b/src/rpc/Counters.cpp @@ -23,13 +23,22 @@ namespace RPC { +void +Counters::rpcFailed(std::string const& method) +{ + std::scoped_lock lk(mutex_); + MethodInfo& counters = methodInfo_[method]; + ++counters.started; + ++counters.failed; +} + void Counters::rpcErrored(std::string const& method) { std::scoped_lock lk(mutex_); MethodInfo& counters = methodInfo_[method]; - counters.started++; - counters.errored++; + ++counters.started; + ++counters.errored; } void @@ -37,8 +46,8 @@ Counters::rpcComplete(std::string const& method, std::chrono::microseconds const { std::scoped_lock lk(mutex_); MethodInfo& counters = methodInfo_[method]; - counters.started++; - counters.finished++; + ++counters.started; + ++counters.finished; counters.duration += rpcDuration.count(); } @@ -47,7 +56,45 @@ Counters::rpcForwarded(std::string const& method) { std::scoped_lock lk(mutex_); MethodInfo& counters = methodInfo_[method]; - counters.forwarded++; + ++counters.forwarded; +} + +void +Counters::rpcFailedToForward(std::string const& method) +{ + std::scoped_lock lk(mutex_); + MethodInfo& counters = methodInfo_[method]; + ++counters.failedForward; +} + +void +Counters::onTooBusy() +{ + ++tooBusyCounter_; +} + +void +Counters::onNotReady() +{ + ++notReadyCounter_; +} + +void +Counters::onBadSyntax() +{ + ++badSyntaxCounter_; +} + +void +Counters::onUnknownCommand() +{ + ++unknownCommandCounter_; +} + +void +Counters::onInternalError() +{ + ++internalErrorCounter_; } boost::json::object @@ -65,12 +112,20 @@ Counters::report() const counters[JS(started)] = std::to_string(info.started); counters[JS(finished)] = std::to_string(info.finished); counters[JS(errored)] = std::to_string(info.errored); + counters[JS(failed)] = std::to_string(info.failed); counters["forwarded"] = std::to_string(info.forwarded); + counters["failed_forward"] = std::to_string(info.failedForward); counters[JS(duration_us)] = std::to_string(info.duration); rpc[method] = std::move(counters); } + obj["too_busy_errors"] = std::to_string(tooBusyCounter_); + obj["not_ready_errors"] = std::to_string(notReadyCounter_); + obj["bad_syntax_errors"] = std::to_string(badSyntaxCounter_); + obj["unknown_command_errors"] = std::to_string(unknownCommandCounter_); + obj["internal_errors"] = std::to_string(internalErrorCounter_); + obj["work_queue"] = workQueue_.get().report(); return obj; diff --git a/src/rpc/Counters.h b/src/rpc/Counters.h index bc8cdea0..60ddae15 100644 --- a/src/rpc/Counters.h +++ b/src/rpc/Counters.h @@ -36,14 +36,23 @@ class Counters { std::uint64_t started = 0u; std::uint64_t finished = 0u; + std::uint64_t failed = 0u; std::uint64_t errored = 0u; std::uint64_t forwarded = 0u; + std::uint64_t failedForward = 0u; std::uint64_t duration = 0u; }; mutable std::mutex mutex_; std::unordered_map methodInfo_; + // counters that don't carry RPC method information + std::atomic_uint64_t tooBusyCounter_; + std::atomic_uint64_t notReadyCounter_; + std::atomic_uint64_t badSyntaxCounter_; + std::atomic_uint64_t unknownCommandCounter_; + std::atomic_uint64_t internalErrorCounter_; + std::reference_wrapper workQueue_; public: @@ -55,6 +64,9 @@ public: return Counters{wq}; } + void + rpcFailed(std::string const& method); + void rpcErrored(std::string const& method); @@ -64,6 +76,24 @@ public: void rpcForwarded(std::string const& method); + void + rpcFailedToForward(std::string const& method); + + void + onTooBusy(); + + void + onNotReady(); + + void + onBadSyntax(); + + void + onUnknownCommand(); + + void + onInternalError(); + boost::json::object report() const; }; diff --git a/src/rpc/RPCEngine.h b/src/rpc/RPCEngine.h index 67b6cebf..7df02772 100644 --- a/src/rpc/RPCEngine.h +++ b/src/rpc/RPCEngine.h @@ -117,24 +117,31 @@ public: auto toForward = ctx.params; toForward["command"] = ctx.method; - auto const res = balancer_->forwardToRippled(toForward, ctx.clientIp, ctx.yield); - notifyForwarded(ctx.method); - - if (!res) + if (auto const res = balancer_->forwardToRippled(toForward, ctx.clientIp, ctx.yield); not res) + { + notifyFailedToForward(ctx.method); return Status{RippledError::rpcFAILED_TO_FORWARD}; - - return *res; + } + else + { + notifyForwarded(ctx.method); + return *res; + } } if (backend_->isTooBusy()) { log_.error() << "Database is too busy. Rejecting request"; + notifyTooBusy(); // TODO: should we add ctx.method if we have it? return Status{RippledError::rpcTOO_BUSY}; } auto const method = handlerTable_.getHandler(ctx.method); if (!method) + { + notifyUnknownCommand(); return Status{RippledError::rpcUNKNOWN_COMMAND}; + } try { @@ -149,24 +156,23 @@ public: if (v) return v->as_object(); else + { + notifyErrored(ctx.method); return Status{v.error()}; - } - catch (InvalidParamsError const& err) - { - return Status{RippledError::rpcINVALID_PARAMS, err.what()}; - } - catch (AccountNotFoundError const& err) - { - return Status{RippledError::rpcACT_NOT_FOUND, err.what()}; + } } catch (Backend::DatabaseTimeout const& t) { log_.error() << "Database timeout"; + notifyTooBusy(); + return Status{RippledError::rpcTOO_BUSY}; } - catch (std::exception const& err) + catch (std::exception const& ex) { - log_.error() << ctx.tag() << " caught exception: " << err.what(); + log_.error() << ctx.tag() << "Caught exception: " << ex.what(); + notifyInternalError(); + return Status{RippledError::rpcINTERNAL}; } } @@ -180,7 +186,13 @@ public: bool post(Fn&& func, std::string const& ip) { - return workQueue_.get().postCoro(std::forward(func), dosGuard_.get().isWhiteListed(ip)); + if (!workQueue_.get().postCoro(std::forward(func), dosGuard_.get().isWhiteListed(ip))) + { + notifyTooBusy(); + return false; + } + + return true; } /** @@ -197,7 +209,24 @@ public: } /** - * @brief Notify the system that specified method failed to execute + * @brief Notify the system that specified method failed to execute due to a recoverable user error + * + * Used for errors based on user input, not actual failures of the db or clio itself. + * + * @param method + */ + void + notifyFailed(std::string const& method) + { + if (validHandler(method)) + counters_.get().rpcFailed(method); + } + + /** + * @brief Notify the system that specified method failed due to some unrecoverable error + * + * Used for erors such as database timeout, internal errors, etc. + * * @param method */ void @@ -218,6 +247,64 @@ public: counters_.get().rpcForwarded(method); } + /** + * @brief Notify the system that specified method failed to be forwarded to rippled + * @param method + */ + void + notifyFailedToForward(std::string const& method) + { + if (validHandler(method)) + counters_.get().rpcFailedToForward(method); + } + + /** + * @brief Notify the system that the RPC system is too busy to handle an incoming request + */ + void + notifyTooBusy() + { + counters_.get().onTooBusy(); + } + + /** + * @brief Notify the system that the RPC system was not ready to handle an incoming request + * + * This happens when the backend is not yet have a ledger range + */ + void + notifyNotReady() + { + counters_.get().onNotReady(); + } + + /** + * @brief Notify the system that the incoming request did not specify the RPC method/command + */ + void + notifyBadSyntax() + { + counters_.get().onBadSyntax(); + } + + /** + * @brief Notify the system that the incoming request specified an unknown/unsupported method/command + */ + void + notifyUnknownCommand() + { + counters_.get().onUnknownCommand(); + } + + /** + * @brief Notify the system that the incoming request lead to an internal error (unrecoverable) + */ + void + notifyInternalError() + { + counters_.get().onInternalError(); + } + private: bool shouldForwardToRippled(Web::Context const& ctx) const diff --git a/src/rpc/RPCHelpers.cpp b/src/rpc/RPCHelpers.cpp index e23503c5..ffe04b2c 100644 --- a/src/rpc/RPCHelpers.cpp +++ b/src/rpc/RPCHelpers.cpp @@ -36,66 +36,6 @@ clio::Logger gLog{"RPC"}; namespace RPC { -std::optional -getBool(boost::json::object const& request, std::string const& field) -{ - if (!request.contains(field)) - return {}; - else if (request.at(field).is_bool()) - return request.at(field).as_bool(); - else - throw InvalidParamsError("Invalid field " + field + ", not bool."); -} - -bool -getBool(boost::json::object const& request, std::string const& field, bool dfault) -{ - if (auto res = getBool(request, field)) - return *res; - else - return dfault; -} - -bool -getRequiredBool(boost::json::object const& request, std::string const& field) -{ - if (auto res = getBool(request, field)) - return *res; - else - throw InvalidParamsError("Missing field " + field); -} - -std::optional -getUInt(boost::json::object const& request, std::string const& field) -{ - if (!request.contains(field)) - return {}; - else if (request.at(field).is_uint64()) - return request.at(field).as_uint64(); - else if (request.at(field).is_int64()) - return request.at(field).as_int64(); - else - throw InvalidParamsError("Invalid field " + field + ", not uint."); -} - -std::uint32_t -getUInt(boost::json::object const& request, std::string const& field, std::uint32_t const dfault) -{ - if (auto res = getUInt(request, field)) - return *res; - else - return dfault; -} - -std::uint32_t -getRequiredUInt(boost::json::object const& request, std::string const& field) -{ - if (auto res = getUInt(request, field)) - return *res; - else - throw InvalidParamsError("Missing field " + field); -} - std::optional parseAccountCursor(std::optional jsonCursor) { @@ -130,143 +70,6 @@ parseAccountCursor(std::optional jsonCursor) return AccountCursor({cursorIndex, startHint}); } -std::optional -getString(boost::json::object const& request, std::string const& field) -{ - if (!request.contains(field)) - return {}; - else if (request.at(field).is_string()) - return request.at(field).as_string().c_str(); - else - throw InvalidParamsError("Invalid field " + field + ", not string."); -} - -std::string -getRequiredString(boost::json::object const& request, std::string const& field) -{ - if (auto res = getString(request, field)) - return *res; - else - throw InvalidParamsError("Missing field " + field); -} - -std::string -getString(boost::json::object const& request, std::string const& field, std::string dfault) -{ - if (auto res = getString(request, field)) - return *res; - else - return dfault; -} - -Status -getHexMarker(boost::json::object const& request, ripple::uint256& marker) -{ - if (request.contains(JS(marker))) - { - if (!request.at(JS(marker)).is_string()) - return Status{RippledError::rpcINVALID_PARAMS, "markerNotString"}; - - if (!marker.parseHex(request.at(JS(marker)).as_string().c_str())) - return Status{RippledError::rpcINVALID_PARAMS, "malformedMarker"}; - } - - return {}; -} - -Status -getAccount( - boost::json::object const& request, - ripple::AccountID& account, - boost::string_view const& field, - bool required) -{ - if (!request.contains(field)) - { - if (required) - return Status{RippledError::rpcINVALID_PARAMS, field.to_string() + "Missing"}; - - return {}; - } - - if (!request.at(field).is_string()) - return Status{RippledError::rpcINVALID_PARAMS, field.to_string() + "NotString"}; - - if (auto a = accountFromStringStrict(request.at(field).as_string().c_str()); a) - { - account = a.value(); - return {}; - } - - return Status{RippledError::rpcACT_MALFORMED, field.to_string() + "Malformed"}; -} - -Status -getOptionalAccount( - boost::json::object const& request, - std::optional& account, - boost::string_view const& field) -{ - if (!request.contains(field)) - { - account = {}; - return {}; - } - - if (!request.at(field).is_string()) - return Status{RippledError::rpcINVALID_PARAMS, field.to_string() + "NotString"}; - - if (auto a = accountFromStringStrict(request.at(field).as_string().c_str()); a) - { - account = a.value(); - return {}; - } - - return Status{RippledError::rpcINVALID_PARAMS, field.to_string() + "Malformed"}; -} - -Status -getAccount(boost::json::object const& request, ripple::AccountID& accountId) -{ - return getAccount(request, accountId, JS(account), true); -} - -Status -getAccount(boost::json::object const& request, ripple::AccountID& destAccount, boost::string_view const& field) -{ - return getAccount(request, destAccount, field, false); -} - -Status -getTaker(boost::json::object const& request, ripple::AccountID& takerID) -{ - if (request.contains(JS(taker))) - { - auto parsed = parseTaker(request.at(JS(taker))); - if (auto status = std::get_if(&parsed); status) - return *status; - else - takerID = std::get(parsed); - } - - return {}; -} - -Status -getChannelId(boost::json::object const& request, ripple::uint256& channelId) -{ - if (!request.contains(JS(channel_id))) - return Status{RippledError::rpcINVALID_PARAMS, "missingChannelID"}; - - if (!request.at(JS(channel_id)).is_string()) - return Status{RippledError::rpcINVALID_PARAMS, "channelIDNotString"}; - - if (!channelId.parseHex(request.at(JS(channel_id)).as_string().c_str())) - return Status{RippledError::rpcCHANNEL_MALFORMED, "malformedChannelID"}; - - return {}; -} - std::optional getDeliveredAmount( std::shared_ptr const& txn, @@ -305,11 +108,6 @@ canHaveDeliveredAmount( if (tt != ripple::ttPAYMENT && tt != ripple::ttCHECK_CASH && tt != ripple::ttACCOUNT_DELETE) return false; - /* - if (tt == ttCHECK_CASH && !getFix1623Enabled()) - return false; - */ - if (meta->getResultTER() != ripple::tesSUCCESS) return false; diff --git a/src/rpc/RPCHelpers.h b/src/rpc/RPCHelpers.h index 1dfb6dbf..e4a00869 100644 --- a/src/rpc/RPCHelpers.h +++ b/src/rpc/RPCHelpers.h @@ -216,54 +216,6 @@ parseBook(boost::json::object const& request); std::variant parseTaker(boost::json::value const& request); -std::optional -getUInt(boost::json::object const& request, std::string const& field); - -std::uint32_t -getUInt(boost::json::object const& request, std::string const& field, std::uint32_t dfault); - -std::uint32_t -getRequiredUInt(boost::json::object const& request, std::string const& field); - -std::optional -getBool(boost::json::object const& request, std::string const& field); - -bool -getBool(boost::json::object const& request, std::string const& field, bool dfault); - -bool -getRequiredBool(boost::json::object const& request, std::string const& field); - -std::optional -getString(boost::json::object const& request, std::string const& field); - -std::string -getRequiredString(boost::json::object const& request, std::string const& field); - -std::string -getString(boost::json::object const& request, std::string const& field, std::string dfault); - -Status -getHexMarker(boost::json::object const& request, ripple::uint256& marker); - -Status -getAccount(boost::json::object const& request, ripple::AccountID& accountId); - -Status -getAccount(boost::json::object const& request, ripple::AccountID& destAccount, boost::string_view const& field); - -Status -getOptionalAccount( - boost::json::object const& request, - std::optional& account, - boost::string_view const& field); - -Status -getTaker(boost::json::object const& request, ripple::AccountID& takerID); - -Status -getChannelId(boost::json::object const& request, ripple::uint256& channelId); - bool specifiesCurrentOrClosedLedger(boost::json::object const& request); diff --git a/src/rpc/handlers/LedgerRange.cpp b/src/rpc/handlers/LedgerRange.cpp index 0de01c62..574e295d 100644 --- a/src/rpc/handlers/LedgerRange.cpp +++ b/src/rpc/handlers/LedgerRange.cpp @@ -27,10 +27,8 @@ namespace RPC { LedgerRangeHandler::Result LedgerRangeHandler::process() const { - if (auto const maybeRange = sharedPtrBackend_->fetchLedgerRange(); maybeRange) - return Output{*maybeRange}; - else - return Error{Status{RippledError::rpcNOT_READY, "rangeNotFound"}}; + // note: we can't get here if range is not available so it's safe + return Output{sharedPtrBackend_->fetchLedgerRange().value()}; } void diff --git a/src/rpc/handlers/ServerInfo.h b/src/rpc/handlers/ServerInfo.h index 1236c318..6924abd5 100644 --- a/src/rpc/handlers/ServerInfo.h +++ b/src/rpc/handlers/ServerInfo.h @@ -115,12 +115,6 @@ public: using namespace std::chrono; auto const range = backend_->fetchLedgerRange(); - - // TODO: remove this check in https://github.com/XRPLF/clio/issues/592 - // note: this should happen on framework level. - if (not range.has_value()) - return Error{Status{RippledError::rpcNOT_READY, "emptyDatabase", "The server has no data in the database"}}; - auto const lgrInfo = backend_->fetchLedgerBySequence(range->maxSequence, ctx.yield); if (not lgrInfo.has_value()) return Error{Status{RippledError::rpcINTERNAL}}; diff --git a/src/webserver/RPCExecutor.h b/src/webserver/RPCExecutor.h index b980c8a8..6a5a8906 100644 --- a/src/webserver/RPCExecutor.h +++ b/src/webserver/RPCExecutor.h @@ -60,34 +60,39 @@ public: void operator()(std::string const& reqStr, std::shared_ptr const& connection) { - auto req = boost::json::object{}; try { - req = boost::json::parse(reqStr).as_object(); + auto req = boost::json::parse(reqStr).as_object(); + perfLog_.debug() << connection->tag() << "Adding to work queue"; + + if (not connection->upgraded and not req.contains("params")) + req["params"] = boost::json::array({boost::json::object{}}); + + if (!rpcEngine_->post( + [request = std::move(req), connection, this](boost::asio::yield_context yc) mutable { + handleRequest(yc, std::move(request), connection); + }, + connection->clientIp)) + { + rpcEngine_->notifyTooBusy(); + connection->send( + boost::json::serialize(RPC::makeError(RPC::RippledError::rpcTOO_BUSY)), + boost::beast::http::status::ok); + } } - catch (boost::exception const& _) + catch (boost::system::system_error const&) { + // system_error thrown when json parsing failed + rpcEngine_->notifyBadSyntax(); connection->send( boost::json::serialize(RPC::makeError(RPC::RippledError::rpcBAD_SYNTAX)), boost::beast::http::status::ok); - return; } - - perfLog_.debug() << connection->tag() << "Adding to work queue"; - // specially handle for http connections - if (!connection->upgraded) + catch (std::exception const& ex) { - if (!req.contains("params")) - req["params"] = boost::json::array({boost::json::object{}}); - } - if (!rpcEngine_->post( - [request = std::move(req), connection, this](boost::asio::yield_context yc) mutable { - handleRequest(yc, std::move(request), connection); - }, - connection->clientIp)) - { - connection->send( - boost::json::serialize(RPC::makeError(RPC::RippledError::rpcTOO_BUSY)), boost::beast::http::status::ok); + perfLog_.error() << connection->tag() << "Caught exception: " << ex.what(); + rpcEngine_->notifyInternalError(); + throw; } } @@ -121,14 +126,11 @@ private: if (!id.is_null()) e["id"] = id; e["request"] = request; + if (connection->upgraded) - { return e; - } else - { return boost::json::object{{"result", e}}; - } }; try @@ -136,9 +138,12 @@ private: auto const range = backend_->fetchLedgerRange(); // for the error happened before the handler, we don't attach the clio warning if (!range) + { + rpcEngine_->notifyNotReady(); return connection->send( boost::json::serialize(composeError(RPC::RippledError::rpcNOT_READY)), boost::beast::http::status::ok); + } auto context = connection->upgraded ? RPC::make_WsContext( @@ -149,6 +154,8 @@ private: { perfLog_.warn() << connection->tag() << "Could not create RPC context"; log_.warn() << connection->tag() << "Could not create RPC context"; + + rpcEngine_->notifyBadSyntax(); return connection->send( boost::json::serialize(composeError(RPC::RippledError::rpcBAD_SYNTAX)), boost::beast::http::status::ok); @@ -162,16 +169,16 @@ private: boost::json::object response; if (auto const status = std::get_if(&v)) { - rpcEngine_->notifyErrored(context->method); + // note: error statuses are counted/notified in buildResponse itself response = std::move(composeError(*status)); auto const responseStr = boost::json::serialize(response); + perfLog_.debug() << context->tag() << "Encountered error: " << responseStr; log_.debug() << context->tag() << "Encountered error: " << responseStr; } else { - // This can still technically be an error. Clio counts forwarded - // requests as successful. + // This can still technically be an error. Clio counts forwarded requests as successful. rpcEngine_->notifyComplete(context->method, us); auto& result = std::get(v); @@ -189,6 +196,7 @@ private: { response["result"] = result; } + // for ws , there is additional field "status" in response // otherwise , the "status" is in the "result" field if (connection->upgraded) @@ -215,10 +223,15 @@ private: response["warnings"] = warnings; connection->send(boost::json::serialize(response), boost::beast::http::status::ok); } - catch (std::exception const& e) + catch (std::exception const& ex) { - perfLog_.error() << connection->tag() << "Caught exception : " << e.what(); - log_.error() << connection->tag() << "Caught exception : " << e.what(); + // note: while we are catching this in buildResponse too, this is here to make sure + // that any other code that may throw is outside of buildResponse is also worked around. + perfLog_.error() << connection->tag() << "Caught exception: " << ex.what(); + log_.error() << connection->tag() << "Caught exception: " << ex.what(); + + rpcEngine_->notifyInternalError(); + return connection->send( boost::json::serialize(composeError(RPC::RippledError::rpcINTERNAL)), boost::beast::http::status::internal_server_error); diff --git a/src/webserver/details/HttpBase.h b/src/webserver/details/HttpBase.h index f6d16795..f6e26dfb 100644 --- a/src/webserver/details/HttpBase.h +++ b/src/webserver/details/HttpBase.h @@ -38,7 +38,8 @@ namespace Server { using tcp = boost::asio::ip::tcp; /** - * This is the implementation class for http sessions + * @brief This is the implementation class for http sessions + * * @tparam Derived The derived class * @tparam Handler The handler class, will be called when a request is received. */ @@ -66,13 +67,11 @@ class HttpBase : public ConnectionBase if (self_.dead()) return; - // The lifetime of the message has to extend - // for the duration of the async operation so - // we use a shared_ptr to manage it. + // The lifetime of the message has to extend for the duration of the async operation so we use a shared_ptr + // to manage it. auto sp = std::make_shared>(std::move(msg)); - // Store a type-erased version of the shared - // pointer in the class to keep it alive. + // Store a type-erased version of the shared pointer in the class to keep it alive. self_.res_ = sp; // Write the response @@ -199,6 +198,7 @@ public: // connection limit if (!dosGuard_.get().request(clientIp)) { + // TODO: this looks like it could be useful to count too in the future return sender_(httpResponse( http::status::service_unavailable, "text/plain", @@ -211,9 +211,8 @@ public: { (*handler_)(req_.body(), derived().shared_from_this()); } - catch (std::exception const& e) + catch (std::exception const&) { - perfLog_.error() << tag() << "Caught exception : " << e.what(); return sender_(httpResponse( http::status::internal_server_error, "application/json", diff --git a/src/webserver/details/WsBase.h b/src/webserver/details/WsBase.h index e3aa3809..742229db 100644 --- a/src/webserver/details/WsBase.h +++ b/src/webserver/details/WsBase.h @@ -230,7 +230,7 @@ public: e["id"] = request.as_object().at("id"); e["request"] = std::move(request); } - catch (std::exception&) + catch (std::exception const&) { e["request"] = std::move(requestStr); } @@ -246,6 +246,7 @@ public: // dosGuard served request++ and check ip address if (!dosGuard_.get().request(clientIp)) { + // TODO: could be useful to count in counters in the future too sendError(RPC::RippledError::rpcSLOW_DOWN, std::move(msg)); } else @@ -254,9 +255,8 @@ public: { (*handler_)(msg, shared_from_this()); } - catch (std::exception const& e) + catch (std::exception const&) { - perfLog_.error() << tag() << "Caught exception : " << e.what(); sendError(RPC::RippledError::rpcINTERNAL, std::move(msg)); } } diff --git a/unittests/rpc/CountersTest.cpp b/unittests/rpc/CountersTest.cpp index f4de451e..95fc8834 100644 --- a/unittests/rpc/CountersTest.cpp +++ b/unittests/rpc/CountersTest.cpp @@ -37,11 +37,18 @@ protected: TEST_F(RPCCountersTest, CheckThatCountersAddUp) { - for (auto i = 0u; i < 512; ++i) + for (auto i = 0u; i < 512u; ++i) { counters.rpcErrored("error"); counters.rpcComplete("complete", std::chrono::milliseconds{1u}); counters.rpcForwarded("forward"); + counters.rpcFailedToForward("failedToForward"); + counters.rpcFailed("failed"); + counters.onTooBusy(); + counters.onNotReady(); + counters.onBadSyntax(); + counters.onUnknownCommand(); + counters.onInternalError(); } auto const report = counters.report(); @@ -51,17 +58,43 @@ TEST_F(RPCCountersTest, CheckThatCountersAddUp) EXPECT_STREQ(rpc.at("error").as_object().at(JS(finished)).as_string().c_str(), "0"); EXPECT_STREQ(rpc.at("error").as_object().at(JS(errored)).as_string().c_str(), "512"); EXPECT_STREQ(rpc.at("error").as_object().at("forwarded").as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("error").as_object().at("failed_forward").as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("error").as_object().at(JS(failed)).as_string().c_str(), "0"); EXPECT_STREQ(rpc.at("complete").as_object().at(JS(started)).as_string().c_str(), "512"); EXPECT_STREQ(rpc.at("complete").as_object().at(JS(finished)).as_string().c_str(), "512"); EXPECT_STREQ(rpc.at("complete").as_object().at(JS(errored)).as_string().c_str(), "0"); EXPECT_STREQ(rpc.at("complete").as_object().at("forwarded").as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("complete").as_object().at("failed_forward").as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("complete").as_object().at(JS(failed)).as_string().c_str(), "0"); EXPECT_STREQ(rpc.at("complete").as_object().at(JS(duration_us)).as_string().c_str(), "512000"); // 1000 per call EXPECT_STREQ(rpc.at("forward").as_object().at(JS(started)).as_string().c_str(), "0"); EXPECT_STREQ(rpc.at("forward").as_object().at(JS(finished)).as_string().c_str(), "0"); EXPECT_STREQ(rpc.at("forward").as_object().at(JS(errored)).as_string().c_str(), "0"); EXPECT_STREQ(rpc.at("forward").as_object().at("forwarded").as_string().c_str(), "512"); + EXPECT_STREQ(rpc.at("forward").as_object().at("failed_forward").as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("forward").as_object().at(JS(failed)).as_string().c_str(), "0"); + + EXPECT_STREQ(rpc.at("failed").as_object().at(JS(started)).as_string().c_str(), "512"); + EXPECT_STREQ(rpc.at("failed").as_object().at(JS(finished)).as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("failed").as_object().at(JS(errored)).as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("failed").as_object().at("forwarded").as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("failed").as_object().at("failed_forward").as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("failed").as_object().at(JS(failed)).as_string().c_str(), "512"); + + EXPECT_STREQ(rpc.at("failedToForward").as_object().at(JS(started)).as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("failedToForward").as_object().at(JS(finished)).as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("failedToForward").as_object().at(JS(errored)).as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("failedToForward").as_object().at("forwarded").as_string().c_str(), "0"); + EXPECT_STREQ(rpc.at("failedToForward").as_object().at("failed_forward").as_string().c_str(), "512"); + EXPECT_STREQ(rpc.at("failedToForward").as_object().at(JS(failed)).as_string().c_str(), "0"); + + EXPECT_STREQ(report.at("too_busy_errors").as_string().c_str(), "512"); + EXPECT_STREQ(report.at("not_ready_errors").as_string().c_str(), "512"); + EXPECT_STREQ(report.at("bad_syntax_errors").as_string().c_str(), "512"); + EXPECT_STREQ(report.at("unknown_command_errors").as_string().c_str(), "512"); + EXPECT_STREQ(report.at("internal_errors").as_string().c_str(), "512"); EXPECT_EQ(report.at("work_queue"), queue.report()); // Counters report includes queue report } diff --git a/unittests/rpc/handlers/LedgerRangeTest.cpp b/unittests/rpc/handlers/LedgerRangeTest.cpp index dfb27830..8d784b00 100644 --- a/unittests/rpc/handlers/LedgerRangeTest.cpp +++ b/unittests/rpc/handlers/LedgerRangeTest.cpp @@ -34,17 +34,6 @@ class RPCLedgerRangeTest : public HandlerBaseTest { }; -TEST_F(RPCLedgerRangeTest, LedgerRangeNotFound) -{ - auto const handler = AnyHandler{LedgerRangeHandler{mockBackendPtr}}; - auto const req = json::parse("{}"); - auto const output = handler.process(req); - ASSERT_FALSE(output); - auto const err = RPC::makeError(output.error()); - EXPECT_EQ(err.at("error").as_string(), "notReady"); - EXPECT_EQ(err.at("error_message").as_string(), "rangeNotFound"); -} - TEST_F(RPCLedgerRangeTest, LedgerRangeMinMaxSame) { mockBackendPtr->updateRange(RANGEMIN); diff --git a/unittests/rpc/handlers/ServerInfoTest.cpp b/unittests/rpc/handlers/ServerInfoTest.cpp index 2b451d8d..d29dfbd7 100644 --- a/unittests/rpc/handlers/ServerInfoTest.cpp +++ b/unittests/rpc/handlers/ServerInfoTest.cpp @@ -131,22 +131,6 @@ protected: } }; -TEST_F(RPCServerInfoHandlerTest, NoRangeErrorsOutWithNotReady) -{ - auto const handler = AnyHandler{TestServerInfoHandler{ - mockBackendPtr, mockSubscriptionManagerPtr, mockLoadBalancerPtr, mockETLServicePtr, *mockCountersPtr}}; - - runSpawn([&](auto& yield) { - auto const req = json::parse("{}"); - auto const output = handler.process(req, Context{std::ref(yield)}); - - ASSERT_FALSE(output); - auto const err = RPC::makeError(output.error()); - EXPECT_EQ(err.at("error").as_string(), "emptyDatabase"); - EXPECT_EQ(err.at("error_message").as_string(), "The server has no data in the database"); - }); -} - TEST_F(RPCServerInfoHandlerTest, NoLedgerInfoErrorsOutWithInternal) { MockBackend* rawBackendPtr = static_cast(mockBackendPtr.get()); diff --git a/unittests/util/MockRPCEngine.h b/unittests/util/MockRPCEngine.h index 670d738c..61472e5b 100644 --- a/unittests/util/MockRPCEngine.h +++ b/unittests/util/MockRPCEngine.h @@ -52,8 +52,15 @@ public: } MOCK_METHOD(void, notifyComplete, (std::string const&, std::chrono::microseconds const&), ()); + MOCK_METHOD(void, notifyFailed, (std::string const&), ()); MOCK_METHOD(void, notifyErrored, (std::string const&), ()); MOCK_METHOD(void, notifyForwarded, (std::string const&), ()); + MOCK_METHOD(void, notifyFailedToForward, (std::string const&), ()); + MOCK_METHOD(void, notifyNotReady, (), ()); + MOCK_METHOD(void, notifyBadSyntax, (), ()); + MOCK_METHOD(void, notifyTooBusy, (), ()); + MOCK_METHOD(void, notifyUnknownCommand, (), ()); + MOCK_METHOD(void, notifyInternalError, (), ()); MOCK_METHOD(RPC::Result, buildResponse, (Web::Context const&), ()); private: @@ -69,5 +76,11 @@ public: MOCK_METHOD(void, notifyComplete, (std::string const&, std::chrono::microseconds const&), ()); MOCK_METHOD(void, notifyErrored, (std::string const&), ()); MOCK_METHOD(void, notifyForwarded, (std::string const&), ()); + MOCK_METHOD(void, notifyFailedToForward, (std::string const&), ()); + MOCK_METHOD(void, notifyNotReady, (), ()); + MOCK_METHOD(void, notifyBadSyntax, (), ()); + MOCK_METHOD(void, notifyTooBusy, (), ()); + MOCK_METHOD(void, notifyUnknownCommand, (), ()); + MOCK_METHOD(void, notifyInternalError, (), ()); MOCK_METHOD(RPC::Result, buildResponse, (Web::Context const&), ()); }; diff --git a/unittests/webserver/RPCExecutorTest.cpp b/unittests/webserver/RPCExecutorTest.cpp index dc02d675..e7ed1026 100644 --- a/unittests/webserver/RPCExecutorTest.cpp +++ b/unittests/webserver/RPCExecutorTest.cpp @@ -94,13 +94,13 @@ TEST_F(WebRPCExecutorTest, HTTPDefaultPath) static auto constexpr result = "{}"; static auto constexpr response = R"({ - "result":{ - "status":"success" + "result": { + "status": "success" }, - "warnings":[ + "warnings": [ { - "id":2001, - "message":"This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" + "id": 2001, + "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" } ] })"; @@ -128,15 +128,14 @@ TEST_F(WebRPCExecutorTest, WsNormalPath) static auto constexpr result = "{}"; static auto constexpr response = R"({ - "result":{ - }, - "id":99, - "status":"success", - "type":"response", - "warnings":[ + "result":{}, + "id": 99, + "status": "success", + "type": "response", + "warnings": [ { - "id":2001, - "message":"This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" + "id": 2001, + "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" } ] })"; @@ -175,10 +174,10 @@ TEST_F(WebRPCExecutorTest, HTTPForwardedPath) "forwarded": true, "warnings":[ { - "id":2001, - "message":"This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" + "id": 2001, + "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" } - ] + ] })"; EXPECT_CALL(*rpcEngine, buildResponse(testing::_)) .WillOnce(testing::Return(boost::json::parse(result).as_object())); @@ -207,19 +206,19 @@ TEST_F(WebRPCExecutorTest, WsForwardedPath) "index": 1 }, "forwarded": true - })"; + })"; static auto constexpr response = R"({ "result":{ "index": 1 - }, + }, "forwarded": true, - "id":99, - "status":"success", - "type":"response", - "warnings":[ + "id": 99, + "status": "success", + "type": "response", + "warnings": [ { - "id":2001, - "message":"This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" + "id": 2001, + "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" } ] })"; @@ -247,15 +246,15 @@ TEST_F(WebRPCExecutorTest, HTTPErrorPath) "method": "ledger", "params": [ { - "ledger_index": "xx" + "ledger_index": "xx" } ] } }, - "warnings":[ + "warnings": [ { - "id":2001, - "message":"This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" + "id": 2001, + "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" } ] })"; @@ -273,7 +272,6 @@ TEST_F(WebRPCExecutorTest, HTTPErrorPath) })"; EXPECT_CALL(*rpcEngine, buildResponse(testing::_)) .WillOnce(testing::Return(RPC::Status{RPC::RippledError::rpcINVALID_PARAMS, "ledgerIndexMalformed"})); - EXPECT_CALL(*rpcEngine, notifyErrored("ledger")).Times(1); EXPECT_CALL(*etl, lastCloseAgeSeconds()).WillOnce(testing::Return(45)); @@ -297,10 +295,10 @@ TEST_F(WebRPCExecutorTest, WsErrorPath) "ledger_index": "xx", "id": "123" }, - "warnings":[ + "warnings": [ { - "id":2001, - "message":"This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" + "id": 2001, + "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" } ] })"; @@ -315,7 +313,6 @@ TEST_F(WebRPCExecutorTest, WsErrorPath) })"; EXPECT_CALL(*rpcEngine, buildResponse(testing::_)) .WillOnce(testing::Return(RPC::Status{RPC::RippledError::rpcINVALID_PARAMS, "ledgerIndexMalformed"})); - EXPECT_CALL(*rpcEngine, notifyErrored("ledger")).Times(1); EXPECT_CALL(*etl, lastCloseAgeSeconds()).WillOnce(testing::Return(45)); @@ -332,23 +329,21 @@ TEST_F(WebRPCExecutorTest, HTTPNotReady) })"; static auto constexpr response = R"({ - "result":{ - "error":"notReady", - "error_code":13, - "error_message":"Not ready to handle this request.", - "status":"error", - "type":"response", - "request":{ - "method":"server_info", - "params":[ - { - - } - ] + "result": { + "error": "notReady", + "error_code": 13, + "error_message": "Not ready to handle this request.", + "status": "error", + "type": "response", + "request": { + "method": "server_info", + "params": [{}] } } })"; + EXPECT_CALL(*rpcEngine, notifyNotReady).Times(1); + (*rpcExecutor)(std::move(request), session); std::this_thread::sleep_for(200ms); EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response)); @@ -364,18 +359,20 @@ TEST_F(WebRPCExecutorTest, WsNotReady) })"; static auto constexpr response = R"({ - "error":"notReady", - "error_code":13, - "error_message":"Not ready to handle this request.", - "status":"error", - "type":"response", - "id":99, - "request":{ - "command":"server_info", - "id":99 + "error": "notReady", + "error_code": 13, + "error_message": "Not ready to handle this request.", + "status": "error", + "type": "response", + "id": 99, + "request": { + "command": "server_info", + "id": 99 } })"; + EXPECT_CALL(*rpcEngine, notifyNotReady).Times(1); + (*rpcExecutor)(std::move(request), session); std::this_thread::sleep_for(200ms); EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response)); @@ -390,18 +387,20 @@ TEST_F(WebRPCExecutorTest, HTTPBadSyntax) static auto constexpr response = R"({ "result":{ - "error":"badSyntax", - "error_code":1, - "error_message":"Syntax error.", - "status":"error", - "type":"response", - "request":{ - "method2":"server_info", - "params":[{}] + "error": "badSyntax", + "error_code": 1, + "error_message": "Syntax error.", + "status": "error", + "type": "response", + "request": { + "method2": "server_info", + "params": [{}] } } })"; + EXPECT_CALL(*rpcEngine, notifyBadSyntax).Times(1); + (*rpcExecutor)(std::move(request), session); std::this_thread::sleep_for(200ms); EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response)); @@ -415,19 +414,21 @@ TEST_F(WebRPCExecutorTest, HTTPBadSyntaxWhenRequestSubscribe) mockBackendPtr->updateRange(MAXSEQ); // max static auto constexpr response = R"({ - "result":{ - "error":"badSyntax", - "error_code":1, - "error_message":"Syntax error.", - "status":"error", - "type":"response", - "request":{ - "method":"subscribe", - "params":[{}] + "result": { + "error": "badSyntax", + "error_code": 1, + "error_message": "Syntax error.", + "status": "error", + "type": "response", + "request": { + "method": "subscribe", + "params": [{}] } } })"; + EXPECT_CALL(*rpcEngine, notifyBadSyntax).Times(1); + (*rpcExecutor)(std::move(request), session); std::this_thread::sleep_for(200ms); EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response)); @@ -445,18 +446,20 @@ TEST_F(WebRPCExecutorTest, WsBadSyntax) mockBackendPtr->updateRange(MAXSEQ); // max static auto constexpr response = R"({ - "error":"badSyntax", - "error_code":1, - "error_message":"Syntax error.", - "status":"error", - "type":"response", - "id":99, + "error": "badSyntax", + "error_code": 1, + "error_message": "Syntax error.", + "status": "error", + "type": "response", + "id": 99, "request":{ - "command2":"server_info", - "id":99 + "command2": "server_info", + "id": 99 } })"; + EXPECT_CALL(*rpcEngine, notifyBadSyntax).Times(1); + (*rpcExecutor)(std::move(request), session); std::this_thread::sleep_for(200ms); EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response)); @@ -466,18 +469,14 @@ TEST_F(WebRPCExecutorTest, HTTPInternalError) { static auto constexpr response = R"({ "result": { - "error":"internal", - "error_code":73, - "error_message":"Internal error.", - "status":"error", - "type":"response", - "request":{ + "error": "internal", + "error_code": 73, + "error_message": "Internal error.", + "status": "error", + "type": "response", + "request": { "method": "ledger", - "params": [ - { - - } - ] + "params": [{}] } } })"; @@ -487,12 +486,10 @@ TEST_F(WebRPCExecutorTest, HTTPInternalError) static auto constexpr requestJSON = R"({ "method": "ledger", - "params": [ - { - - } - ] + "params": [{}] })"; + + EXPECT_CALL(*rpcEngine, notifyInternalError).Times(1); EXPECT_CALL(*rpcEngine, buildResponse(testing::_)).Times(1).WillOnce(testing::Throw(std::runtime_error("MyError"))); (*rpcExecutor)(std::move(requestJSON), session); @@ -505,15 +502,15 @@ TEST_F(WebRPCExecutorTest, WsInternalError) session->upgraded = true; static auto constexpr response = R"({ - "error":"internal", - "error_code":73, - "error_message":"Internal error.", - "status":"error", - "type":"response", - "id":"123", - "request":{ - "command":"ledger", - "id":"123" + "error": "internal", + "error_code": 73, + "error_message": "Internal error.", + "status": "error", + "type": "response", + "id": "123", + "request": { + "command": "ledger", + "id": "123" } })"; @@ -524,6 +521,8 @@ TEST_F(WebRPCExecutorTest, WsInternalError) "command": "ledger", "id": "123" })"; + + EXPECT_CALL(*rpcEngine, notifyInternalError).Times(1); EXPECT_CALL(*rpcEngine, buildResponse(testing::_)).Times(1).WillOnce(testing::Throw(std::runtime_error("MyError"))); (*rpcExecutor)(std::move(requestJSON), session); @@ -543,17 +542,17 @@ TEST_F(WebRPCExecutorTest, HTTPOutDated) static auto constexpr result = "{}"; static auto constexpr response = R"({ - "result":{ - "status":"success" + "result": { + "status": "success" }, - "warnings":[ + "warnings": [ { - "id":2001, - "message":"This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" + "id": 2001, + "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" }, { - "id":2002, - "message":"This server may be out of date" + "id": 2002, + "message": "This server may be out of date" } ] })"; @@ -582,19 +581,18 @@ TEST_F(WebRPCExecutorTest, WsOutdated) static auto constexpr result = "{}"; static auto constexpr response = R"({ - "result":{ - }, - "id":99, - "status":"success", - "type":"response", + "result":{}, + "id": 99, + "status": "success", + "type": "response", "warnings":[ { - "id":2001, - "message":"This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" + "id": 2001, + "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request" }, { - "id":2002, - "message":"This server may be out of date" + "id": 2002, + "message": "This server may be out of date" } ] })"; @@ -613,9 +611,9 @@ TEST_F(WebRPCExecutorTest, WsTooBusy) { session->upgraded = true; - auto rpcEngine2 = std::make_shared(); - auto rpcExecutor2 = - std::make_shared>(cfg, mockBackendPtr, rpcEngine2, etl, subManager); + auto localRpcEngine = std::make_shared(); + auto localRpcExecutor = std::make_shared>( + cfg, mockBackendPtr, localRpcEngine, etl, subManager); static auto constexpr request = R"({ "command": "server_info", "id": 99 @@ -626,22 +624,25 @@ TEST_F(WebRPCExecutorTest, WsTooBusy) static auto constexpr response = R"({ - "error":"tooBusy", - "error_code":9, - "error_message":"The server is too busy to help you now.", - "status":"error", - "type":"response" + "error": "tooBusy", + "error_code": 9, + "error_message": "The server is too busy to help you now.", + "status": "error", + "type": "response" })"; - EXPECT_CALL(*rpcEngine2, post).WillOnce(testing::Return(false)); - (*rpcExecutor2)(std::move(request), session); + + EXPECT_CALL(*localRpcEngine, notifyTooBusy).Times(1); + EXPECT_CALL(*localRpcEngine, post).WillOnce(testing::Return(false)); + + (*localRpcExecutor)(std::move(request), session); EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response)); } TEST_F(WebRPCExecutorTest, HTTPTooBusy) { - auto rpcEngine2 = std::make_shared(); - auto rpcExecutor2 = - std::make_shared>(cfg, mockBackendPtr, rpcEngine2, etl, subManager); + auto localRpcEngine = std::make_shared(); + auto localRpcExecutor = std::make_shared>( + cfg, mockBackendPtr, localRpcEngine, etl, subManager); static auto constexpr request = R"({ "method": "server_info", "params": [{}] @@ -652,15 +653,17 @@ TEST_F(WebRPCExecutorTest, HTTPTooBusy) static auto constexpr response = R"({ - "error":"tooBusy", - "error_code":9, - "error_message":"The server is too busy to help you now.", - "status":"error", - "type":"response" + "error": "tooBusy", + "error_code": 9, + "error_message": "The server is too busy to help you now.", + "status": "error", + "type": "response" })"; - EXPECT_CALL(*rpcEngine2, post).WillOnce(testing::Return(false)); - (*rpcExecutor2)(std::move(request), session); + EXPECT_CALL(*localRpcEngine, notifyTooBusy).Times(1); + EXPECT_CALL(*localRpcEngine, post).WillOnce(testing::Return(false)); + + (*localRpcExecutor)(std::move(request), session); EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response)); } @@ -669,13 +672,15 @@ TEST_F(WebRPCExecutorTest, HTTPRequestNotJson) static auto constexpr request = "not json"; static auto constexpr response = R"({ - "error":"badSyntax", - "error_code":1, - "error_message":"Syntax error.", - "status":"error", - "type":"response" + "error": "badSyntax", + "error_code": 1, + "error_message": "Syntax error.", + "status": "error", + "type": "response" })"; + EXPECT_CALL(*rpcEngine, notifyBadSyntax).Times(1); + (*rpcExecutor)(std::move(request), session); EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response)); } @@ -686,13 +691,15 @@ TEST_F(WebRPCExecutorTest, WsRequestNotJson) static auto constexpr request = "not json"; static auto constexpr response = R"({ - "error":"badSyntax", - "error_code":1, - "error_message":"Syntax error.", - "status":"error", - "type":"response" + "error": "badSyntax", + "error_code": 1, + "error_message": "Syntax error.", + "status": "error", + "type": "response" })"; + EXPECT_CALL(*rpcEngine, notifyBadSyntax).Times(1); + (*rpcExecutor)(std::move(request), session); EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response)); }