-
Notifications
You must be signed in to change notification settings - Fork 3
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
IPv6 support #5
Comments
@jcourreges is helping patching this, seems like the whole usage of sockaddr through gethostbyname is unfortunate, and it might be that ocaml-ssl is also responsible for bad behaviour here. I incorporated his work so far on https://github.com/gilou/ocaml-cry/tree/ipv6 (diff @ master...gilou:ipv6) |
Thanks! This seems to be going the right direction. Keep us posted, I'll try to get to it when I have time. |
Fixed in #6! |
OK, I know we've closed that, but … As I was playing around, I tested liq 2.2 / ocaml-cry 1.0.2 … so:
That is, using a dual stack host, totally randomly e.g. stream.wolface.net, that one has A / AAAA records. output.icecast in liq connects using IPv4 from a dual stack client. This can be tested easily using /etc/hosts on Linux at least. This is not blocking, but it's not exactly nice either, as on Linux, it should conform to getaddrinfo(3) settings in /etc/gai.conf, which should prioritize IPv6 by default… |
Looks like a deliberate choice introduced in 6671b4a. I don't know the rationale behind it. I agree that it's generally better for applications to honor gai.conf and similar mechanisms. Application-specific settings can then favor/require v4/v6. The canonical pattern for using getaddrinfo(3) is a loop over a list of addresses, until it gets a valid socket. Why is it a problem for v4 being preferred over v6 and reversely for v6 over v4? Doesn't the code attempt using next addresses in case of failure? Or is it more about special cases, like IPv6 being broken (unusable, times out), or being cheaper and thus more interesting than IPv41? Footnotes
|
Well this is why I try to be super verbose in my commit comments these days but I guess I miss the mark here and now I have only vague ideas why. What I seem to remember is that, specifically for I cannot reproduce at the moment, however. I think I'll revert the changes and will add a configuration option. |
Ok I think I have a live example of the ipv4 preference issue: savonet/liquidsoap#3610 (comment) |
Well… not sure about that, if their machine has IPv6 connectivity, but can't reach the IPv6 servers… I'd say they have something else to fix (or it could call for a fallback). It's not good to bury that kind of decision in the last piece of the chain ;)
Could be a lot of issues there… |
Well I'm in the business of making things work for most users out of the box and giving alternative options for others so I think that, for now, we'll prefer |
I second this.
IPv4:
IPv6:
There's definitely a difference between IPv4 and IPv6 on the server side, it's not specific to the client, and it's not specific to liquidsoap. However, the getaddrinfo loop should survive to an |
Well I think the question we have to answer is: given the existence of both I have two examples here and you can see a long list here related to when node changed their default address resolution. Then, there's also docker whose support for Looking at the code it is in fact trying all addresses in order: let rec connect_any ?bind_address ?timeout (addrs : Unix.addr_info list) =
match addrs with
| [] -> raise Not_found
| [addr] ->
(* Let a possible error bubble up *)
connect_sockaddr ?bind_address ?timeout addr.ai_addr
| addr :: tail -> (
try connect_sockaddr ?bind_address ?timeout addr.ai_addr
with _ -> connect_any ?bind_address ?timeout tail)
in
connect_any ?bind_address ?timeout (resolve_host ~prefer host port) The error is actually raised during a I'm not sure what's causing a let connect_sockaddr ?bind_address ?timeout sockaddr =
let domain = Unix.domain_of_sockaddr sockaddr in
let socket = Unix.socket ~cloexec:true domain Unix.SOCK_STREAM 0 in
(try
match bind_address with
| None -> ()
| Some s -> Unix.bind socket (sockaddr_of_address s)
with exn ->
let bt = Printexc.get_raw_backtrace () in
begin
try Unix.close socket with _ -> ()
end;
Printexc.raise_with_backtrace exn bt);
let do_timeout = timeout <> None in
let check_timeout () =
match timeout with
| Some timeout ->
(* Block in a select call for [timeout] seconds. *)
let _, w, _ = select [] [socket] [] timeout in
if w = [] then raise Timeout;
Unix.clear_nonblock socket;
socket
| None -> assert false
in
let finish () =
try
if do_timeout then Unix.set_nonblock socket;
Unix.connect socket sockaddr;
if do_timeout then Unix.clear_nonblock socket;
socket
with
| Unix.Unix_error (Unix.EINPROGRESS, _, _) -> check_timeout ()
| Unix.Unix_error (Unix.EWOULDBLOCK, _, _) when Sys.os_type = "Win32" ->
check_timeout ()
in
try finish ()
with e ->
let bt = Printexc.get_raw_backtrace () in
begin
try Unix.close socket with _ -> ()
end;
Printexc.raise_with_backtrace e bt |
Ok. I think I found the issue: we were not properly checking for errors when connecting in unblocking mode: 8afc835 |
Heh, we arrived to the same conclusion and the same code! :) FWIW I had trouble confirming that checking the socket error improved things, since on OpenBSD I hit the I'm definitely not convinced by the Anyway, I'll stop the rant here, I'm starting to sound like a zealot. You're thinking about your users first, and I totally respect that. Cheers, |
No that makes sense. I'm glad we got to the bottom of it, @gilou was right there was something underlying it. I have a build reverting back to system default with the fix above, I'll ask the user with the error to test it. |
As mentionned in savonet/liquidsoap#1425 ocaml-cry seems to be misbehaving when it comes to IPv6.
Using an explicit IPv6, this happens, as if it was trying to resolve it… great.
If the name is ipv4/ipv6 resolvable it ignores the IPv6, which is … not what is supposed to happen.
I can provide an IPv6 reachable client/server if it helps.
I'm vaguely guessing something in connect or in unix_transport is fishy, but I'm reaaaally not good at OCAML :P
The text was updated successfully, but these errors were encountered: