Skip to content
This repository has been archived by the owner on Jun 12, 2018. It is now read-only.

Common polymorphic base class #168

Open
killoctal opened this issue Nov 15, 2017 · 15 comments
Open

Common polymorphic base class #168

killoctal opened this issue Nov 15, 2017 · 15 comments

Comments

@killoctal
Copy link

Hi

Will it be possible to have a base class without template for being able to create a polymorphic instance ?
For now it is impossible to do something like this :

Server* server = (ssl) ? new HttpsServer() : new HttpServer();
server->start(); // <- polymorphic call

For now I extracted a part of the code in a new class called HttpServerGeneric but so I broke compatibility with your code...

Thanks
Gabriel

@eidheim
Copy link
Owner

eidheim commented Nov 15, 2017

You raise an interesting question, but the main problem is that the socket type (HTTP or HTTPS) has to be decided at compile time. This means that for instance ServerBase::resource has to be build at compile time due to its value type, handler function, and its parameters (response and request types) are dependent on the socket type.

One could create a Server interface but this interface could not contain for instance the resource map. This interface would in other words not be very useful other than setting config values and running start() and stop(). Still, I'll leave this issue open for further discussion on this topic.

@SethHamilton
Copy link
Contributor

SethHamilton commented Nov 22, 2017

I don't have the HTTPS code incorporated yet, but to avoid this issue when I get to it, I've abstracted the web server completely to the point where there is essentially just a Message object, which is created when a request comes in. The message object is queued and the queue is drained by a pool of worker threads waiting on a condition_variable.

This message can be passed around my application. The Message object contains reply functions, those reply functions call a callback which is implemented using a lambda created in a templated factory function MakeMessage. This allows the type information (HTTP and eventually HTTPS) to be "hidden" in the closure.

The worker pool is also in these files. I do my resource mapping a little further up in my workers (that function call is here: https://github.com/opset/openset/blob/master/src/http_serve.cpp#L90). The maps are defined here: https://github.com/opset/openset/blob/master/src/rpc.h#L122-L154 and the function that dispatches to functions that do stuff in my app are here: https://github.com/opset/openset/blob/master/src/rpc.cpp#L2352-L2375

@killoctal
Copy link
Author

Hi,

Sorry for long reply time.

The goal was to make a REST/JSON server which hides HttpServer implementation but allows anyway to give a custom instance (this is justconcept code) :

class ServerSOA
{
public:
	ServerSOA(int port) : ServerSOA(new HttpServer(port)) { }
	ServerSOA(HttpServerGeneric* server) : _server(server)
	{
		_server->start();
	}

private:
	HttpServerGeneric* _server;
};

ServerSOA server1(80); // normal http
ServerSOA server2(new HttpsServer(81)); // http with SSL
ServerSOA server2(new SuperComplicatedHttpServer(82)); // relly customized server

// server1, server2, server3 were started automatically

Here is the pseudo-code of my generic server (sorry it is from at least 1 year old version of your project). The required generic features are :

  • start/stop
  • resources add
  • HTTP codes
class HttpServerGeneric
{
public:
	virtual ~HttpServerGeneric() {}

	class Request { /* full implementation */};
	class ResponseGeneric { /* minimal code */ };
	enum HTTP_RETURN_CODE { /* all enum values */};

	using ResourceMethod = std::function<void(ResponseGeneric&, std::shared_ptr<Request>)>;

	virtual void start(bool background) = 0;
	virtual void stop() = 0;
	std::unordered_map<std::string, std::unordered_map<std::string, ResourceMethod> >  resource;
	std::unordered_map<std::string, ResourceMethod> default_resource;
protected:
	std::vector<std::pair<std::string, std::vector<std::pair<std::regex, ResourceMethod > > > > opt_resource;
};
using HTTPCode = HttpServerGeneric::HTTP_RETURN_CODE;

Since I manage to do it, do you think you could also do it ?

Thank you, best regards !
Gabriel

@killoctal
Copy link
Author

I just precise that my ServerSOA system already works but I broke compatibility so I can't use your updates for now unfortunately

@moodboom
Copy link
Contributor

I have definitely needed to add common functionality and use pointers and refs to clients and servers where a non-template base class would help. I vote Yes if it makes sense for the project at some point. Thanks eidheim!

@eidheim
Copy link
Owner

eidheim commented Jan 25, 2018

Thank you all for great feedback, and sorry for being absent in this thread. I have however given this some thought from time to time.

The problem as I see it is that ServerBase::Response::send is dependent on ServerBase::Connection that again has socket_type socket. Thus the Response class would need to be built at compile time depending on if you use HTTP or HTTPS and could not be part of a non-template base class. This again makes it impossible to put the resource handlers in a non-template base class as well. Now if asio::ssl::stream and asio::ip::tcp::socket shared a base class this issue could be solved, but they do not as far as I can see.

If someone has a solution for this, a PR is most welcome, or at least enlighten me on how this could be solved:)

