feat: add HTTPClient implementation using curl as a module #50964

jonbonazza · 2021-07-28T05:34:04Z

This PR uses libcurl in multi mode (curl_multi) to implement the HTTPClient interface. It cleans up the implementations significantly and should reduce maintenance burden while giving us easier access to more features in the end.

fixes godotengine/godot-proposals#2903

Scope

The goal of this PR is only to create a curl-based implementation of HTTPClient that conforms to the existing interface and nothing more.

The net team has discussed several improvements to the HTTPClient interface in general that may be implemented in future PRs, but they remain outside the scope of this one.

Remaining Work

The current state of the PR is DRAFT. It still needs more testing and there are still a few remaining pieces left.

Support IPv6 DNS resolution using Godot's resolver APIs. I was unable to get this to work, but I consider it essential for this PR to be merged. IPv4 works fine though.
See if there's a way to make DNS resolution more performant by using the async apis. I'm not familiar enough with Godot's DNS APIs to know how best to use them, and the documentation for them is quite lacking. May want to just leave this up to a future improvement with its own PR.
Implement set_connection and get_connection. The way libcurl works doesn't lend itself well to the StreamPeer interface, so without some changes to that API, there's not a good way to do t his. If we do decide to use NetSocket as the socket implementation (see above item) then we get this for free, so that's one pro to that approach. That said, I don't see these functions used anywhere in the Godot codebase, so I'd question whether they are even needed to begin with.
Get libcurl building on all platforms. Currently, I only have it working on Windows. I know @Faless has gotten it working on Linux, but I'm not sure what was necessary there. This might take some effort. We should also take the time to decide what features to enable/disable to tune binary size and also add a flag for using the system libcurl if available.

If I've missed anything in either the implementation itself, or the TODO list above, please don't hesitate to bring it up so we can discuss and potentially address it.

Regarding Unit Tests

While we should certainly strive for as much unit tests coverage as possible, I didn't see any existing tests for the http client, so I've decided to leave that for an exercise for another day (and potentially another person).

I want to stress that my decision not to include unit tests is not to the discredit of the idea of unit tests in general; quite the contrary, in fact. I would like to see a day where Godot's unit test coverage nears 100% just as much as anyone. However the best way to test http code is by running it against a real HTTP server, and to my knowledge, Godot doesn't currently have the facilities to do that and I'm not comfortable convoluting this particular PR by adding those facilities.

A Note About The Commits

There's quite a lot of commits in this one and none of them are particularly clean. I'm a bit embarrassed about that, but it's mostly due to the fact I was learning so many new technologies along the way and ended up doing a lot of things wrong in a lot of different places in odd orders.

I suggest when merging this PR to squash all of the commits in order to keep things as clean as possible in master.

Calinou · 2021-07-28T11:20:04Z

Currently, we only resolve one address for a domain. Godot's resolver APIs support resolving multiple

Wasn't this implemented by #49026?

We should also take the time to decide what features to enable/disable to tune binary size and also add a flag for using the system libcurl if available.

I would only enable HTTP support (both HTTP/1.x and HTTP/2). FTP support is rarely used nowadays.

However the best way to test http code is by running it against a real HTTP server, and to my knowledge, Godot doesn't currently have the facilities to do that and I'm not comfortable convoluting this particular PR by adding those facilities.

We can add something to CI that runs a Godot project configured to make HTTP requests against a resource known for its high uptime, such as https://httpbin.org. If any of the requests fail, exit the project with a non-zero exit code (get_tree().quit(1)). If we are worried about the web server being unavailable, we can host our own local Web server in GitHub Actions just before the project starts.

Faless · 2021-07-28T14:01:05Z

Few things also mentioned in the chat:

Only fetch the body during read_response_body_chunk. Not during poll ❗

For IP resolution, see:

godot/core/io/http_client_tcp.cpp

Lines 81 to 83 in 0caaaf4

    
           // Host contains hostname and needs to be resolved to IP 
        
           resolving = IP::get_singleton()->resolve_hostname_queue_item(conn_host); 
        
           status = STATUS_RESOLVING;

godot/core/io/http_client_tcp.cpp

