Update Mongoose to newest release version (6.18) #388

whisperity · 2020-05-01T19:00:46Z

The Mongoose library major version we were using (5.4) is from 2014 and is no longer actively developed. There is a newer version of the 6.y line, however, moving to this version required a significant rewrite of the threaded response handling.

Originally my first hunch of implementing SSL for #387 was to update Mongoose and try, I only afterwards found out that 5.4 also supports SSL.

The previous behaviour was that we were running on each thread a Mongoose server that was listening on the same (manually injected) socket. This behaviour is not supported any more, the new Mongoose version threw away "real" multithreading support, and the API no longer allows injecting the socket.
(In addition, I was told that the same process on n threads listening on the same socket resulted in the kernel waking up every thread whenever a request was received.)

To align the API with the new version, a different approach is used: incoming HTTP requests, if are of trivial nature (static file access, which will also help for this patch to be joined together with #386!) are served as if Mongoose was serving a static directory - there were also massive API changes here.
Otherwise, the request will be saved into a thread pool's queue, similar to the one used by the parser engine, and handled by dispatching to the Thrift endpoints.

The indicators MG_FALSE and MG_TRUE are also gone, the new API requires for the user of the library to manually handle doing A or B when an URL is accessed. Previously Mongoose got to know that the callback didn't do anything with the request and fell back by itself to serving the static resource.

The upside of this is that there is a dedicated thread for listening on the socket, and as such, heavy-handed operations will not lock static page accesses from being served. Of course, the number of threads for the service workers is an upper limit to how much Thrift requests can be served at a time.

Most of the changes in this patch are of a highly technical nature:

Need to bind the listener with new API.
The request handler callback (from Mongoose's point of view) does not directly handle the request, and as such, returning from it would destroy the request buffer, so we have to side-step this issue by saving the important data out.
Performing the same decoupling on the handlers' API.

whisperity

Adding a few comments on stuff that look weird if you just check the diff.

whisperity · 2020-05-01T19:05:18Z

plugins/cpp/test/CMakeLists.txt

@@ -4,6 +4,7 @@ include_directories(
  ${PROJECT_BINARY_DIR}/service/language/gen-cpp
  ${PROJECT_BINARY_DIR}/service/project/gen-cpp
  ${PROJECT_SOURCE_DIR}/model/include
+  ${PROJECT_SOURCE_DIR}/webserver/include


Changes in other headers broke the test's file.
The test's header includes the service header, and as such, this subproject also needs to know how to resolve the #include <webserver/servercontext.h> in cppservice.h.

whisperity · 2020-05-01T19:06:14Z

util/include/util/threadpool.h

@@ -4,6 +4,7 @@
 #include <atomic>
 #include <chrono>
 #include <condition_variable>
+#include <functional>


The default for a template parameter is std::function. This include was needed ever since #120, but the files this header was included to already made <functional> available.

whisperity · 2020-05-01T19:10:13Z

webserver/src/webserver.cpp

+// Ensure that Mongoose API is defined the same way in all translation units!
+#include <webserver/mongoose.h> // IWYU pragma: keep


This was weird to debug. Basically, if C++ headers are included before mongoose, there'll be some weird issues and the static_assert in mongooseserver.h will fire... Which shouldn't because the typedef and all that remains the exact same thing. Digging through a Clang AST I was able to find out that somewhere deep __falign_t becomes defined in a different way.

Without the static assert, the code compiles, and I did not find any issues run-time, but it's better to be ODR-safe.

whisperity · 2020-05-01T19:10:40Z

webserver/src/webserver.cpp

 #include "mainrequesthandler.h"
+#include "mongooseserver.h"


If these include were in a different order (keeping the alphabetic style), webserver.cpp would not compile.

whisperity · 2020-05-01T19:11:34Z

webserver/src/mongooseserver.h

+  static void
+  nativeRequestHandler(struct mg_connection* conn_, int ev_, void* data_);
+  static_assert(
+    std::is_same<decltype(&nativeRequestHandler), ::mg_event_handler_t>::value,
+    "Mongoose-interfacing handler function should match signature!");


This is the static assert I (will be|was) talking about. It basically just says: make sure the interface of the function pointer you are passing actually matches the receiver's side.

webserver/src/webserver.cpp

whisperity · 2020-05-11T10:48:12Z

Rebased over #386.

There are no licensing changes incurred, the license of Mongoose did not change between versions. (Skip-CI because this commit ONLY contains the Mongoose changes, which right now break the remaining server code.)

Mongoose 6.x line broke the API significantly, and multi-threading is no longer officially supported, so we had to come up with a new multithreading logic. Naive HTTP requests for static files is served trivially by Mongoose itself, and requests to the Thrift API will use a worker thread pool instead. This way the listener socket is not blocked until a reply is created, and other users can still send requests.

bruntib · 2020-07-24T19:57:18Z

webserver/include/webserver/httprequest.h

+  // '[' + sizeof(ipv6-address) + ']' + strlen(":12345")
+  char address[1 + INET6_ADDRSTRLEN + 1 + 6];


Don't we need one more character for \0? Same in "else" branch.

Both INET_ADDRSTRLEN and INET6_ADDRSTRLEN already calculate the null terminator into their length. For example, the IPv4 version is defined to be 16, which means 255.255.255.255 (4 * 3 + 3 = 15 real characters) fits perfectly into a buffer of char[INET_ADDRSTRLEN].

bruntib

With this patch the page load and every network request take a few seconds. Everything became slow, so this needs some debugging. I'll try to investigate too. Apart from this, the PR seems to be fine.

mcserep · 2020-07-25T14:50:39Z

With this patch the page load and every network request take a few seconds. Everything became slow, so this needs some debugging. I'll try to investigate too. Apart from this, the PR seems to be fine.

I can verify @bruntib 's observation, with this PR, a HTTP request towards e.g. ProjectService takes more than 1 sec. Previously it was around 4-7 ms, so it got slower about 150-250 times, which cannot be a normal functionality. Tested on localhost, in Release build.

whisperity · 2020-07-25T19:33:18Z

Huh, weird, I haven't measured a difference when I made this. Well, one reason could be, is that the handling of the request is a blocking operation because Mongoose no longer allows on the API to share the listening socket between individual threads...

I am, unfortunately, out of ideas, then. Apart from sticking to the old Mongoose ad infinitum.

mcserep · 2020-07-25T19:47:41Z

@whisperity I don't think it is a threading (blocking) issue, since I have measured it on a single HTTP request, making sure that there were no other requests. (Also if Moongose's performance would be this poor under normal usage, it would render it unusable, so my guess is that we are doing something not correctly.)

It would be nice to update Moongose, maybe we can execute a performance test on CodeCompass_webserver (e.g. with CLion and perf) and check where does it spends so much time when handling a request.

whisperity · 2020-07-26T10:41:12Z

I'm leaving the patch up, but I do not think I'll have the mental capacity and acuity to reverse engineer and dissect this patch in the foreseeable future.

mcserep · 2020-07-28T10:48:34Z

webserver/CMakeLists.txt

+  MG_ENABLE_MQTT=0
+
+  # Only exposes a pthread_create() API, changes no behaviour w.r.t CodeCompass.
+  MG_ENABLE_THREADS=0


Where are all these flags properly documented? I have found this:
https://cesanta.com/docs/overview/build-options.html

The following flags you use are not listed here:

MG_ENABLE_DEBUG

MG_ENABLE_ASYNC_RESOLVER

MG_ENABLE_CALLBACK_USERDATA

MG_ENABLE_DNS (probably should be MG_ENABLE_DNS_SERVER ?)

MG_ENABLE_FILESYSTEM

MG_ENABLE_HTTP_URL_REWRITES

I went purely by reading mongoose.h and mongoose.c. As the code is meant for embedded system, the header (and to some extent, the implementation code), is relatively straightforward to read.

MG_ENABLE_DEBUG simply results in a very obscure dump of steps taken and enum values for each request. It's somewhat useful for debugging if you manually trace the requests and such, but not useful for production system. (It's set to 0 because is a build-time flag, due to how Mongoose's code is structured, you can't make this dependent on --loglevel debug at runtime.)

Mongoose is not just a server, it can also be used as a client library. MG_ENABLE_DNS (defaulting to 1) toggles whether, if used as a client, the code has the ability to do DNS resolution. Because we are not using Mongoose as a client, we do not need this functionality.

(_IPV6 and _MQTT similarly disable networking stacks that we are not using.)

MG_ENABLE_ASYNC_RESOLVER is related to this, too. (If enabled, clientside DNS resolution is done asynchronously, by default via MG_DEFAULT_NAMESERVER "8.8.8.8".

MG_ENABLE_FILESYSTEM is to be used if we are serving a conventional HTTP root (i.e. like Apache Options Index) and static files. This can't be used, as we have special endpoints (for Thrift) which would conflict, if Mongoose was to serve the filesystem natively.

MG_ENABLE_HTTP_URL_REWRITES is explained pretty well in the header:

MG_ENABLE_CALLBACK_USERDATA is a flag that changes the signatures of Mongoose handlers to take an additional userdata argument, like this:

In many functions, if this is enabled, an additional void * argument is present in the functions' signatures, which allows registering/passing an arbitrary object through Mongoose.

mcserep · 2020-07-28T11:12:35Z

I'm leaving the patch up, but I do not think I'll have the mental capacity and acuity to reverse engineer and dissect this patch in the foreseeable future.

Okay, I might add some information meanwhile if I get to debug this.
Right now the addition I can make is that static files are served normally, only our services are served slowly.

mcserep · 2021-05-08T19:08:22Z

What is the status of this PR, @whisperity ? Do you think you will have the time to get back on this issue anytime in the future? It would be really nice to upgrade the obsolete Mongoose version we are using. As I have checked, version 7.x came out December 2020, and we still use version 5.4.

mcserep · 2023-02-11T01:46:42Z

I have updated this development to be sync with the current master:
https://github.com/mcserep/CodeCompass/tree/update-mongoose

If you enable in the PR settings to "Allow edits by maintainers", then I can update it on your fork, @whisperity.

More importantly, I have made progress towards the reason of the slow page load time of the Mongoose 6 refactored webserver. The key is the timeout parameter for mg_mgr_poll().

void MongooseServer::loop()
{
  // Trap termination so the server stops.
  _detail::SignalHandler trapSigint(SIGINT, MongooseServer::signalHandler);
  _detail::SignalHandler trapSigterm(SIGTERM, MongooseServer::signalHandler);

  _loopThreadId = std::this_thread::get_id();

  while (!exitCode)
    mg_mgr_poll(_mongoose.get(), 1000);
}

With the value 1000, all (non-statically served) requests take at least 1000 milliseconds. If I set the parameter to 20, then requests are served in 24-26 ms. If I set the parameter to 10, then requests are served in 14-16 ms. However, if I set the parameter to 0, then some requests are failed to be served.

This timeout parameter of Mongoose seems to work strange.

mcserep · 2023-02-11T01:50:14Z

Apart from this, the webserver does not seem to serve requests in parallel - at least for Thrift requests, where it was easily observable with the high page load time.

Is there any reason we are not using the built-in multithreaded support of Mongoose? (MG_ENABLE_THREADS and mg_start_thread)

mcserep · 2024-02-05T07:18:05Z

I close this PR, as there have been no real development on this in the past 3+ years.

This could be reopened if the discussed issues are resolved; or since Mongoose 7.x is the current major release, maybe a new PR could be made in the future, updating the webserver to that version.

whisperity added the Kind: Enhancement 🌟 label May 1, 2020

whisperity requested review from bruntib and mcserep May 1, 2020 19:00

whisperity commented May 1, 2020

View reviewed changes

webserver/src/webserver.cpp Outdated Show resolved Hide resolved

whisperity force-pushed the update-mongoose branch from 75b89f2 to 7074951 Compare May 1, 2020 19:28

whisperity force-pushed the update-mongoose branch 2 times, most recently from 9dfce01 to 97b4073 Compare May 11, 2020 10:47

whisperity mentioned this pull request May 26, 2020

Switched to odb 2.5 from 2.4, since the version 2.4 is currently broken. #385

Merged

Cesanta Software Limited and others added 2 commits July 22, 2020 11:59

Update Mongoose 5.4 to newest release version 6.18 [skip ci]

1603dea

There are no licensing changes incurred, the license of Mongoose did not change between versions. (Skip-CI because this commit ONLY contains the Mongoose changes, which right now break the remaining server code.)

whisperity force-pushed the update-mongoose branch from 97b4073 to 7e40607 Compare July 22, 2020 10:03

whisperity changed the title ~~Update Mongoose to newest release version (6.17)~~ Update Mongoose to newest release version (6.18) Jul 22, 2020

bruntib reviewed Jul 24, 2020

View reviewed changes

bruntib requested changes Jul 24, 2020

View reviewed changes

mcserep reviewed Jul 28, 2020

View reviewed changes

mcserep added this to the Future milestone Aug 3, 2020

mcserep added the Status: WIP 👷 Issue or PR under development - feel free to review, though! label Sep 24, 2020

mcserep removed this from the Future milestone Jan 23, 2022

mcserep force-pushed the master branch 2 times, most recently from bcca4c9 to 52327cd Compare January 25, 2022 09:18

mcserep added the Status: At risk 🚨 Issue or PR which got stuck and progress in work is at risk. label Oct 22, 2022

mcserep force-pushed the master branch from 986d2f3 to fd3edc1 Compare June 9, 2023 12:31

mcserep mentioned this pull request Oct 31, 2023

Making codebase macOS compatible #637

Draft

8 tasks

mcserep closed this Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Mongoose to newest release version (6.18) #388

Update Mongoose to newest release version (6.18) #388

whisperity commented May 1, 2020 •

edited

Loading

whisperity left a comment

whisperity May 1, 2020

whisperity May 1, 2020

whisperity May 1, 2020

whisperity May 1, 2020

whisperity May 1, 2020

whisperity commented May 11, 2020

bruntib Jul 24, 2020

whisperity Jul 25, 2020

bruntib left a comment

mcserep commented Jul 25, 2020 •

edited

Loading

whisperity commented Jul 25, 2020

mcserep commented Jul 25, 2020 •

edited

Loading

whisperity commented Jul 26, 2020

mcserep Jul 28, 2020

whisperity Jul 28, 2020

whisperity Jul 28, 2020

mcserep commented Jul 28, 2020

mcserep commented May 8, 2021

mcserep commented Feb 11, 2023

mcserep commented Feb 11, 2023

mcserep commented Feb 5, 2024

		// Ensure that Mongoose API is defined the same way in all translation units!
		#include <webserver/mongoose.h> // IWYU pragma: keep

		// '[' + sizeof(ipv6-address) + ']' + strlen(":12345")
		char address[1 + INET6_ADDRSTRLEN + 1 + 6];

Update Mongoose to newest release version (6.18) #388

Update Mongoose to newest release version (6.18) #388

Conversation

whisperity commented May 1, 2020 • edited Loading

whisperity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruntib left a comment

Choose a reason for hiding this comment

mcserep commented Jul 25, 2020 • edited Loading

whisperity commented Jul 25, 2020

mcserep commented Jul 25, 2020 • edited Loading

whisperity commented Jul 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcserep commented Jul 28, 2020

mcserep commented May 8, 2021

mcserep commented Feb 11, 2023

mcserep commented Feb 11, 2023

mcserep commented Feb 5, 2024

whisperity commented May 1, 2020 •

edited

Loading

mcserep commented Jul 25, 2020 •

edited

Loading

mcserep commented Jul 25, 2020 •

edited

Loading