Adding an Async HttpClient module #4573

danilopucci · 2023-11-25T19:32:05Z

Pull Request Prelude

I have followed proper The Forgotten Server code styling.
I have read and understood the contribution guidelines before making this PR.
I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

This PR adds a Http client module.

It has support to make Http resquests in lua and in C++
Async requests
easy usage syntax (inspired on Requests from Python)

There is example on how to use it via talkaction: /httpclient <request_name>
/httpclient get
/httpclient put
More details, check in data/talkactions/scripts/httpclient.lua

How to use in lua:

local function requestCallback(response)
 --[[response is a table with current fields:
    "requestId"
    "version"
    "statusCode"
    "location"
    "contentType"
    "responseTimeMs"
    "headerData"
    "bodySize"
    "bodyData"
    "success"
    "errorMessage"
    
    where you can use "contentType" to get if it is a json and parse "bodyData" accordinly
 ]]
end


-- dummy exemple to get something
local httpClientRequest = HttpClientRequest()
local url = "https://myapi.mydomain.com"
httpClientRequest:get(url, requestCallback)


-- dummy example to post something

local url = "https://myapi.mydomain.com"
local token = "abcdef123456789"
local data = "I want to post something"
local fields = {
    accept = "application/json", 
    authorization = "Bearer " .. token
    }
httpClientRequest:post(url, requestCallback, fields, data)

How to use in C++:

//define a callback function to parse response:
void requestCallback(const HttpClient::HttpResponse_ptr& response)
{
//checkout HttpResponse structure to know its contents
}

HttpClientLib::HttpRequest_ptr httpRequest = std::make_shared<HttpClientLib::HttpRequest>();

httpRequest->url = "https://myapi.mydomain.com";
httpRequest->data = "I want to post something";
httpRequest->fields = {
                {"Authorization", "Bearer " + token},
                {"Accept", "application/json"}
            };

httpRequest->callbackData.callbackFunction = requestCallback;

g_http.addRequest(httpRequest);

How does it works?

It is based on http client lib. There is a dedicated thread, very similar to DatabaseTasks which will handle requests both comming from C++ or Lua context.

Files added:

httpclient.cpp : here you can find the thread implementation, which is similar to DatabaseTasks
httpclient.h: header to httpclient
httpclientlib.h: here you can find the lib.
- In short it is a header only library, implemented over Boost.beast lib

Issues addressed:
There is any issues addressed, some folks requested it in otland and I decided to contribute with that.

ranisalt · 2023-11-27T02:16:33Z

I don't see the utility in this at all. Why would you want the server to make a request? It would help if you pointed out where this feature was requested so we can evaluate it.

danilopucci · 2023-11-28T14:47:54Z

The requests of this feature cames from a way to use ChatGPT API in NPCs, which looks very promising.
But the sense of have a server making requests is that this repository is a server in point of view of the game-server and game-client. But the game-server may want to interact with external services, and this external services mostly uses HTTP APIs.
Here is some examples that people may use, which I know that is very common in games in general:

Post real time data to monitoring services
Make use of moderating services on game chats
Rapid language translations on game chats
Fetch geolocalization of players by its IP
Post real time data into server's owner discord
Fetch real time configurations to the game-server
User login 2FA

lgrossi · 2023-11-29T23:29:42Z

The requests of this feature cames from a way to use ChatGPT API in NPCs, which looks very promising. But the sense of have a server making requests is that this repository is a server in point of view of the game-server and game-client. But the game-server may want to interact with external services, and this external services mostly uses HTTP APIs. Here is some examples that people may use, which I know that is very common in games in general:

Post real time data to monitoring services

Make use of moderating services on game chats

Rapid language translations on game chats

Fetch geolocalization of players by its IP

Post real time data into server's owner discord

Fetch real time configurations to the game-server

User login 2FA

This is absolutely valid, indeed. There are multiple reasons why one might want to perform HTTP calls from a game server. That doesn't mean that you should necessarily support them in a generic way, indiscriminately. The main problem for me here is the overall design. There are quite a feel things in your design that needs to be taken into consideration:

