Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Trivial: Rename ServerHandlerImp to ServerHandler #4516

Merged
merged 1 commit into from
Jun 28, 2023
Merged

Trivial: Rename ServerHandlerImp to ServerHandler #4516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Builds/CMake/RippledCore.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,7 @@ target_sources (rippled PRIVATE
src/ripple/rpc/impl/RPCHandler.cpp
src/ripple/rpc/impl/RPCHelpers.cpp
src/ripple/rpc/impl/Role.cpp
src/ripple/rpc/impl/ServerHandlerImp.cpp
src/ripple/rpc/impl/ServerHandler.cpp
src/ripple/rpc/impl/ShardArchiveHandler.cpp
src/ripple/rpc/impl/ShardVerificationScheduler.cpp
src/ripple/rpc/impl/Status.cpp
Expand Down
199 changes: 189 additions & 10 deletions src/ripple/rpc/ServerHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,200 @@
#ifndef RIPPLE_RPC_SERVERHANDLER_H_INCLUDED
#define RIPPLE_RPC_SERVERHANDLER_H_INCLUDED

#include <ripple/basics/BasicConfig.h>
#include <ripple/beast/utility/Journal.h>
#include <ripple/core/Config.h>
#include <ripple/app/main/CollectorManager.h>
#include <ripple/core/JobQueue.h>
#include <ripple/resource/ResourceManager.h>
#include <ripple/rpc/impl/ServerHandlerImp.h>
#include <ripple/server/Port.h>
#include <boost/asio/io_service.hpp>
#include <boost/asio/ip/address.hpp>
#include <memory>
#include <ripple/json/Output.h>
#include <ripple/rpc/RPCHandler.h>
#include <ripple/rpc/impl/WSInfoSub.h>
#include <ripple/server/Server.h>
#include <ripple/server/Session.h>
#include <ripple/server/WSSession.h>
#include <boost/beast/core/tcp_stream.hpp>
#include <boost/beast/ssl/ssl_stream.hpp>
#include <boost/utility/string_view.hpp>
#include <condition_variable>
#include <map>
#include <mutex>
#include <vector>

