Reimplemented http_client using the ocurl library by carlonluca · Pull Request #127 · ygrek/mldonkey

carlonluca · 2025-02-09T01:44:12Z

This commit reimplements the http_client module by using the ocurl library. This should add support for the https protocol.

carlonluca · 2025-02-09T01:47:33Z

The implementation is still a work in progress, which is the reason why it is a bit too verbose. This is the first patch needed to fix #119 and #41. Comments are welcome.

ygrek

cursory review

This commit reimplements the http_client module by using the ocurl library. This should add support for the https protocol.

ygrek

yes, getting there
didn't review http_client.ml completely yet

xOneca

I was going to test this PR, but found some issues with the configure script.

Also made a suggestion to use the app name instead of the library name as User-Agent.

Now that compiles for me, I'll be testing it some time...

xOneca · 2025-06-23T11:10:00Z

The changes in this file do not match configure.in and will be overwritten by running autoconf. Specially important with the two new exit 1 that only exist here.

Not sure why there are both. Maybe I'm supposed to generate the configure script and version it.

xOneca · 2025-06-23T11:17:50Z


 ICONS_CMXA=icons.cmxa

+LIBS_opt += $(OCAMLLIB)/threads/threads.cmxa


Shouldn't be searched using ocamlfind, as is done with Curl?

Don't know if there is a way to do it with ocamlfind alone:

# ocamlfind query threads /usr/lib/ocaml

xOneca · 2025-06-23T11:37:10Z

+let def_user_agent =
+  let ua = Curl.version_info () in
+  let v = ua.Curl.version in
+  Printf.sprintf "curl/%s" v


Why not identify the real application making the request? I would use MLDonkey/x.y.z or, if you prefer, MLDonkey/x.y.z curl/a.b.c. (See example in RFC 9110 § User-Agent)

TcpBufferedSocket.connect is using the first form for the proxy mode:

mldonkey/src/utils/net/tcpBufferedSocket.ml

Line 1355 in 0d44635

Printf.bprintf buf "User-Agent: MLdonkey/%s\n" Autoconf.current_version;

Also, there is a global option, used in many places:

mldonkey/src/daemon/common/commonOptions.ml

Lines 1017 to 1019 in 0d44635

let user_agent = define_option current_section ["user_agent"]

"User agent string (default = \"default\")"

string_option "default"

The value default uses MLDonkey/x.y.z format.

My patch should only switch the default from wget to curl. Callers already pass in the proper user agent.

xOneca · 2025-06-23T12:07:59Z

Also, I suppose GitHub workflow now needs libcurl-ocaml-dev installed too.

xOneca

Hi @carlonluca,

Thanks for your updates!

I think I didn't explain the changes correctly, so here is a second review round for the Autoconf files.. 😄

xOneca · 2025-06-25T17:47:30Z

So, here I was thinking this was the only HTTP client implementation in MLDonkey, and in fact there are two! 😕

As I can see, the HTTP client in this PR is used only for:

WebInfos: tested both HTTP and HTTPS and works perfectly! 💯
Torrent trackers: could not find (yet) an HTTPS tracker, but HTTP works!

And dllink <url> also seems to use it (to no avail):

2025/06/25 16:45:57 [dCmd] execute command "dllink" "https://cdimage.debian.org/debian-cd/current/amd64/bt-cd/debian-12.11.0-amd64-netinst.iso.torrent"
2025/06/25 16:45:57 [HTTPcl] whead
2025/06/25 16:45:57 [HTTPcl] wget_string https://cdimage.debian.org/debian-cd/current/amd64/bt-cd/debian-12.11.0-amd64-netinst.iso.torrent
2025/06/25 16:45:58 [HTTPcl] HTTP success: https://cdimage.debian.org/debian-cd/current/amd64/bt-cd/debian-12.11.0-amd64-netinst.iso.torrent
2025/06/25 16:45:58 [BT] Not_found and url has non valid torrent extension: Date: Wed, 25 Jun 2025 16:45:58 GMT
Server: Apache/2.4.63 (Unix)
Last-Modified: Sat, 17 May 2025 11:47:38 GMT
Accept-Ranges: bytes
Content-Length: 54073
Age: 3184
Content-Type: application/x-bittorrent

It seems to download the torrent file, but then fails to detect that it is, in fact, a torrent file and the torrent is not enqueued for download. Tried both HTTP and HTTPS. 😞

Seems to be parsing headers as the name of the file? Maybe searching the name in the headers? 🤔

carlonluca · 2025-06-25T20:38:06Z

Hello,
yes, there are two different implementations of HTTP client. I discussed this point somewhere in the issues. From what I can remember, one implementation is only used for FileTP, and I said I won't work on that. I think I tested HTTPS trackers with an Ubuntu torrent, and it worked.