One for all: you're trying to cover all use cases of a HTTP communication in a single generic option. Thus, everything is sharing the same thread. If you have one of your target APIs down, partially unavailable or slowed everything else will get impacted. I'd very much favor separation of concerns/intentions here. You should build those as the need comes, and not prepare your server for all use cases, which take us to point two.
Too many use cases can be a problem: Even though the scenarios you mentioned are valid and valuable, it makes no sense to have a single application trying to encapsulate all those responsibilities. While for a few use cases might be viable and even desirable that they live within the server, there are physical limitations to what you can serve/do from a single place. Even if you spread them in dedicated threads, your threading strategy will go as far as your CPU support (e.g. as an attempt to mitigate point 1), anything above that will be overhead.
Exposing too much to Lua and Talk Actions: There are already quite a few issues associated to expose a full functional HTTP capability to your scripting component by itself. We could discuss security issues, coupling issues, etc. I won't even go that further. However, why in earth would you have a generic talk-action without any validation or group check, where any player could basically create chaos in your server? 🤔

I don't really care about what gets merged to TFS, but I'm really concerned with the consequences with such poorly thought HTTP functionality to the general OT community, specially considering that TFS is still somewhat relevant. I'd advise rethinking a bit the extension of this functionality and start smaller, with one or two use cases in mind first and gradually iterate over it.

danilopucci · 2023-11-30T00:46:34Z

Thank you so much for this great feedback, Lucas!
It was worth it just to read the sentence "start smaller, with one or two use cases in mind first and gradually iterate over it". Completely agreed!

1 - Yeah, this is a very good catch! We could simplify it as much, by making one instance of HttpClientLib::Request for each request. This way, if the API went down, the timeout would be executed and it would endup the request, without blocking any other. My only concern about this is the overhead to create an executor for each request... but it is just a matter of trade-off.

3 - Oh, I completely forgot to mention. You are right, there is no reason to create a talkaction for that, it was just for a matter of examples that I left there 😄. The idea would be to remove it before merge, of course! Even because the requests that I wrote there are pretty much useless.
I would like to hear more about the security and coupling issues.

ranisalt · 2023-11-30T00:56:23Z

Is there anything you can't do with lua-http? I understand the appeal, but don't feel like reinventing the wheel for something so fundamental as HTTP. lua-http has the added benefit of supporting websockets which seem more related to the features you pointed out than HTTP itself.

danilopucci · 2023-11-30T01:05:23Z

Is there anything you can't do with lua-http? I understand the appeal, but don't feel like reinventing the wheel for something so fundamental as HTTP. lua-http has the added benefit of supporting websockets which seem more related to the features you pointed out than HTTP itself.

To use lua-http is an option, but the only downside is to not be possible to use it in C++

src/httpclient.cpp

+	// Print the string to the console
+	// std::cout << headerStr << std::endl;
+	// std::cout << bodyStr << std::endl;


MillhioreBT · 2023-11-30T05:32:31Z

Is there anything you can't do with lua-http? I understand the appeal, but don't feel like reinventing the wheel for something so fundamental as HTTP. lua-http has the added benefit of supporting websockets which seem more related to the features you pointed out than HTTP itself.

I also agree with you, I am not aware of whether there is a TOP and modern bookstore that is kept updated, but I was doing a quick search and it caught my attention CPR

It seems active and maintained by a community, I don't like the idea of reinventing the wheel if there are already libraries like this that cover everything related to http/s

danilopucci · 2023-11-30T22:51:32Z

Is there anything you can't do with lua-http? I understand the appeal, but don't feel like reinventing the wheel for something so fundamental as HTTP. lua-http has the added benefit of supporting websockets which seem more related to the features you pointed out than HTTP itself.

I also agree with you, I am not aware of whether there is a TOP and modern bookstore that is kept updated, but I was doing a quick search and it caught my attention CPR

