Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

dualstack for tcp #635

Closed
txdv opened this Issue · 2 comments

1 participant

@txdv

Dualstack binding for udp is already supported.
Tcp would be nice too!

@txdv

I'll just use this issue to propose different API's for udp and for tcp regarding dual stack.

On all Unices which support dual stack, dual stack is on by default. If it doesn't support dual stack, well, it is off. At least it is the assumption of the libuv code. Now I found this Article which says that there are some unices where IPV6Only is on by default. On linux you can set IPV6Only on by default if 1 is written in /proc/sys/net/ipv6/bindv6only. So libuvs assumption is wrong and it means that the current behaviour of libuv is faulty in the this case.

Libuvs assumption about windows dualstack is that it is off by default and it is consistent with the documentation

The current API looks like this;

int uv_udp_bind(uv_udp_t* handle, struct sockaddr_in addr,  unsigned flags);
int uv_udp_bind6(uv_udp_t* handle, struct sockaddr_in6 addr, unsigned flags);

The uv_udp_bind flags argument takes only a 0. Seems like a useless argument. Well maybe in the future it could be useful, right?
The uv_udp_bind6 flags argument can take only IPV6Only. Without it, it will use the dual stack if the ipaddr argument is "::" (it is the counterpart of "0.0.0.0" for ipv4, IPV6AddressAny).

So now when we have code with uv_udp_bind6(handle, IPV6AddressAny, 0), it will turn on dual stack on every supported platform. But if the platform doesn't support dual stack, it will silently do nothing and only bind against ipv6. Now if I supply IPV6Only on the platforms which don't support dualstack, it will throw a not supported error thow technically if IPV6_V6ONLY is not defined, the platform is not likely to support dual stacking anyway (i'm not sure about this, according to this I am right).

It seems more intuitive. In this case if you want to write correct code which behaves on all platforms the same, you have to execute multiple binds and check against errors.

Solution 1

Instead of calling the option IPV6Only, we inverse it and call it DUALSTACK. We ensure that on all platforms which have IPV6_V6ONLY defined the socket option IPV6_V6ONLY is turned on, the other platforms won't support dual stacking anyway. So now if the user executes in uv_udp_bind(handle, IPV6AddressAny, 0) he will bind only against the ipv6 address, and only if he supplies the DUALSTACK flag, it will bind against the ipv4.

This seems more intuitive to me, because dualstacking seems like an additional feature and with 0 as the flag argument it doesn't signalize that to the user. This is what this pull requests does: #652

Solution 2

Now the idea of inversing the attribute name to DUALSTACK will stay. I think it makes it clearer to the user what he get's when he binds. But I am still not impressed with the api, uv_udp_bind has a flags argument which is basically useless. So instead of adding an additional flag, we add an additional method:

int uv_udp_bind(uv_udp_t* handle, int port);
int uv_udp_bind4(uv_udp_t* handle, struct sockaddr_in addr);
int uv_udp_bind6(uv_udp_t* handle, struct sockaddr_in6 addr);

uv_udp_bind will now bind with the dual stack function on (since addr has to be IPV6AddrAny (or "::") anyway, we save the user from the trouble writing it on it's own).
uv_udp_bind4 can bind only against ipv4 addresses and uv_udp_bind6 only against ipv6 addresses.

No useless arguments, only an additional function which makes the pain of supplying IPV6AddrAny go away. But it breakes the API, libuv is not stable yet, right? #652

Solution 3

We still leave the DUALSTACK option, but use an additional method:

uv_udp_dualstack(uv_handle_t*, int);

Naturally it takes 0 and 1(0 is the default). On ipv4 bound handles it would return ENOTSUP, on ipv6 bound handles it would return ENOTSUP if the platform doesn't support dual stacking.

TCP

All the above API changes can be applied to tcp to, just change uv_udp to uv_tcp.

Additional API change

Since we are already breaking the api with Solution 2, which is my favorite, why don't we make the sockaddr struct arguments to pointers in uv_udp_bind4 and uv_udp_bind6.
So there would be

int uv_udp_bind(uv_udp_t* handle, struct sockaddr* addr);
int uv_udp_binda(uv_udp_t* handle, int port);

The only downside of this is that the user can't supply uv_ip4_addr directly as an argument, into uv_udp_bind, but would have rather create a local struct sockaddr_in addr or struct sockaddr_in6 addr6 and convert it to (struct sockaddr*) like this:

struct sockaddr_in addr = uv_ip4_addr("127.0.0.1", 7000);
uv_udp_bind(handle, (struct sockaddr*)&addr);

instead of

uv_udp_bind(handle, uv_ip4_addr("127.0.0.1", 7000));
uv_udp_bind6(handle, uv_ip4_addr("::", 7000));

It causes a lot of pain when writing bindings to push entire struct arguments instead of pointers to the function. And some dude on the #vala channel said that passing addr structs by value seems not well minded.

@txdv

Indutny goes with Solution 3: indutny/libuv@dd927aa

@txdv txdv closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.