Expose getaddrinfo errors. Was: Return empty list on EAI_ errors for getaddrinfo@luv (#351) #352

haesbaert · 2022-10-20T10:55:42Z

Unix.getaddrinfo used in u-ring ignores gar_errno and returns an empty list, this makes the luv backend follow a similar behaviour.

Should fix #351 where we would get EAI_NONAME (or others) if resolution failed, this is a bit defensive as we just handle expected EAI errors.

haesbaert · 2022-10-20T10:56:55Z

cc @patricoferris

talex5 · 2022-10-20T14:02:49Z

lib_eio_luv/eio_luv.ml

+    | Error `EAI_ADDRFAMILY | Error `EAI_AGAIN | Error `EAI_BADFLAGS | Error `EAI_BADHINTS
+    | Error `EAI_CANCELED   | Error `EAI_FAIL  | Error `EAI_FAMILY   | Error `EAI_MEMORY
+    | Error `EAI_NODATA     | Error `EAI_NONAME| Error `EAI_OVERFLOW | Error `EAI_PROTOCOL
+    | Error `EAI_SERVICE    | Error `EAI_SOCKTYPE -> []


This list seems excessive (even if it does match the stdlib version). In particular, EAI_AGAIN, EAI_BADFLAGS, EAI_MEMORY and EAI_SOCKTYPE seem like they should be reported as errors, rather than claiming there are no addresses. Maybe EAI_FAIL too.

I thought about it too, just some notes:
EAI_FAIL and EAI_AGAIN (unlike EAGAIN) can be the result of a an actual DNS reply (would have to dig deeper, but those should return an empty list). EAI_SOCKTYPE, EAI_BADFLAGS, EAI_FAMILY we could, since they are mostly programming errors from our side.
I don't have a strong opinion, but I'd err on the side of just emulating the same behaviour of Unix.getaddrinfo which ignores everything.

I did a quick grep in the OpenBSD tree, just to see the most common idiom.
Basically it's just log gai_errno -> return NULL, some cases are a bit more careful into checking for EAI_NONAME and EAI_NODATA and not logging it as an error.

I wonder if we should just copy the getaddrinfo API and not allow an empty result. The wrapper function could enforce this:

let getaddrinfo ?(service="") (t:#t) hostname = match t#getaddrinfo ~service hostname with | [] -> raise (Dns_error (Failure "No addresses for ...")) | xs -> xs

I guess most people getting an empty list will want to report an error, and it's a bit silly you have to guess what the error was when we just threw the real error away. e.g. in with_tcp_connect we currently do:

| [] -> raise (Connection_failure (Failure (Fmt.str "No TCP addresses for %S" host)))

Then we could say if that's because the DNS server is busy, the entry doesn't exist, doesn't have an IP address, etc.

(edited since I misread the first time)
I like this better than the empty list, but if we're going this direction we could expose the gai_strerror in Dns_error so the user has something more meaningful than "not found". That would require providing a C stub for u-ring that exposes the error.

Thinking a bit more about this, I think connect should take a Sockaddr.stream list, and loop over the candidates until it succeeds connecting to one. This is virtually what everyone does with getaddrinfo + connect and bind.
In this case connect itself could raise something, getaddrinfo could still return just an empty list, and the user would still have to handle an exception.

Yes, the error code was just an example. We can use a better exception name and contents.

I think connect should take a Sockaddr.stream list, and loop over the candidates until it succeeds connecting to one.

with_tcp_connect already handles looping over the candidates; I'm not sure there's much benefit to changing connect too. In fact, that would just mean having to do something for the extra case of an empty list.

In this case connect itself could raise something, getaddrinfo could still return just an empty list, and the user would still have to handle an exception.

I don't see how that helps. We'd still be losing the detailed error from getaddrinfo and replacing it with a guess.

Yes, the error code was just an example. We can use a better exception name and contents.

I think connect should take a Sockaddr.stream list, and loop over the candidates until it succeeds connecting to one.

with_tcp_connect already handles looping over the candidates; I'm not sure there's much benefit to changing connect too. In fact, that would just mean having to do something for the extra case of an empty list.

empty list would return some Dns_error 'no candidates found'.

In this case connect itself could raise something, getaddrinfo could still return just an empty list, and the user would still have to handle an exception.

I don't see how that helps. We'd still be losing the detailed error from getaddrinfo and replacing it with a guess.

Yes, I was just assuming you wouldn't want a getaddrinfo stub that exports the getaddrinfo errno 😄 .
I'd prefer to export the error from getaddrinfo and also make connect loop, so users don't connect (List.first (getaddrinfo...)).
I don't feel too strong about it, I just want to move forward so both backends have the same behaviour.
Should we just raise all errors from getaddrinfo on both cases ? That involves the stub and creating some Dns_error values.

can I get a ruling here ?

Should we just raise all errors from getaddrinfo on both cases?

Yes, that makes sense to me.

( Having connect take a Sockaddr.stream list doesn't seem useful to me. You can't give a good error if there are no addresses - you don't know what the DNS error was, what the failing hostname was, or even whether DNS was used at all! )

patricoferris · 2022-10-20T14:53:53Z

Thanks @haesbaert ! This works for me, but I'll differ to @talex5's knowledge of which EAI_ errors to catch and which to no :))

talex5 · 2022-10-20T14:58:08Z

I'll differ to @talex5's knowledge of which EAI_ errors to catch and which to no :))