namespace ripple {

using ServerHandler = ServerHandlerImp;
inline bool
operator<(Port const& lhs, Port const& rhs)
{
return lhs.name < rhs.name;
}

class ServerHandler
{
public:
struct Setup
{
explicit Setup() = default;

std::vector<Port> ports;

// Memberspace
struct client_t
{
explicit client_t() = default;

bool secure = false;
std::string ip;
std::uint16_t port = 0;
std::string user;
std::string password;
std::string admin_user;
std::string admin_password;
};

// Configuration when acting in client role
client_t client;

// Configuration for the Overlay
struct overlay_t
{
explicit overlay_t() = default;

boost::asio::ip::address ip;
std::uint16_t port = 0;
};

overlay_t overlay;

void
makeContexts();
};

private:
using socket_type = boost::beast::tcp_stream;
using stream_type = boost::beast::ssl_stream<socket_type>;

Application& app_;
Resource::Manager& m_resourceManager;
beast::Journal m_journal;
NetworkOPs& m_networkOPs;
std::unique_ptr<Server> m_server;
Setup setup_;
JobQueue& m_jobQueue;
beast::insight::Counter rpc_requests_;
beast::insight::Event rpc_size_;
beast::insight::Event rpc_time_;
std::mutex mutex_;
std::condition_variable condition_;
bool stopped_{false};
std::map<std::reference_wrapper<Port const>, int> count_;

// A private type used to restrict access to the ServerHandler constructor.
struct ServerHandlerCreator
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about using other types as a private tag in constructors. Can we use an int or a char or some static variable (in the private access specifier of class) as a tag?

I know that this struct is being passed with const& and may not incur high overhead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First it's worth noting that we're pushing C++ to do something it's not naturally inclined to do. We're trying to limit who is allowed to construct this type.

Let's look at how ServerHandlerCreator is being used. Here's the implementation of make_ServerHandler :

std::unique_ptr<ServerHandler>
make_ServerHandler(
    Application& app,
    boost::asio::io_service& io_service,
    JobQueue& jobQueue,
    NetworkOPs& networkOPs,
     Resource::Manager& resourceManager,
     CollectorManager& cm)
 {
     return std::make_unique<ServerHandler>(
         ServerHandler::ServerHandlerCreator(),
         app,
         io_service,
         jobQueue,
         networkOPs,
         resourceManager,
         cm);
 }

Note that make_ServerHandler() does not return a ServerHandler or even a reference to a ServerHandler. It returns a std::unique_ptr<ServerHandler>. And it does so by calling std::make_unique(). This is awkward because somewhere in the depths of std::make_unique the constructor of ServerHandler will be invoked. We have no idea where in the standard library that invocation will occur. So we can't declare the part of the standard library as a friend. And even if we could, different standard library implementations will do it differently.

The work around is to pass a unique type (e.g. ServerHandlerCreator) to the constructor of ServerHandler. We can make sure that only make_ServerHandler() has access to that type by making the type private and then declaring make_ServerHandler() a friend. Once make_ServerHandler() has constructed that unique type, the type can be passed around to anyone.

In effect we've made a way for only make_serverHandler() to give permission to std::make_unique() to construct a ServerHandler.

We have to use a type for this. Parameter lists are sensitive to types. If we used an int or a char to grant this permission, well, anyone can pass an int or a char. If we make the permission thing a type, then the compiler can assure us that the correct type is being passed as a parameter.

Make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, that makes a lot of sense. Yeah, we can't impersonate types, understood.

{
explicit ServerHandlerCreator() = default;
};

// Friend declaration that allows make_ServerHandler to access the
// private type that restricts access to the ServerHandler ctor.
friend std::unique_ptr<ServerHandler>
make_ServerHandler(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please point me to resources about this make_XYX design pattern? I've read about the factory design pattern and I'm trying to understand it better.

  1. Why not make this function a member of the class, instead of declaring it as a friend?
  2. Why do we need this function? It appears to be calling a constructor of the class (without any additional preprocessing). Can't we make the said constructor public, thereby reducing the number of indirections.

I'm new to this design pattern, I'd like to learn more about it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why not make this function a member of the class, instead of declaring it as a friend?

It could be a static member of the class. I'm not sure why a free function was preferred over a static member in this case. It's probably for uniformity. There are quite a number of other make_ factory functions around the code base. They are easier for developers to use and think about if they have similar interfaces and behavior.

  1. Why do we need this function?

Factory functions can have a number of different purposes. In this case it's because we want to prevent anyone from constructing a ServerHandler that is not held by a std::unique_ptr<ServerHandler>. If we made the ServerHandler constructor public, then anyone could create one. So, in this case, we give the factory the permission to allow the construction and trust that the factory will do the right thing with the unique_ptr. And we don't have to trust very hard, since the implementation for make_ServerHandler() is in the implementation file for ServerHandler.

Factory functions can be used for other purposes as well. For example, the Meyer's Singleton is (in effect) a factory function: https://en.wikipedia.org/wiki/Singleton_pattern

The (ancient) Gang of Four Design Patterns book has a chapter on Factory Method.

Factory functions really just solve the problem that sometimes a simple constructor can't do quite enough. The factory function can do the extra work that is required.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm okay. I'll check out these resources 👍

Application& app,
boost::asio::io_service&,
JobQueue&,
NetworkOPs&,
Resource::Manager&,
CollectorManager& cm);

public:
// Must be public so make_unique can call it.
ServerHandler(
ServerHandlerCreator const&,
Application& app,
boost::asio::io_service& io_service,
JobQueue& jobQueue,
NetworkOPs& networkOPs,
Resource::Manager& resourceManager,
CollectorManager& cm);

~ServerHandler();

using Output = Json::Output;

void
setup(Setup const& setup, beast::Journal journal);

Setup const&
setup() const
{
return setup_;
}

void
stop();

//
// Handler
//

bool
onAccept(Session& session, boost::asio::ip::tcp::endpoint endpoint);

Handoff
onHandoff(
Session& session,
std::unique_ptr<stream_type>&& bundle,
http_request_type&& request,
boost::asio::ip::tcp::endpoint const& remote_address);

Handoff
onHandoff(
Session& session,
http_request_type&& request,
boost::asio::ip::tcp::endpoint const& remote_address)
{
return onHandoff(
session,
{},
std::forward<http_request_type>(request),
remote_address);
}

void
onRequest(Session& session);

void
onWSMessage(
std::shared_ptr<WSSession> session,
std::vector<boost::asio::const_buffer> const& buffers);

void
onClose(Session& session, boost::system::error_code const&);

void
onStopped(Server&);

private:
Json::Value
processSession(
std::shared_ptr<WSSession> const& session,
std::shared_ptr<JobQueue::Coro> const& coro,
Json::Value const& jv);

void
processSession(
std::shared_ptr<Session> const&,
std::shared_ptr<JobQueue::Coro> coro);

void
processRequest(
Port const& port,
std::string const& request,
beast::IP::Endpoint const& remoteIPAddress,
Output&&,
std::shared_ptr<JobQueue::Coro> coro,
boost::string_view forwardedFor,
boost::string_view user);

Handoff
statusResponse(http_request_type const& request) const;
};

ServerHandler::Setup
setup_ServerHandler(Config const& c, std::ostream&& log);
Expand Down
43 changes: 25 additions & 18 deletions src/ripple/rpc/impl/ServerHandlerImp.cpp → src/ripple/rpc/impl/ServerHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
*/
//==============================================================================

#include <ripple/rpc/ServerHandler.h>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay to leave an extra line in the include headers? I hoped the clang-format command took care of extra whitespace lines.

My apologies, I don't mean to be anal about coding style here, just wanted to bring it to your attention.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line of whitespace is allowed by clang-format and actually serves a purpose.

We ask clang-format to alphabetize our #include lines. That helps make it more obvious when the same file is included twice. Sounds silly, but it happens when there are lots of #includes.

But there are situations where we don't want things alphabetized. Here is one of them. In the book Large Scale C++ Software Design, John Lakos recommends that in an implementation file the first file that should be included is the header file for that implementation. Here's the principle he gives for that rule (pg 111 of the edition I own):

Latent usage errors can be avoided by ensuring that the .h file of a component parses by itself--without externally-provided declarations or definitions.

By placing that header first the compiler can show us that the header itself includes all of the headers it needs to be complete.

Suppose that a header relied in <vector> but did not #include <vector> itself or through some other include. As long as that header is included elsewhere after <vector> has already been included, then everything builds and the problem is hidden.

If there's at least one place where the header is included first, then the compiler can assure us that the header contains everything it needs to stand alone. Mr. Lakos suggests that one place can be the implementation file. We follow his advice.

So, back to the line of whitespace.

clang-format only alphabetizes contiguous #includes, without intervening whitespace. So by putting in the whitespace we can keep clang-format from pushing this #include out of its top-most position.

We may also use a line of white space between ripple #includes and system #includes. We want the more-local headers to be included first, followed by more general headers. That line of whitespace prevents clang-format from stirring them all together.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is really helpful, thanks for letting me know about the convention 👍
To paraphrase what I understood : It is good to include the header file of an implementation at the beginning of the implementation file. This helps in providing a self-contained list of header files for the implementation right?

I understood the dependency ordering of the #include's in the third paragraph from the end 👍

#include <ripple/app/main/Application.h>
#include <ripple/app/misc/NetworkOPs.h>
#include <ripple/basics/Log.h>
Expand All @@ -35,9 +37,7 @@
#include <ripple/resource/ResourceManager.h>
#include <ripple/rpc/RPCHandler.h>
#include <ripple/rpc/Role.h>
#include <ripple/rpc/ServerHandler.h>
#include <ripple/rpc/impl/RPCHelpers.h>
#include <ripple/rpc/impl/ServerHandlerImp.h>
#include <ripple/rpc/impl/Tuning.h>
#include <ripple/rpc/json_body.h>
#include <ripple/server/Server.h>
Expand Down Expand Up @@ -101,7 +101,8 @@ authorized(Port const& port, std::map<std::string, std::string> const& h)
return strUser == port.user && strPassword == port.password;
}