Lines 250 to 255 in 0caaaf4

    
           case STATUS_RESOLVING: { 
        
           	ERR_FAIL_COND_V(resolving == IP::RESOLVER_INVALID_ID, ERR_BUG); 
        
           	IP::ResolverStatus rstatus = IP::get_singleton()->get_resolve_item_status(resolving); 
        
           	switch (rstatus) { 
        
           		case IP::RESOLVER_STATUS_WAITING:

[blocking] I'm not even really sure how this is expected to be used. Is it even needed?

When you want to handle the request in a thread.

Implement set_connection and get_connection.

If we don't use our NetSocket implementation, just have them do nothing/return null and spit an error. Like JavaScript does:

godot/platform/javascript/http_client_javascript.cpp

Lines 74 to 76 in 0caaaf4

    
           void HTTPClientJavaScript::set_connection(const Ref<StreamPeer> &p_connection) { 
        
           	ERR_FAIL_MSG("Accessing an HTTPClientJavaScript's StreamPeer is not supported for the HTML5 platform."); 
        
           }

Docs... I dread this one.

Well, be happy, you won't have to write any new doc, given you are creating a new implementation of an exposed class, not a new exposed class (so HTTPClient is already fully documented).
You can of course try to improved the documentation, if you like :).

Get libcurl building on all platforms.

Yeah, this is going to be quite annoying, on Linux I only got it to work with system-wide mbedtls (not with built-in) and using @fire curl_config.h.

Faless · 2021-07-28T14:10:44Z

While we should certainly strive for as much unit tests coverage as possible, I didn't see any existing tests for the http client, so I've decided to leave that for an exercise for another day (and potentially another person).

I had testing for HTTPClient on a separate repository like the one you are proposing (those are not unit tests):
https://github.com/Faless/gd-tests/

We can use those to verify the correct behaviour, the main problem was that websites would, from time to time slightly change their layout, so of course tests would unexpectedly break.

Faless · 2021-07-28T14:22:38Z

Size comparison (platform=linuxbsd target=release builtin_mbedtls=no):
Curl: 53436 KiB
NoCurl: 53176 KiB
Diff: ~260 KiB (could probably save some more with LTO)

jonbonazza · 2021-07-29T04:08:47Z

Currently, we only resolve one address for a domain. Godot's resolver APIs support resolving multiple

Wasn't this implemented by #49026?

Yes, the engine supports it. This code, however does not. I am also unsure whether it's possible to do in an async manner. It's also worth mentioning that the current HTTPClient implementation does not seem to do multi address resolution, so Im gonna leave it to be outside the scope of this PR for now.

jonbonazza · 2021-07-29T05:13:21Z

Okay I've trimmed down the TODO list a bit.

Only fetch the body during read_response_body_chunk. Not during poll ❗

Fixed this.

Also, @Faless I've updated the code to use the async resolver polling. It's basically the same code as the existing HTTPClientTCP you referenced, so nothing too crazy there.

Still can't get IPv6 to work and still not quite groking the blocking mode stuff. I've left a couple messages in the dev chat.

jonbonazza · 2021-08-29T05:45:45Z

This is ready for review.

I've tested this on Windows using the attached project, however it should probably be tested on linux, osx, android, and ios as well for posterity.

curl_test.zip

modules/mbedtls_curl/http_client_curl.h

Faless · 2021-08-29T12:02:56Z

Would be great to squash, leaving 2 commits, one adding the third-party library the other adding the module itself.

thirdparty/README.md

fire · 2021-08-29T17:49:45Z

@jonbonazza Will work on rebasing into two prs and resolving the reviews.

fire · 2021-08-30T00:23:18Z

Historical note: The module name mbedtls_curl was done to force this module to build after mbedtls. If it is possible to rename to curl, it would be better.

fire · 2021-08-30T03:44:46Z

In file included from thirdparty/curl/lib/vtls/mbedtls.c:59:
thirdparty/curl/lib/sendf.h:37:2: error: "missing VARIADIC macro define, fix and rebuild!"
#error "missing VARIADIC macro define, fix and rebuild!"
 ^
Compiling ==> thirdparty/curl/lib/bufref.c
1 error generated.
scons: *** [thirdparty/curl/lib/vtls/mbedtls.windows.opt.tools.64.o] Error 1
In file included from thirdparty/curl/lib/altsvc.c:35:
thirdparty/curl/lib/sendf.h:37:2: error: "missing VARIADIC macro define, fix and rebuild!"
#error "missing VARIADIC macro define, fix and rebuild!"
 ^
1 error generated.
scons: *** [thirdparty/curl/lib/altsvc.windows.opt.tools.64.o] Error 1
In file included from thirdparty/curl/lib/asyn-thread.c:67:
thirdparty/curl/lib/sendf.h:37:2: error: "missing VARIADIC macro define, fix and rebuild!"
#error "missing VARIADIC macro define, fix and rebuild!"
 ^
1 error generated.
scons: *** [thirdparty/curl/lib/asyn-thread.windows.opt.tools.64.o] Error 1
In file included from thirdparty/curl/lib/vtls/vtls.c:55:
thirdparty/curl/lib/sendf.h:37:2: error: "missing VARIADIC macro define, fix and rebuild!"
#error "missing VARIADIC macro define, fix and rebuild!"
 ^
1 error generated.
scons: *** [thirdparty/curl/lib/vtls/vtls.windows.opt.tools.64.o] Error 1

Using mingw llvm.

/bin/bash -c "PATH=/opt/llvm-mingw/bin:$PATH scons werror=no platform=windows target=release_debug -j`nproc` use_lto=no deprecated=no use_mingw=yes use_llvm=yes use_lld=yes use_thinlto=yes LINKFLAGS=-Wl,-pdb= CCFLAGS='-g -gcodeview' debug_symbols=no custom_modules=../godot_custom_modules"

jonbonazza · 2021-12-07T18:36:24Z

@fire I've reduced the commits down to 2.

Faless · 2022-01-12T19:06:04Z

It doesn't so handle HEAD requests correctly:

@onready
var req = $HTTPRequest

# Called when the node enters the scene tree for the first time.
func _ready():
        req.connect("request_completed", Callable(self, "_completed"))
        # Stalls instead of completing after the header like the current implementation does.
        req.request("https://google.com/", [], false, HTTPClient.METHOD_HEAD)

proj_httprequest_head.zip

Please refer to https://github.com/Faless/gd-tests/ (quite outdated) for some test URL.

Please test at least:

GET/HEAD/POST both w/ w/o SSL, both w/ w/o known content length.
Chunked/Content-Length keep-alive.
Read-till-EOF.

Would be better to also test:

PUT/DELETE (very common in rest APIs)
OPTIONS (not sure, but should be supported by the current implementation, for CORS pre-flight).

jonbonazza · 2022-01-27T09:34:25Z

@Faless okay, I've tested every HTTP method I could think of and both chunked and not chunked for the ones that made sense. I also went through all the tests in your test project and made sure my responses matched what the tests expected. I also added all of the default headers that the old http client was adding.

One thing is I did screw up the git history so I ended up just squashing everything down.

jonbonazza · 2022-01-27T09:38:07Z

FWIW, a suuuper handy tool for doing these tests was https://httpbin.org

mhilbrunner · 2022-01-27T09:41:53Z

I just want to chime in and say that its awesome how consistently you've been working away on this. Thanks for the efforts!

jonbonazza · 2022-01-27T14:09:56Z

More bugs were founds. Squashing. 🐞

Co-authored-by: Jon Bonazza <jonbonazza@gmail.com> Co-authored-by: Fabio Alessandrelli <fabio.alessandrelli@gmail.com>

jonbonazza · 2022-01-27T14:52:11Z

Ooookay. Squashed all the bugs I found. @fire the issue you found should also be resolved.

fire · 2022-01-27T21:47:18Z

I can still see a bug. Having trouble reducing a test case.

int-72h · 2023-03-01T16:26:51Z

This feature looks great (as at the moment I'm having to use a GDNative extension), will it be merged anytime soon?

andy5995 · 2023-03-20T15:13:51Z

This feature looks great (as at the moment I'm having to use a GDNative extension), will it be merged anytime soon?

Looks like it has merge conflicts at the moment.

fire · 2023-03-20T15:43:28Z

This feature is abandoned.

mhilbrunner · 2023-06-02T14:36:29Z

Closing after discussion at the Valencia sprint - besides this being obviously abandoned now, this was more difficult than anticipated, the upsides were smaller than hoped, and all in all the networking team now feels this is firmly in GDExtension territory for now.

Huge thanks to jonbonazza for exploring this and spending so much time on the implementation. (And of course to everyone reviewing :))

YeldhamDev added enhancement topic:core topic:network labels Jul 28, 2021

Calinou added this to the 4.0 milestone Jul 28, 2021

jonbonazza force-pushed the curl branch from 7bd76bb to 1add3f7 Compare August 28, 2021 21:02

jonbonazza marked this pull request as ready for review August 29, 2021 05:28

jonbonazza requested review from a team as code owners August 29, 2021 05:28

YuriSizov requested review from a team and removed request for a team August 29, 2021 10:41

Faless reviewed Aug 29, 2021

View reviewed changes

modules/mbedtls_curl/http_client_curl.h Outdated Show resolved Hide resolved

Faless reviewed Aug 29, 2021

View reviewed changes

thirdparty/README.md Show resolved Hide resolved

Faless reviewed Aug 29, 2021

View reviewed changes

thirdparty/README.md Show resolved Hide resolved

fire force-pushed the curl branch 2 times, most recently from d028cdd to 8546adb Compare August 29, 2021 18:20

YuriSizov removed the request for review from a team August 29, 2021 18:29

jonbonazza force-pushed the curl branch from eae5ee3 to 8a62fe6 Compare December 7, 2021 18:35

jonbonazza force-pushed the curl branch 2 times, most recently from 9ae14fa to c918d7e Compare December 7, 2021 20:49

jonbonazza force-pushed the curl branch 5 times, most recently from e0567f3 to 5f019e6 Compare January 27, 2022 09:32

jonbonazza force-pushed the curl branch from 5f019e6 to ffc6159 Compare January 27, 2022 09:39

Add libcurl module.

19150aa

Co-authored-by: Jon Bonazza <jonbonazza@gmail.com> Co-authored-by: Fabio Alessandrelli <fabio.alessandrelli@gmail.com>

jonbonazza force-pushed the curl branch from ffc6159 to 19150aa Compare January 27, 2022 14:51

akien-mga modified the milestones: 4.0, 4.x Jan 19, 2023

mhilbrunner closed this Jun 2, 2023

mhilbrunner removed this from the 4.x milestone Jun 2, 2023

mhilbrunner mentioned this pull request Jun 2, 2023

Add HTTP client implementation using libcurl as a module, optionally replacing the default one godotengine/godot-proposals#2903

Closed

akien-mga added the archived label Jun 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add HTTPClient implementation using curl as a module #50964

feat: add HTTPClient implementation using curl as a module #50964

jonbonazza commented Jul 28, 2021 •

edited

Loading

Calinou commented Jul 28, 2021 •

edited

Loading

Faless commented Jul 28, 2021

Faless commented Jul 28, 2021

Faless commented Jul 28, 2021 •

edited

Loading

jonbonazza commented Jul 29, 2021

jonbonazza commented Jul 29, 2021

jonbonazza commented Aug 29, 2021 •

edited

Loading

Faless commented Aug 29, 2021

fire commented Aug 29, 2021

fire commented Aug 30, 2021 •

edited

Loading

fire commented Aug 30, 2021 •

edited

Loading

jonbonazza commented Dec 7, 2021

Faless commented Jan 12, 2022 •

edited

Loading

jonbonazza commented Jan 27, 2022

jonbonazza commented Jan 27, 2022

mhilbrunner commented Jan 27, 2022 •

edited

Loading

jonbonazza commented Jan 27, 2022

jonbonazza commented Jan 27, 2022

fire commented Jan 27, 2022

int-72h commented Mar 1, 2023

andy5995 commented Mar 20, 2023

fire commented Mar 20, 2023

mhilbrunner commented Jun 2, 2023

feat: add HTTPClient implementation using curl as a module #50964

feat: add HTTPClient implementation using curl as a module #50964

Conversation

jonbonazza commented Jul 28, 2021 • edited Loading

Scope

Remaining Work

Regarding Unit Tests

A Note About The Commits

Calinou commented Jul 28, 2021 • edited Loading

Faless commented Jul 28, 2021

Faless commented Jul 28, 2021

Faless commented Jul 28, 2021 • edited Loading

jonbonazza commented Jul 29, 2021

jonbonazza commented Jul 29, 2021

jonbonazza commented Aug 29, 2021 • edited Loading

Faless commented Aug 29, 2021

fire commented Aug 29, 2021

fire commented Aug 30, 2021 • edited Loading

fire commented Aug 30, 2021 • edited Loading

jonbonazza commented Dec 7, 2021

Faless commented Jan 12, 2022 • edited Loading

jonbonazza commented Jan 27, 2022

jonbonazza commented Jan 27, 2022

mhilbrunner commented Jan 27, 2022 • edited Loading

jonbonazza commented Jan 27, 2022

jonbonazza commented Jan 27, 2022

fire commented Jan 27, 2022

int-72h commented Mar 1, 2023

andy5995 commented Mar 20, 2023

fire commented Mar 20, 2023

mhilbrunner commented Jun 2, 2023

jonbonazza commented Jul 28, 2021 •

edited

Loading

Calinou commented Jul 28, 2021 •

edited

Loading

Faless commented Jul 28, 2021 •

edited

Loading

jonbonazza commented Aug 29, 2021 •

edited

Loading

fire commented Aug 30, 2021 •

edited

Loading

fire commented Aug 30, 2021 •

edited

Loading

Faless commented Jan 12, 2022 •

edited

Loading

mhilbrunner commented Jan 27, 2022 •

edited

Loading