f2e-spec [WIP] Port webserver from cpprestsdk

What does this PR do?

Migrates our webserver to Nlohmann JSON and either Restinio or Restbed (not fully decided yet, appreciate input from @performous/core-owners); both have pros and cons re: dependencies.

Restinio depends on Boost.Asio, http_parser and libfmt (other stuff too but those are what we'd need): On one hand we already depend on Boost.Asio so it's not a big deal. http_parser we could either use it externally or have it build and link statically from the restinio tree. libfmt more-or-less the same.
Restbed depends on standalone Asio (and an old version at that): That would IMO a) be a potential security risk (although probably not too important considering our context) and I do expect they'll update it at some point and/or support Boost.Asio. Chief advantage is I think Restbed's developers might be saner and using Restbed would probably require less patching of the source-code and build-scripts on our part.

Closes Issue(s)

#452

Motivation

CppRestSDK is full of Microsoft ew.

More

[ ] Added/updated documentation

Additional Notes

May 01 '20 21:05 Lord-Kamina

RESTinio definitely looks like the more sane option here, although the webserver will still come with heavy dependencies so it should still be possible to disable the feature at build time. Restbed's API is ridden with shared_ptrs, which is not a very good sign, and seems more cumbersome from their examples, compared to RESTinio which actually has a modern feel to it and uses concepts similar to NodeJS Express.

May 02 '20 13:05 Tronic

RESTinio definitely looks like the more sane option here, although the webserver will still come with heavy dependencies so it should still be possible to disable the feature at build time. Restbed's API is ridden with shared_ptrs, which is not a very good sign, and seems more cumbersome from their examples, compared to RESTinio which actually has a modern feel to it and uses concepts similar to NodeJS Express.

Generally, my impression towards RESTinio has always been that it's a very good library but its developers make weird decisions hahaha

May 02 '20 14:05 Lord-Kamina

Now is when the real work begins. This has a ways to go still but in the meantime... @performous/core-owners (especially, @Tronic); is what I did to the Restinio CMakeLists acceptable? Essentially the idea is if you have the restinio dependencies installed in your system then cool you can use them but else they come bundled and don't require any extra interaction.

May 04 '20 05:05 Lord-Kamina

I'm too busy to look into that but generally I dislike bundling things, especially anything that requires a build (i.e. is not header-only). I believe that the packagers of distributions such as Debian are obligated to revert these and package each dependency separately in any case.

May 04 '20 12:05 Tronic

I'm too busy to look into that but generally I dislike bundling things, especially anything that requires a build (i.e. is not header-only). I believe that the packagers of distributions such as Debian are obligated to revert these and package each dependency separately in any case.

Fmt is header only and http-parser is a single header and a single c file.

For me, a problem is that somr of the most popular distros use packages that haven't been updated in years.

May 04 '20 14:05 Lord-Kamina

Just getting into this PR but as for now i'd suggest to split nlohmann-json and restinio in seperate PR's. As you've already done in the code. This will help for an easier merge 👍

May 11 '20 09:05 Baklap4

Indentation is still a mess but I'd rather fix that in another PR so as to not make this huge diff even harder to read.

May 28 '20 04:05 Lord-Kamina

Looking good so far, however sorting the library (on the frontend) doesn't work correctly. The list doesn't get sorted as if it doesn't take query params atm

Jun 15 '20 20:06 Baklap4

Looking good so far, however sorting the library (on the frontend) doesn't work correctly. The list doesn't get sorted as if it doesn't take query params atm

Ah, I missed that. I'm pretty sure I implemented the code for sorting but I must have forgotten to catch the actual press or something.

Jun 15 '20 20:06 Lord-Kamina

Looking good so far, however sorting the library (on the frontend) doesn't work correctly. The list doesn't get sorted as if it doesn't take query params atm

Should be fixed now.

The idea is that now both sorting and filtering are completely independent between UI and webserver; else, it was very easy for someone to troll you from the webserver.

Jun 17 '20 04:06 Lord-Kamina

Hmmm while loading my song library i notice a lot of '404' upon adding a song which is already found in the frontend. A little bit later this is fixed i suspect some threading issue not synching the state of m_songs correctly

Jun 17 '20 14:06 Baklap4

Hmmm while loading my song library i notice a lot of '404' upon adding a song which is already found in the frontend. A little bit later this is fixed i suspect some threading issue not synching the state of m_songs correctly

You mean when adding to playlist? I haven't seen this; on the other hand, I'm testing with a small library.

Jun 17 '20 18:06 Lord-Kamina

Yup adding to the playlist while loading around 5-10k songs

Jun 17 '20 19:06 Baklap4

Yup adding to the playlist while loading around 5-10k songs

I tried again with my full library (which is still only a fraction of yours); 2650 songs and saw no 404s.

Jun 17 '20 19:06 Lord-Kamina

Hmm i'll think i'll have to retry this to see if it's just my system

Sep 15 '20 18:09 Baklap4

It would probably make sense to split this PR in several smaller PRs:

Various improvements and refactorings
Introducing code and configurations that will be required by the new webserver implementation, without breaking the existing stuff
The new webserver implementation. In principle, point 3. should be limited to changes to webserver.cc and requesthandler.cc