It seems active and maintained by a community, I don't like the idea of reinventing the wheel if there are already libraries like this that cover everything related to http/s

Yeah, CPR is a really good one! I had the opportunity to use it somewhere. When I started my research before put the hands on code my first though was on CPR. But in my opinion, this library has much more than we need here. The one that I used is very well know too, and it is maintaned by boost: boost.beast.
While CPR is wrapping curl, beast is built around boost.

But in anyway, I think we had very valuable discussions here. I am open to adapt my work to fit all the requirements. What do you guys think, can we list what is good, bad, need improvements etc and endup in a design specification to work on?

lgrossi · 2023-12-03T08:57:33Z

Is there anything you can't do with lua-http? I understand the appeal, but don't feel like reinventing the wheel for something so fundamental as HTTP. lua-http has the added benefit of supporting websockets which seem more related to the features you pointed out than HTTP itself.

I think that the boost http library he is using under the hood is as solid as lua http if not more. However, I'd challenge the need or even the desire of doing http call from lua here.

I really think that in a server + lua script customisation setup, lua shouldn't have direct access to http exposed function. We have a long history of giving lua too much power and mixing concerns.

Imho, anything that communicates with external services directly (db calls, web hooks, http calls, tcp calls) shouldn't be exposed indiscriminately to lua, but that's a deeper discussion.

danilopucci · 2023-12-13T01:49:39Z

For whoever may be interested in: I could investigate the great comment that @lgrossi did about "One for all".

I could make some experiments and now it is pretty clear that if any ongoing HTTP request delay or some other problem, neither the main thread and new requests will be impacted. This happens because I used the async calls, so behind the scenes the boost.beast library handles all the requests until it timeout (in a worst case scenario).

EvilHero90 · 2024-01-12T03:45:08Z

I'm actually in favor of having this added once there are a few issues adressed which where already mentioned.

Security issues:
The server should never be exposed in any way to public, as already mentioned people could just flood the server with http requests, this might not be a problem at first sight since we are not blocking the main thread, however it still consumes cpu and puts stress on the hosting machine.
I'd rather have a middle man API which can be either a localhost system and also acting as a Firewall or a strict IP bind to a specific outside API, that way we can assure that nothing bad can happen, just adding one more level of security.

lua:
This was also already mentioned, exposing to much power to lua in this case can turn into worst nightmares for people if they have a malicious setup on one of their requests.
There should be an extra lua env which is restricted in what it can do ex:
_G or db could be fully disabled there, imagine you would stream a script directly from a github fork and the owner suddenly changes the code to a malicious one, this could easily destroy an entire server without any hestiation if there isn't some kind of "verification" system or a strict denial for critical functions exposed to lua.

marcingoralski · 2024-01-23T10:14:35Z

src/httpclient.cpp

 	}