I have no special knowledge - just reading the getaddrinfo man-page :-)

…#351) Unix.getaddrinfo used in u-ring ignores gar_errno and returns an empty list, this makes the luv backend follow a similar behaviour.

…ronment.

haesbaert · 2022-12-01T09:48:02Z

Some corrections to my previous statement, getaddrinfo(3) cannot return an empty list and no error, it's either something or an error. I checked libc.

This PR should be ~~ready for reviewing~~ (debian is failing, looking into it), some notes:

A lot of the stub is just copied from ocaml's stdlib
The values chosen for the undefined EAI_* values are taken from libuv (since it works for them it works for us), they're added for completion sakeness and a future guard. I'm sure once we have some other backend those will popup and we will move the stub, or maybe glibc adds them in the future, who knows.
The strings for getaddrinfo_error_to_string were copied verbatim from libc.

haesbaert · 2022-12-01T10:08:31Z

Tests are zefixed, marking this as ready for review

Another note, I've added the stubs to a new file for two reasons

It's too polluting
Since it's a lot of copy and paste from stdlib, I have to preserve the copyright. ~~Actually now I noticed EIO's is not GPL, so I should get a clear from Xavier to relicense those bits in EIO~~, forget that, it's LGPL.

talex5

Looks sensible.

Note that I'm changing the error handling quite a lot in #378, which will impact this PR a bit.

talex5 · 2022-12-01T15:48:47Z

lib_eio_linux/getaddrinfo_stubs.c

+/*   All rights reserved.  This file is distributed under the terms of    */
+/*   the GNU Lesser General Public License version 2.1, with the          */
+/*   special exception on linking described in the file LICENSE.          */