ServerHandlerImp::ServerHandlerImp(
ServerHandler::ServerHandler(
ServerHandlerCreator const&,
Application& app,
boost::asio::io_service& io_service,
JobQueue& jobQueue,
Expand All @@ -121,13 +122,13 @@ ServerHandlerImp::ServerHandlerImp(
rpc_time_ = group->make_event("time");
}

ServerHandlerImp::~ServerHandlerImp()
ServerHandler::~ServerHandler()
{
m_server = nullptr;
}

void
ServerHandlerImp::setup(Setup const& setup, beast::Journal journal)
ServerHandler::setup(Setup const& setup, beast::Journal journal)
{
setup_ = setup;
m_server->ports(setup.ports);
Expand All @@ -136,7 +137,7 @@ ServerHandlerImp::setup(Setup const& setup, beast::Journal journal)
//------------------------------------------------------------------------------

void
ServerHandlerImp::stop()
ServerHandler::stop()
{
m_server->close();
{
Expand All @@ -148,7 +149,7 @@ ServerHandlerImp::stop()
//------------------------------------------------------------------------------

bool
ServerHandlerImp::onAccept(
ServerHandler::onAccept(
Session& session,
boost::asio::ip::tcp::endpoint endpoint)
{
Expand All @@ -170,7 +171,7 @@ ServerHandlerImp::onAccept(
}

Handoff
ServerHandlerImp::onHandoff(
ServerHandler::onHandoff(
Session& session,
std::unique_ptr<stream_type>&& bundle,
http_request_type&& request,
Expand Down Expand Up @@ -272,7 +273,7 @@ buffers_to_string(ConstBufferSequence const& bs)
}

void
ServerHandlerImp::onRequest(Session& session)
ServerHandler::onRequest(Session& session)
{
// Make sure RPC is enabled on the port
if (session.port().protocol.count("http") == 0 &&
Expand Down Expand Up @@ -312,7 +313,7 @@ ServerHandlerImp::onRequest(Session& session)
}

void
ServerHandlerImp::onWSMessage(
ServerHandler::onWSMessage(
std::shared_ptr<WSSession> session,
std::vector<boost::asio::const_buffer> const& buffers)
{
Expand Down Expand Up @@ -362,14 +363,14 @@ ServerHandlerImp::onWSMessage(
}

void
ServerHandlerImp::onClose(Session& session, boost::system::error_code const&)
ServerHandler::onClose(Session& session, boost::system::error_code const&)
{
std::lock_guard lock(mutex_);
--count_[session.port()];
}

void
ServerHandlerImp::onStopped(Server&)
ServerHandler::onStopped(Server&)
{
std::lock_guard lock(mutex_);
stopped_ = true;
Expand Down Expand Up @@ -398,7 +399,7 @@ logDuration(
}

Json::Value
ServerHandlerImp::processSession(
ServerHandler::processSession(
std::shared_ptr<WSSession> const& session,
std::shared_ptr<JobQueue::Coro> const& coro,
Json::Value const& jv)
Expand Down Expand Up @@ -545,7 +546,7 @@ ServerHandlerImp::processSession(

// Run as a coroutine.
void
ServerHandlerImp::processSession(
ServerHandler::processSession(
std::shared_ptr<Session> const& session,
std::shared_ptr<JobQueue::Coro> coro)
{
Expand Down Expand Up @@ -586,7 +587,7 @@ Json::Int constexpr forbidden = -32605;
Json::Int constexpr wrong_version = -32606;

void
ServerHandlerImp::processRequest(
ServerHandler::processRequest(
Port const& port,
std::string const& request,
beast::IP::Endpoint const& remoteIPAddress,
Expand Down Expand Up @@ -1022,7 +1023,7 @@ ServerHandlerImp::processRequest(
is reported, meaning the server can accept more connections.
*/
Handoff
ServerHandlerImp::statusResponse(http_request_type const& request) const
ServerHandler::statusResponse(http_request_type const& request) const
{
using namespace boost::beast::http;
Handoff handoff;
Expand Down Expand Up @@ -1252,8 +1253,14 @@ make_ServerHandler(
Resource::Manager& resourceManager,
CollectorManager& cm)
{
return std::make_unique<ServerHandlerImp>(
app, io_service, jobQueue, networkOPs, resourceManager, cm);
return std::make_unique<ServerHandler>(
ServerHandler::ServerHandlerCreator(),
app,
io_service,
jobQueue,
networkOPs,
resourceManager,
cm);
}

} // namespace ripple
Loading