-	// free resources
-	luaL_unref(luaState, LUA_REGISTRYINDEX, callbackId);
+	if (callbackId > 0) {


Early return here

marcingoralski · 2024-01-23T10:15:25Z

src/httpclient.cpp

+			requestSignal.wait(requestLockUnique);
+		}
+
+		if (!pendingRequests.empty() || !pendingResponses.empty()) {


Early return unlock

I didn`t get on how the early return would be placed here. Could you please add an example?

Instead of checking for falseness in L#28, check for pendingRequests.empty() && pendingRequests.empty() and return requestLockUnique.unlock();, which otherwise is unlocked in L#54.

A small thing that will reduce nesting, keep up the good work 👍

marcingoralski · 2024-01-23T10:25:52Z

src/luascript.cpp

+		lua_pushnil(L);
+		while (lua_next(L, 4) != 0) {
+			if (lua_isstring(L, -2) && lua_isstring(L, -1)) {
+				std::string key = getString(L, -2);


Use references/rvalues

danilopucci · 2024-01-24T02:14:52Z

Hey @EvilHero90 thanks for your comment.

Security issues: The server should never be exposed in any way to public, as already mentioned people could just flood the server with http requests.

I didn't get this point. You mean exposing it on TalkActions? If so, the code on TalkActions is an example only, just to be an easy way to the server owner test.

I'd rather have a middle man API which can be either a localhost system and also acting as a Firewall or a strict IP bind to a specific outside API, that way we can assure that nothing bad can happen, just adding one more level of security.

In this case, lets say that I am using the HttpClient to send messages to a discord server. Your proposal is to have a localhost service that the TFS server will bind to, and then send the request to this localhost service which will perform the request?

lua: This was also already mentioned, exposing to much power to lua in this case can turn into worst nightmares for people if they have a malicious setup on one of their requests. There should be an extra lua env which is restricted in what it can do ex: _G or db could be fully disabled there, imagine you would stream a script directly from a github fork and the owner suddenly changes the code to a malicious one, this could easily destroy an entire server without any hestiation if there isn't some kind of "verification" system or a strict denial for critical functions exposed to lua.

Don't get me wrong, maybe I just didn't understood your proposals, but by reading your message it looks that you are confusing the HttpClient with a HttpServer. Is it the case?

danilopucci · 2024-01-24T02:15:41Z

@4mrcn4 thanks for your review. Going to address it as soon as possible

gaamelu · 2024-04-17T12:58:15Z

Great work adding a http client to tfs! For sure it'll be useful. Any news on the pr?

danilopucci · 2024-04-19T12:44:13Z

Great work adding a http client to tfs! For sure it'll be useful. Any news on the pr?

Hey @gaamelu, I don't think the PR is going to merged, but it is fully working and tested. I know about 3 or 4 servers with TFS that are using it for a while with any problems.

Just keep in mind that the talkaction code is just for testing purposes. The talkaction has protection for Gamemasters, but in any way it would not be useful to do not add it to your server.

By the way, If someone is interested, I have added it to canary too.

ericcobblepot · 2024-04-20T06:04:15Z

Hi, I tried to compile your repo in vs release mode x64 with Vcpkg manifest and I got this error.
i am bad at cpp

1>D:\desktop\test\vcpkg_installed\x64-windows\x64-windows\include\boost\asio\ssl\detail\openssl_types.hpp(23,10): fatal  error C1083: Cannot open include file: 'openssl/conf.h': No such file or directory
1>Done building project "theforgottenserver.vcxproj" -- FAILED.

Severity	Code	Description	Project	File	Line	Suppression State
Error (active)	E0282	the global scope has no "ERR_get_error"	theforgottenserver	D:\desktop\test\src\httpclientlib.h	486	
Error (active)	E0020	identifier "SSL_set_tlsext_host_name" is undefined	theforgottenserver	D:\desktop\test\src\httpclientlib.h	485

danilopucci · 2024-04-20T15:42:52Z

Hi, I tried to compile your repo in vs release mode x64 with Vcpkg manifest and I got this error. i am bad at cpp

Can you share the vcpkg manifest file?
You should add "boost-beast" to it too

ericcobblepot · 2024-04-21T00:00:04Z

Can you share the vcpkg manifest file? You should add "boost-beast" to it too

I added 'openssl' to vcpkg.json and it worked.

"boost-beast",
"openssl",

Thanks

Ochmar · 2024-05-21T16:36:22Z

As much as I do like the idea, I'm not convinced this fits TFS purposes?

I'm not here to discover possibilities, pros and cons, but I think hiding it behind feature flag is the least we shuold do.

danilopucci added 4 commits November 25, 2023 15:46

Merge remote-tracking branch 'otland/master'

7e880ee

- adding httpclient module files

0219aa7

- adding full C++ support

a790018

- adding full lua support + examples

c4a376b

danilopucci changed the title ~~Feature/httpclient~~ Adding an Async HttpClient module Nov 25, 2023

EPuncker requested review from DSpeichert, ranisalt, nekiro and MillhioreBT November 25, 2023 21:00

EPuncker added the feature New feature or functionality label Nov 25, 2023

danilopucci added 3 commits November 28, 2023 15:55

- fix clang-format

e03623d

- fix clang-format (game.cpp)

2cae0cf

- fix clang-format (other missed files)

6cde0d0

danilopucci added 4 commits November 29, 2023 22:40

- fixing compile errors

ea259f4

- fix more formatting

b7fb10f

Merge branch 'fix/httpclient-compile-errors' into feature/httpclient

8df0a9b

- update vcpkg dependency

8f52d51

github-advanced-security bot found potential problems Nov 30, 2023

View reviewed changes

src/httpclient.cpp

Comment on lines +124 to +126

// Print the string to the console

// std::cout << headerStr << std::endl;

// std::cout << bodyStr << std::endl;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.

danilopucci added 2 commits January 10, 2024 18:31

- bugfix in linux

b54ff1e

bugfix in httplib

294e9aa

- adding missed pop on lua stack

b5a671b

danilopucci added 2 commits January 21, 2024 14:48

- refactor lua callback. Set it in the same thread of all lua calls

7251d25

- missed dispatcher on last commit

63f1010

marcingoralski reviewed Jan 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding an Async HttpClient module #4573

Adding an Async HttpClient module #4573

danilopucci commented Nov 25, 2023

ranisalt commented Nov 27, 2023

danilopucci commented Nov 28, 2023

lgrossi commented Nov 29, 2023 •

edited

danilopucci commented Nov 30, 2023

ranisalt commented Nov 30, 2023

danilopucci commented Nov 30, 2023

MillhioreBT commented Nov 30, 2023

danilopucci commented Nov 30, 2023

lgrossi commented Dec 3, 2023

danilopucci commented Dec 13, 2023

EvilHero90 commented Jan 12, 2024

marcingoralski Jan 23, 2024

marcingoralski Jan 23, 2024

danilopucci Jan 24, 2024

marcingoralski Jan 25, 2024

marcingoralski Jan 23, 2024

danilopucci commented Jan 24, 2024

danilopucci commented Jan 24, 2024

gaamelu commented Apr 17, 2024

danilopucci commented Apr 19, 2024 •

edited

ericcobblepot commented Apr 20, 2024 •

edited

danilopucci commented Apr 20, 2024

ericcobblepot commented Apr 21, 2024 •

edited

Ochmar commented May 21, 2024

Adding an Async HttpClient module #4573

Are you sure you want to change the base?

Adding an Async HttpClient module #4573

Conversation

danilopucci commented Nov 25, 2023

Pull Request Prelude

Changes Proposed

How to use in lua:

How to use in C++:

How does it works?

ranisalt commented Nov 27, 2023

danilopucci commented Nov 28, 2023

lgrossi commented Nov 29, 2023 • edited

danilopucci commented Nov 30, 2023

ranisalt commented Nov 30, 2023

danilopucci commented Nov 30, 2023

MillhioreBT commented Nov 30, 2023

danilopucci commented Nov 30, 2023

lgrossi commented Dec 3, 2023

danilopucci commented Dec 13, 2023

EvilHero90 commented Jan 12, 2024

marcingoralski Jan 23, 2024

Choose a reason for hiding this comment

marcingoralski Jan 23, 2024

Choose a reason for hiding this comment

danilopucci Jan 24, 2024

Choose a reason for hiding this comment

marcingoralski Jan 25, 2024

Choose a reason for hiding this comment

marcingoralski Jan 23, 2024

Choose a reason for hiding this comment

danilopucci commented Jan 24, 2024

danilopucci commented Jan 24, 2024

gaamelu commented Apr 17, 2024

danilopucci commented Apr 19, 2024 • edited

ericcobblepot commented Apr 20, 2024 • edited

danilopucci commented Apr 20, 2024

ericcobblepot commented Apr 21, 2024 • edited

Ochmar commented May 21, 2024

lgrossi commented Nov 29, 2023 •

edited

danilopucci commented Apr 19, 2024 •

edited

ericcobblepot commented Apr 20, 2024 •

edited

ericcobblepot commented Apr 21, 2024 •

edited