I think we want to avoid LGPL code in Eio (it's currently BSD and ISC only). Though they might relicense this bit if asked.

@gasche @xavierleroy (sorry for the ping).
I've copied significant part of otherlibs/unix/getaddrinfo.c into EIO (https://github.com/ocaml-multicore/eio/blob/61b737d5be683b9be611844b16c7e73b5bece09a/lib_eio_linux/getaddrinfo_stubs.c). The original file is LGPLed and we would like to relicense our new file to ISC.
Do you permit us to relicense the bits we copied out from LGPL to ISC in EIO ?

No. I only use copyleft licenses for my free software. Please consider using the LGPL with OCaml linking exception for your library, or do not reuse any of my code.

No. I only use copyleft licenses for my free software. Please consider using the LGPL with OCaml linking exception for your library, or do not reuse any of my code.

Thank you, will do one of the two as suggested.

dra27

Drive-by comments on the borrowed C code!

dra27 · 2022-12-01T17:12:52Z

lib_eio_linux/getaddrinfo_stubs.c

+#include <caml/unixsupport.h>
+#include <caml/socketaddr.h>
+
+static value caml_unix_cst_to_constr(int n, int *tbl, int size, int deflt)


You could avoid this duplication by borrowing the extern declaration in cst2constr.h

fixed, somehow my brain remembered the function being static in first place.

dra27 · 2022-12-01T17:19:02Z

lib_eio_linux/getaddrinfo_stubs.c

+  if (retcode == 0) {
+    for (r = res; r != NULL; r = r->ai_next) {
+      v = caml_alloc_small(2, Tag_cons);
+      Field(v, 0) = convert_addrinfo(r);


This violates GC rule 5 - the convert_addrinfo call needs to be done before the caml_alloc_small and stored in a registered local (convert_addrinfo allocates).

thanks for spotting this, fixed it now

Many thanks to dra27 ! Somehow this function was static on first place in my head.

When I removed the `e` and I violated GC #rule 5. Basically a small block must be fully initialized before we trigger the next allocation, TIL.

talex5 reviewed Oct 20, 2022

View reviewed changes

haesbaert marked this pull request as draft October 26, 2022 12:48

haesbaert added 4 commits December 1, 2022 08:17

Return empty list on EAI_ errors for getaddrinfo@luv (ocaml-multicore…

2e09d49

…#351) Unix.getaddrinfo used in u-ring ignores gar_errno and returns an empty list, this makes the luv backend follow a similar behaviour.

works

97f093d

EINVAL on bad EAI

a26ddac

Move things to net and implement libuv

b419150

haesbaert force-pushed the getaddrinfo branch from cb602ac to b419150 Compare December 1, 2022 07:17

haesbaert added 12 commits December 1, 2022 09:10

Move getaddrinfo stubs into its own file due to copyrights

b81b9a8

zap gai_strerror bindings, copy strings from libc

864a742

Add Net.getaddrinfo_to_string

e93f944

Add EAI_PROTOCOL and don't be sloppy with defines

6029e39

restore eio_stubs properly now that we moved getaddrinfo

2ada9c7

whitespace

4505148

EAI_PROTOCOL WHOOPS

17bc361

Add EAI_NONAME tests

82812a1

EAI_OVERFLOW

5a2a401

Document semantics in net.mli

4e08267

Better paranoia

8b13591

We can't really guarantee the error code since it depends on the envi…

bc2a62c

…ronment.

fix tests

928617d

haesbaert marked this pull request as ready for review December 1, 2022 10:08

zap duplicate

61b737d

talex5 reviewed Dec 1, 2022

View reviewed changes

dra27 reviewed Dec 1, 2022

View reviewed changes

Just extern the declaration so we don't need to copy cst_to_constr.

3cb0647

Many thanks to dra27 ! Somehow this function was static on first place in my head.

Address dra27 point ocaml-multicore#2, many thanks again !

83bda02

When I removed the `e` and I violated GC #rule 5. Basically a small block must be fully initialized before we trigger the next allocation, TIL.

haesbaert changed the title ~~Return empty list on EAI_ errors for getaddrinfo@luv (#351)~~ ~~Return empty list on EAI_ errors for getaddrinfo@luv (#351)~~ Expose getaddrinfo errors Dec 1, 2022

haesbaert changed the title ~~~~Return empty list on EAI_ errors for getaddrinfo@luv (#351)~~ Expose getaddrinfo errors~~ Expose getaddrinfo errors. Was: Return empty list on EAI_ errors for getaddrinfo@luv (#351) Dec 1, 2022

raphael-proust mentioned this pull request Feb 6, 2023

fix setting Lwt_process env on Windows ocsigen/lwt#967

Open

raphael-proust mentioned this pull request Aug 7, 2023

Prepare for release ocsigen/lwt#1000

Merged

talex5 mentioned this pull request Oct 6, 2024

Add advice about using AI for code generation #765

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose getaddrinfo errors. Was: Return empty list on EAI_ errors for getaddrinfo@luv (#351) #352

Expose getaddrinfo errors. Was: Return empty list on EAI_ errors for getaddrinfo@luv (#351) #352

haesbaert commented Oct 20, 2022

haesbaert commented Oct 20, 2022

talex5 Oct 20, 2022

haesbaert Oct 20, 2022 •

edited

Loading

haesbaert Oct 20, 2022 •

edited

Loading

talex5 Oct 20, 2022

haesbaert Oct 20, 2022 •

edited

Loading

haesbaert Oct 21, 2022

talex5 Oct 24, 2022

haesbaert Oct 24, 2022

haesbaert Oct 31, 2022

talex5 Oct 31, 2022

patricoferris commented Oct 20, 2022

talex5 commented Oct 20, 2022

haesbaert commented Dec 1, 2022 •

edited

Loading

haesbaert commented Dec 1, 2022 •

edited

Loading

talex5 left a comment

talex5 Dec 1, 2022

haesbaert Dec 1, 2022

xavierleroy Dec 1, 2022

haesbaert Dec 1, 2022

dra27 left a comment

dra27 Dec 1, 2022

haesbaert Dec 1, 2022

dra27 Dec 1, 2022

haesbaert Dec 1, 2022

Expose getaddrinfo errors. Was: Return empty list on EAI_ errors for getaddrinfo@luv (#351) #352

Are you sure you want to change the base?

Expose getaddrinfo errors. Was: Return empty list on EAI_ errors for getaddrinfo@luv (#351) #352

Conversation

haesbaert commented Oct 20, 2022

haesbaert commented Oct 20, 2022

Choose a reason for hiding this comment

haesbaert Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

haesbaert Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haesbaert Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricoferris commented Oct 20, 2022

talex5 commented Oct 20, 2022

haesbaert commented Dec 1, 2022 • edited Loading

haesbaert commented Dec 1, 2022 • edited Loading

talex5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dra27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haesbaert Oct 20, 2022 •

edited

Loading

haesbaert Oct 20, 2022 •

edited

Loading

haesbaert Oct 20, 2022 •

edited

Loading

haesbaert commented Dec 1, 2022 •

edited

Loading

haesbaert commented Dec 1, 2022 •

edited

Loading