Much time has passed since I sent this PR, but I think torrent files are downloaded with the new implementation. In fact, I tested your link and also this one https://releases.ubuntu.com/25.04/ubuntu-25.04-desktop-amd64.iso.torrent. Both started immediately.

I remember I had to fix multiple bugs related to this problem. Can you confirm you are not able to download that torrent you provided?

carlonluca · 2025-06-25T20:50:51Z

Ok, I suspect I missed some commits in this PR. I probably restricted this PR to the HTTPS client itself. This may fix the issue you are seeing: carlonluca/mldonkey@d4592d7. My fork includes this, but I can't find it in this PR.

Unfortunately my fork has diverged quite a bit. I'll have to go through everything again and see if something else is missing.

In the meantime, thanks for your review. I'll work on what is missing.

carlonluca · 2025-06-25T21:00:17Z

Ok, sorry, it seems you may need more than this PR to get proper HTTPS for bittorrent support. This is what I have in my fork: https://github.com/carlonluca/mldonkey/commits/dev/. You probably want to use that if you want bittorrent to work properly. This PR is strictly for the client.

Co-authored-by: xOneca <xoneca@gmail.com>

carlonluca · 2025-08-16T12:54:54Z

I moved my fork to dune and ocaml5, making this PR pretty outdated (more info here: https://bugfreeblog.duckdns.org/2025/08/mldonkey-ocaml5.html). I will not update this further so I close it.

ygrek requested changes Feb 10, 2025

View reviewed changes

Comment thread src/utils/net/http_client.ml Outdated

Comment thread config/Makefile.in Outdated

Reimplemented http_client using the ocurl library

91ee6c6

This commit reimplements the http_client module by using the ocurl library. This should add support for the https protocol.

carlonluca force-pushed the https-fix branch from 2455e3f to 91ee6c6 Compare February 10, 2025 18:28

Use ocamlfind to determine the proper build params for curl.

3b79f73

ygrek reviewed Feb 11, 2025

View reviewed changes

Comment thread config/Makefile.in Outdated

Comment thread config/configure.in Outdated

Comment thread src/utils/net/http_client.ml Outdated

carlonluca added 2 commits February 15, 2025 00:34

Improved configure script.

58358e7

Simplified syntax in http_client.

df669f6

xOneca reviewed Jun 23, 2025

View reviewed changes

carlonluca added 5 commits June 24, 2025 19:27

Use AC_MSG_CHECKING.

a34aabe

Use test instead of [].

0124125

Use AC_MSG_ERROR instead of AC_MSG_RESULT.

c65890a

Fix missing bracket.

8588952

Make configure properly fail.

1e411fd

xOneca reviewed Jun 25, 2025

View reviewed changes

Comment thread config/configure.in Outdated

Comment thread config/configure.in Outdated

Comment thread config/configure.in Outdated

Comment thread config/configure.in

Comment thread config/configure.in Outdated

Comment thread config/configure.in

Comment thread config/configure.in

carlonluca and others added 10 commits June 26, 2025 19:06

Updated config/configure.in.

fa99111

Co-authored-by: xOneca <xoneca@gmail.com>

Updated config/configure.in.

82b59cf

Co-authored-by: xOneca <xoneca@gmail.com>

Updated config/configure.in.

a8059ad

Co-authored-by: xOneca <xoneca@gmail.com>

Updated config/configure.in.

1c1cd46

Co-authored-by: xOneca <xoneca@gmail.com>

Updated config/configure.in.

0743159

Co-authored-by: xOneca <xoneca@gmail.com>

Updated config/configure.in.

f1246d3

Co-authored-by: xOneca <xoneca@gmail.com>

Fix message.

380b2b4

Removed duplicated line.

395014b

Fixed github ci script.

78d4d67

Add ocurl to opam.

e5bc97c

Do not install ocurl through apt.

c8e459e

carlonluca closed this Aug 16, 2025


		ICONS_CMXA=icons.cmxa

		LIBS_opt += $(OCAMLLIB)/threads/threads.cmxa

	let user_agent = define_option current_section ["user_agent"]
	"User agent string (default = \"default\")"
	string_option "default"

Conversation

carlonluca commented Feb 9, 2025

Uh oh!

carlonluca commented Feb 9, 2025

Uh oh!

ygrek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ygrek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xOneca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xOneca commented Jun 23, 2025

Uh oh!

xOneca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xOneca commented Jun 25, 2025

Uh oh!

carlonluca commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlonluca commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carlonluca commented Jun 25, 2025

Uh oh!

carlonluca commented Aug 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

carlonluca commented Jun 25, 2025 •

edited

Loading

carlonluca commented Jun 25, 2025 •

edited

Loading