-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding an Async HttpClient module #4573
base: master
Are you sure you want to change the base?
Conversation
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. |
The requests of this feature cames from a way to use ChatGPT API in NPCs, which looks very promising.
|
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:
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. |
Thank you so much for this great feedback, Lucas! 1 - Yeah, this is a very good catch! We could simplify it as much, by making one instance of 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. |
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. |
To use lua-http is an option, but the only downside is to not be possible to use it in C++ |
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. 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? |
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. |
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). |
I'm actually in favor of having this added once there are a few issues adressed which where already mentioned. Security issues: lua: |
} | ||
|
||
// free resources | ||
luaL_unref(luaState, LUA_REGISTRYINDEX, callbackId); | ||
if (callbackId > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early return here
requestSignal.wait(requestLockUnique); | ||
} | ||
|
||
if (!pendingRequests.empty() || !pendingResponses.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Early return unlock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn`t get on how the early return would be placed here. Could you please add an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
lua_pushnil(L); | ||
while (lua_next(L, 4) != 0) { | ||
if (lua_isstring(L, -2) && lua_isstring(L, -1)) { | ||
std::string key = getString(L, -2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use references/rvalues
Hey @EvilHero90 thanks for your comment.
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.
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?
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? |
@4mrcn4 thanks for your review. Going to address it as soon as possible |
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. |
Hi, I tried to compile your repo in vs release mode x64 with Vcpkg manifest and I got this error. 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
|
Can you share the vcpkg manifest file? |
I added 'openssl' to vcpkg.json and it worked. "boost-beast",
"openssl", Thanks |
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. |
Pull Request Prelude
Changes Proposed
This PR adds a Http client module.
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:
How to use in C++:
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:
Issues addressed:
There is any issues addressed, some folks requested it in otland and I decided to contribute with that.