Restino states that they are a C++ 14 lib. Any issue expected with c++17 ?
Restino appears to offer a choice of asio implementation. Why choose the boost one ?

Jul 17 '21 18:07 deuteragenie

It would probably make sense to split this PR in several smaller PRs:

Various improvements and refactorings

Introducing code and configurations that will be required by the new webserver implementation, without breaking the existing stuff

The new webserver implementation. In principle, point 3. should be limited to changes to webserver.cc and requesthandler.cc

Many of the smaller things already existed as unmerged PRs; I'm aiming to either get them merged or scrapped and adjust accordingly. I'll also probably try to tidy the commit history in this PR up a bit before actually merging it.

Restino states that they are a C++ 14 lib. Any issue expected with c++17 ?

I don't think so, and per https://stackoverflow.com/questions/46746878/is-it-safe-to-link-c17-c14-and-c11-objects this shouldn't really be a concern in most cases.

Restino appears to offer a choice of asio implementation. Why choose the boost one ?

I think the main reason was chiefly because standalone asio was not as well maintained as boost in package managers.

Jul 27 '21 18:07 Lord-Kamina

Alright tested it for a bit:

Build failures on Ubuntu 21.04 with boost 1.74.0. To fix it add: add_definitions(-DBOOST_ASIO_USE_TS_EXECUTOR_AS_DEFAULT) somehwere in cmakelist file. Hopefully you'll come up with a nicer version of that :)

The webserver config menu is out of sight because it became a new menu item. Maybe rescale these menu items?

Meaning you have to scroll to see it? I'm sure that can be fixed. That will change eventually in my PR to fix text rendering but that one's not nearly close to finished so maybe I can do something else in the meantime.

On exit i'm getting a segfault

Can you give me a trace? I specifically tested this quite a bit because this used to be a bug in CppRest also. Maybe something got fucked up in a rebase?

When running on port 80 and starting performous i'm getting Permission denied. Here should the retry-mechanism kick in. It retries but is not increasing the 'tried' times so it gets in a loop of continuous retrying instead of failing after 3 times on normal port than falling back on fallback port.

I'll try this out. I've never used port 80 so maybe I missed it.

sometimes i'm getting performous in 'weird colors':

That's really, really weird. And... I don't see how that could be caused by the PR? 🥴

~~I can't search for language~~ I thought this was a bug, but then after digging i have all my songs in a folder per language. The game detects filters also on 'path' so a language can always be found. Would be nice to search for language though -> song.cc#206 - strFull()

Sure.

I have only tested this with 4 songs as my library. Once the items from here are picked up i'll test with my big collection

Aug 04 '21 18:08 Lord-Kamina

Meaning you have to scroll to see it? I'm sure that can be fixed. That will change eventually in my PR to fix text rendering but that one's not nearly close to finished so maybe I can do something else in the meantime.

Yup, i think for now switching it with for example path would suffice. The actual fix comes in the text rendering thingy

Can you give me a trace? I specifically tested this quite a bit because this used to be a bug in CppRest also. Maybe something got fucked up in a rebase?

After a one time (without gdb) it didn't happen to me anymore. Maybe other ppl should test this too

I'll try this out. I've never used port 80 so maybe I missed it.

The thing with port 80 is you'll get an exception for a permission error (which is correct) but was the only thing i could think of to 'simulate' a port being used (= also permission error)

That's really, really weird. And... I don't see how that could be caused by the PR?

For now let it be, if i keep getting it i guess i'll file a bug. Maybe my gpu is toasted 🤷

Aug 04 '21 19:08 Baklap4

The thing with port 80 is you'll get an exception for a permission error (which is correct) but was the only thing i could think of to 'simulate' a port being used (= also permission error)

I see. I'll try it when I get home.

Aug 04 '21 19:08 Lord-Kamina

Download the artifacts for this pull request:

This service is provided by nightly.link. These artifacts will expire in 90 days and will not be available for download after that time.

Oct 07 '22 19:10 github-actions[bot]

Looks like this PR is dragging badly, but to get back to the initial motivation:

CppRestSDK is full of Microsoft ew.

Do you mean it has some actual flaws (like WINAPI crap), or just that it is made by Microsoft? If it is open source, that shouldn't matter.

Oct 08 '22 10:10 Tronic

Looks like this PR is dragging badly, but to get back to the initial motivation:

CppRestSDK is full of Microsoft ew.

Do you mean it has some actual flaws (like WINAPI crap), or just that it is made by Microsoft? If it is open source, that shouldn't matter.

I didn't like that it's bloated, not very performant (although @yoda-jm made a very valid point in his own PR that we don't need that much performance) and chiefly that it didn't build on mingw-w64, which is how we were making builds at the time. Some of those issues might be solved eventually. I am almost done getting this in a state where it can be reviewed again. Even if we end-up not merging it, it has valuable stuff like separating the search in the web UI and the actual UI.

Oct 08 '22 11:10 Lord-Kamina

f2e-spec f2e-spec copied to clipboard

[WIP] Port webserver from cpprestsdk

What does this PR do?

Closes Issue(s)

Motivation

More

Additional Notes

f2e-spec
f2e-spec copied to clipboard