@moodboom
Copy link
Contributor

moodboom commented Jan 28, 2018 via email

@ttislerdg
Copy link

I ended up tackling this over the last couple of days. What I did was to eliminate the templating on ServerBase and its related inner-classes. I derived an Http/HttpsConnection class and Http/HttpsResponse class to store the specifics.

I then pulled out the uses of sockets (asio:ssl::stream and asio:ip:tcp::socket) and put them in virtual methods, which are called by ServerBase. The extensive use of lambdas caused a number of headaches, but I think I got them worked out.

I really didn't change any of the executable code, just where it lives.
I'm relatively unfamiliar with the http/https transport, so I'm not sure I've done this correctly or if there is any way to reduce the specific code even more.

I've tested using my app (currently only HTTP) and using the http_examples.cpp and https_examples.cpp examples.

You can see my changes here. If they look OK, I'll submit a pull-request.
https://github.com/ttislerdg/Simple-Web-Server

Hope this helps

@eidheim
Copy link
Owner

eidheim commented Feb 4, 2018

@ttislerdg Thank you! I'll have to study your changes in more detail, but it seems that you have found a solution that might very well have solved this issue. A pull request is most welcome, and we'll take it from there.

@eidheim
Copy link
Owner

eidheim commented Feb 16, 2018

I also made an attempt in https://github.com/eidheim/Simple-Web-Server/tree/base_class. Feedback is welcome.

@eidheim
Copy link
Owner

eidheim commented Mar 21, 2018

After some thought, I think I have come to the conclusion that making a non-templated base class complicates that source code too much with respect to what is achieved. Also keep in mind that for instance std::vector does not have a base class, so in a way the design patterns from the standard library is in a way currently followed instead.

That said, one can still define the resource-handlers at one place and reuse them in both a HTTP and HTTPS server. For instance, this works using c++14:

#include "server_https.hpp"

using namespace std;

using HttpServer = SimpleWeb::Server<SimpleWeb::HTTP>;
using HttpsServer = SimpleWeb::Server<SimpleWeb::HTTPS>;

int main() {
  auto io_service = std::make_shared<boost::asio::io_service>();

  HttpServer http_server;
  http_server.config.port = 8080;
  http_server.io_service = io_service;

  HttpsServer https_server("server.crt", "server.key");
  https_server.config.port = 8081;
  https_server.io_service = io_service;

  auto default_resource = [](auto response, auto /*request*/) {
    response->write("Hello World");
  };

  http_server.default_resource["GET"] = default_resource;
  https_server.default_resource["GET"] = default_resource;

  http_server.start();
  https_server.start();

  io_service->run();
}

This issue is still however open for discussion:)

@killoctal
Copy link
Author

Hello
The fact is that choose HTTP/HTTPS should be basically a parameter, like new HttpServer(Protocol::HTTPS)
For me the current configuration is like a workaround which became the standard way to do :-/

@killoctal
Copy link
Author

Or maybe using a factory, like :

HttpServer httpServer = HttpServer::create(8080)
HttpServer httpsServer = HttpServer::createHttps(8081, "server.crt", "server.key");

@eidheim
Copy link
Owner

eidheim commented Mar 23, 2018

I found some interesting discussion around OOP/Templates here: https://stackoverflow.com/questions/1039853/why-is-the-c-stl-is-so-heavily-based-on-templates-and-not-on-interfaces. I'm not agreeing with everything that is said in the replies for this stackoverflow-question, but I think a healthy balance between OOP, template and functional programming can be achieved.

Regarding the original issue post, here is one solution using templates:

#include "server_https.hpp"

using namespace std;

using HttpServer = SimpleWeb::Server<SimpleWeb::HTTP>;
using HttpsServer = SimpleWeb::Server<SimpleWeb::HTTPS>;

template <class T>
void setup_and_start(T &server) {
  server.config.port = 8080;

  server.default_resource["GET"] = [](auto response, auto /*request*/) {
    response->write("Hello World");
  };

  server.start();
}

int main() {
  bool use_https = false;

  if(use_https) {
    HttpsServer server("server.crt", "server.key");
    setup_and_start(server);
  }
  else {
    HttpServer server;
    setup_and_start(server);
  }
}

On the plus side, using templates simplifies the library code. This might seem like a small argument, but in an open source project like this it is important that potential contributors can understand the code. On the other hand, template programming makes it harder for static analysing tools to identify problems, and IDE tooling is lacking when it comes to making sense of code in template functions and classes.

@moodboom
Copy link
Contributor

moodboom commented Mar 23, 2018 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants