Massive cleanup #4

saidinesh5 · 2018-05-25T11:53:07Z

Add support for multithreaded request processing
Enable streaming multipart downloads for reduced memory usage
Cleaned up the architecture a little by deleting a lots of code!

Upstream mongoose (and upstream libyuarel) are now submodules in vendor/

Removed support for WebSockets - for now.

Add support for multithreaded request processing Enable streaming multipart downloads for reduced memory usage Cleaned up the architecture a little by deleting a lots of code! Upstream mongoose (and upstream libyuarel) are now submodules in vendor/ Removed support for WebSockets - for now.

rferrazz · 2018-05-28T07:14:37Z

LICENSE

@@ -1,16 +1,24 @@
+The MIT License
+
+Copyright (c) 2007-2019 Dinesh Manajipet <dinesh.manajipet@proemion.com>


❓ Has been the license changed even on the upstream mongoose?

Oh there are 2 licenses. One is the license of upstream mongoose C library - which is GPL something and commercial something.

Then there is the license of the mongoose-cpp fork we started off with. That was a "fork" of mongoose C library - but the license is MIT. With this pull request - the upstream mongoose is just a git submodule inside vendor/ , and we rewrote so much of the mongoose-cpp library - thought to update the license.

rferrazz · 2018-05-28T07:22:23Z

lib/Request.cpp

+
+    void Request::setIsValid(bool value)
+    {
+        mIsValid = value;


What if two threads are calling this function? Is it possible?

Yes, it is possible:

mongoose-cpp/lib/Server.cpp

Line 265 in a1d9149

server->mCurrentRequests[c]->setIsValid(false);

The Server.cpp mostly uses this variable to notify any threads related to this Request when a connection closes, that this request is no longer valid.

Good catch. I had the Response::mIsValid an std::atomic_bool but forgot to make this one the same. Fixed.

rferrazz · 2018-05-28T07:22:31Z

lib/Request.cpp

+
+    void Request::setMultipartEntities(const std::vector<Request::MultipartEntity> &entities)
+    {
+        mMultipartEntities = entities;


What if two threads are calling this function? Is it possible?

Technically it is possible. So maybe we should ask the users to not do that.

This is mostly done after we finished getting a multipart request, before we call Controller::process (which is where a user can spawn new threads if they need).
A one time call really.

rferrazz · 2018-05-28T07:28:07Z

examples/basic_auth.cpp

-            addRoute("GET", "/hello", MyController, hello);
-        }
+public:
+    bool hello(const std::shared_ptr<Request>& req, const std::shared_ptr<Response>& res)


Is it possible that two handlers like this one gets the same response object? I mean is it possible that two threads are working on the same Response *? If so there's a problem because i saw response methods are not thread safe. If not why passing a shared pointer?

I can't see any owning transfer or sharing here. Why not just Response&?

Checked the rest of the code. Looks like shared pointers are everywhere. But I can't see what to share here. Looks like it should be unique_ptr or just rvalue. So we can transfer the ownership of request and response to other thread.

@rferrazz nope. before "handling" any request, the Server.cpp creates a request/response pair.

@asergunov the shared pointer is mostly there to support "end users" of this library to do multithreaded things.

Like here: https://github.com/Proemion/quarks-recovery-server/blob/massive_cleanup/src/recoverycontroller.cpp#L135

All the magic happens here:
https://github.com/Proemion/mongoose-cpp/blob/multithreaded-server/lib/Server.cpp#L43

That's where we create and destroy a request/response pair for each incoming connection. Then we simply use the controller to dispatch the request to the appropriate handler.

I do have to test the multi threaded server a lot though. Thinking of writing unit tests for it later today.

asergunov

That's totally unclear how ownership passes for arguments and class members. I don't think smart pointers is right tool here.

asergunov · 2018-05-28T08:44:01Z

examples/basic_auth.cpp

-            addRoute("GET", "/hello", MyController, hello);
-        }
+public:
+    bool hello(const std::shared_ptr<Request>& req, const std::shared_ptr<Response>& res)


I can't see any owning transfer or sharing here. Why not just Response&?

asergunov · 2018-05-28T09:07:39Z

examples/basic_auth.cpp

-            addRoute("GET", "/hello", MyController, hello);
-        }
+public:
+    bool hello(const std::shared_ptr<Request>& req, const std::shared_ptr<Response>& res)


Checked the rest of the code. Looks like shared pointers are everywhere. But I can't see what to share here. Looks like it should be unique_ptr or just rvalue. So we can transfer the ownership of request and response to other thread.

saidinesh5 · 2018-05-28T10:55:13Z

P.S I wish we were reviewing this repo, https://github.com/saidinesh5/mongoose-cpp/issues .. because that reflects the correct "relationship" between us and the upstream mongoose C library now.

The plan was to delete this repo and fork from my clone

asergunov · 2018-05-28T12:48:37Z

@saidinesh5 For threading it will be nice to use non-copyable objects to make sure they are using only from one thread at once

saidinesh5 · 2018-05-28T22:00:00Z

@asergunov but the threads are sharing the Request/Response objects because of the shared std::atomic_bool mIsValid in both of them. That variable is kind of used by the main thread to notify any other client threads when the incoming connection has been closed and there is no point in processing / sending things anymore.

asergunov · 2018-05-29T02:34:24Z

@saidinesh5 it will be nice to split server and client side API like that done in std::future-std::promise. They share the state but that's closed from the user. So only server will be able to mark response as invalid.

saidinesh5 · 2018-05-29T10:09:20Z

@asergunov but responses are also marked as invalid in the "client" too.. Responses are marked invalid

When you perform a "Send operation"
When the incoming connection is closed
whichever happens first.

The Send operation can happen in the main thread/some subthread created by the user.. That's how I couldn't use promises. Plus we don't have (easy) control over the event loop run by mongoose library. So we can't just wait for a promise to finish in the main thread that runs the event loop.

asergunov · 2018-05-29T12:34:44Z

@saidinesh5 I'm not about using using future/promise there. I'm just about pattern shared private part pattern.

It will be much cleaner if controller will take ownership of request/response objects. It could take it by rvalue or by unique_ptr. So controller could start new worker thread and pass ownership of request/response there. When response is filled up can just destroy it.

Server still could have control over request/response validity in case connection was closed but using shared private part

struct SharedValidity {
atomic_bool mIsValid;
};

class Server {

   std::unordered_map<Connection*, std::shared_ptr<SharedValidity>> mConnectionsValidity;

   std::tuple<Request, Response> wrapConnection(Connection *c) {
     auto validity = make_shared<SharedValidity>();
     mConnectionsValidity.emplace(c, validity);
     return {{c, validity}, {c, validity}};
   }

   void invalidateConnection(Connection *c) 
   {
      mConnectionsValidity[c]->mIsValid = false;
   }
};

class Response {
  std::shared_ptr<SharedValidity> validity;

public:
   bool isValid() const { return validity->mIsValid; }

   ~Response() { validity->mIsValid = false; } 
};

[EDIT]
So client could not invalidate Response while it exists (opened) but server could mark it as invalid.

asergunov · 2018-05-29T12:58:05Z

I don't like (don't understand) the reason to have shared pointers there. When you share something you have to make sure it's thread safe. But that's really bad pattern.

I personally prefer RAII pattern. So you have object wrapping connection. So you can transfer it but can't copy.

preProcess and postProcess functions are just notifications there so they should take references, not shared pointers.

To be able to process it in threads it should be something like


class Controller {
  virtual void handle(Request&&, Response&&) = 0; ///< consumes request and response to be able to process in separate thread.
};
...
class SameThreadController 
{
  void handle(Request&&, Response&& response) override {
    response->send("hello");
  }
};

class SeparateThreadController 
{
  void handle(Request&& request, Response&& response) override {
    std::thread([](Request&&, Response&& response) {
      response->send("hello");
     }, move(request), move(response)).detach();
  }
};

saidinesh5 requested a review from rferrazz May 25, 2018 11:53

Move all weak_ptr -> const shared_ptr&

91b3e15

rferrazz reviewed May 28, 2018

View reviewed changes

asergunov reviewed May 28, 2018

View reviewed changes

saidinesh5 added 2 commits May 28, 2018 16:08

Made AbstractRequestPreprocessor into AbstractRequestCoprocessor

a1d9149

Request::isValid is not an std::atomic_bool

ac555a8

configure cmake to use c++11

e90ac6c

		@@ -1,16 +1,24 @@
		The MIT License

		Copyright (c) 2007-2019 Dinesh Manajipet <dinesh.manajipet@proemion.com>

Massive cleanup #4

Are you sure you want to change the base?

Massive cleanup #4

Uh oh!

Conversation

saidinesh5 commented May 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asergunov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saidinesh5 commented May 28, 2018

Uh oh!

asergunov commented May 28, 2018

Uh oh!

saidinesh5 commented May 28, 2018

Uh oh!

asergunov commented May 29, 2018

Uh oh!

saidinesh5 commented May 29, 2018

Uh oh!

asergunov commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asergunov commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

asergunov commented May 29, 2018 •

edited

Loading

asergunov commented May 29, 2018 •

edited

Loading