Simple-Web-Server icon indicating copy to clipboard operation
Simple-Web-Server copied to clipboard

Common polymorphic base class

Open killoctal opened this issue 8 years ago • 15 comments

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

killoctal avatar Nov 15 '17 09:11 killoctal

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.

eidheim avatar Nov 15 '17 20:11 eidheim

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.

  • Message class is here: https://github.com/opset/openset/blob/master/src/http_serve.h#L27-L55
  • MakeMessage (the lambdaa) is created here: https://github.com/opset/openset/blob/master/src/http_serve.cpp#L39-L59
  • Templated resource mapping is here: https://github.com/opset/openset/blob/master/src/http_serve.cpp#L119-L150

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

SethHamilton avatar Nov 22 '17 17:11 SethHamilton

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 avatar Nov 23 '17 09:11 killoctal

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

killoctal avatar Nov 23 '17 09:11 killoctal

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!

moodboom avatar Jan 22 '18 20:01 moodboom

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:)

eidheim avatar Jan 25 '18 16:01 eidheim

Thinking intial thought out loud... maybe use stream and socket pointers in the base, init to 0, and only created by derived class as needed? Or accessors, eg virtual asio::ssl::stream* get_stream() { return 0; } in base. Very rough thought.

On Thu, Jan 25, 2018 at 11:53 AM, Ole Christian Eidheim < [email protected]> wrote:

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 http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/ssl__stream.html and asio::ip::tcp::socket http://www.boost.org/doc/libs/1_66_0/doc/html/boost_asio/reference/ip__tcp/socket.html 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:)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/eidheim/Simple-Web-Server/issues/168#issuecomment-360528467, or mute the thread https://github.com/notifications/unsubscribe-auth/ABt1YK7WZnaa8O5VaHRx9cWy33VUa1sdks5tOLGcgaJpZM4QeqOv .

--

news https://bitpost.com/news | trade https://abettertrader.com/ | blog https://bitpost.com/blog

moodboom avatar Jan 28 '18 18:01 moodboom

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

ttislerdg avatar Feb 02 '18 15:02 ttislerdg

@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 avatar Feb 04 '18 15:02 eidheim

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

eidheim avatar Feb 16 '18 14:02 eidheim

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:)

eidheim avatar Mar 21 '18 11:03 eidheim

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 avatar Mar 22 '18 09:03 killoctal

Or maybe using a factory, like :

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

killoctal avatar Mar 22 '18 09:03 killoctal

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.

eidheim avatar Mar 23 '18 13:03 eidheim

I think most users of the code, that is, developers who need to use an http/https library, would prefer the interface be as simple as possible. For me, that would mean encapsulating everything in one easy interface, and making as invisible as possible any difference between http and https. A few parameters and away you go. Having to actually have a different template parameter (as opposed to driven entirely by variables) makes use of the library less convenient in some cases. That seems to be a hard metric to override.

On the other side of the coin, I think your IMPLEMENTATION with templates is extremely elegant - probably the most elegant solution possible. It would be a major change to have to give up that elegant implementation. So there's the rub. Not sure of the conclusion! Just mostly confirming the conflict. :-)

On Fri, Mar 23, 2018 at 9:55 AM, Ole Christian Eidheim < [email protected]> wrote:

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::ServerSimpleWeb::HTTP;using HttpsServer = SimpleWeb::ServerSimpleWeb::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.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/eidheim/Simple-Web-Server/issues/168#issuecomment-375672530, or mute the thread https://github.com/notifications/unsubscribe-auth/ABt1YGFFJZ3m6rYq_ne9y8mZ5IAFZxp4ks5thP7tgaJpZM4QeqOv .

--

news https://bitpost.com/news | trade https://abettertrader.com/ | blog https://bitpost.com/blog

moodboom avatar Mar 23 '18 20:03 moodboom