Skip to content
This repository has been archived by the owner on Oct 1, 2023. It is now read-only.

[Network] allow setting socket options #106

Open
azjezz opened this issue Jan 28, 2020 · 11 comments
Open

[Network] allow setting socket options #106

azjezz opened this issue Jan 28, 2020 · 11 comments

Comments

@azjezz
Copy link
Contributor

azjezz commented Jan 28, 2020

see: facebook/hhvm#8451

@fredemmott fredemmott changed the title [Network] allow setting no-dely and keep-alive options on sockets [Network] allow setting no-delay and keep-alive options on sockets Jan 28, 2020
@fredemmott
Copy link
Contributor

Unless I'm missing something, the HHVM issue is not relevant here - the sockets are created with socket_create, which means that TCP_NODELAY and SO_KEEPALIVE can be set with socket_set_option

@azjezz
Copy link
Contributor Author

azjezz commented Jan 28, 2020

HHVM doesn't support no-dely / keep-alive 😞

@fredemmott
Copy link
Contributor

fredemmott commented Jan 28, 2020

That doesn't appear to be the case

hphpd> $sock = socket_create(AF_INET, SOCK_STREAM, SOL_TCP)
hphpd> socket_set_option($sock, SOL_SOCKET, /* random number */ 12345, 1)
Warning: unable to set socket option [42]: Protocol not available
hphpd> =socket_get_option($sock, SOL_SOCKET, SO_KEEPALIVE)
0
hphpd> =socket_get_option($sock, SOL_TCP, TCP_NODELAY)
=socket_get_option($sock, SOL_TCP, TCP_NODELAY)
0
hphpd> socket_set_option($sock, SOL_SOCKET, SO_KEEPALIVE, 1)
hphpd> socket_set_option($sock, SOL_TCP, TCP_NODELAY, 1)
hphpd> =socket_get_option($sock, SOL_SOCKET, SO_KEEPALIVE)
8
hphpd> =socket_get_option($sock, SOL_TCP, TCP_NODELAY)
4

@fredemmott fredemmott changed the title [Network] allow setting no-delay and keep-alive options on sockets [Network] allow setting socket options Jan 28, 2020
@fredemmott
Copy link
Contributor

that said, retitled as I won't be doing these in small batches; there's many SO_ and TCP_ options, and I'm hoping to avoid adding something as rough as mixed, but that might end up being necessary. At the least, there should be an enum for socket options, a different enum for tcp options, etc.

@azjezz
Copy link
Contributor Author

azjezz commented Jan 28, 2020

That doesn't appear to be the case

😱 i clearly remember they didn't exist back when i was working on #36

At the least, there should be an enum for socket options, a different enum for tcp options, etc.

wouldn't using a shape be "stricter" ? e.g, keep-alive value should be bool, while timeout should be float

@fredemmott
Copy link
Contributor

perhaps - probably separate shapes, then TCP\Conenct would take shape('socket_options' => SocketOptionsShape', 'tcp_options' => TcpOptionsShape) or something like that or options -> socket, options -> tcp

@azjezz
Copy link
Contributor Author

azjezz commented Jan 28, 2020

or maybe tcp options shape can have all socket options therefore it can be passed as both ? :)

@fredemmott
Copy link
Contributor

while timeout should be float

When we move to native implementaitons, I'm probably going to change all timeouts to be ints, or something like the timeval C struct - the native implementaiton is an int seconds and int microseconds. It's not as convenient, but avoids issues like #104

or maybe tcp options shape can have all socket options therefore it can be passed as both ? :)

  • I wouldn't want to do this unless we get shape splat
  • doesn't really fit with the type system - shape('TCP_NODELAY' => bool, 'SO_KEEPALIVE' => bool) is not a subtype of shape('SO_KEEPALIVE' => bool), and ... should definitely not be part of these definitions. Perhaps records could make that reasonable.

@fredemmott
Copy link
Contributor

fredemmott commented Jan 28, 2020

😱 i clearly remember they didn't exist back when i was working on #36

Looks like they were added in 2015; for both #36 and the HHVM issue, you were specific about the stream_socket_*() functions - they still appear to be unsupported for stream_context_set_option() - could be this is what you were referring to, instead of socket_set_option() ?

@azjezz
Copy link
Contributor Author

azjezz commented Jan 28, 2020

probably 🤔

@fredemmott
Copy link
Contributor

Splat would be really nice here; there's a few (SO_NREAD, SO_ERROR, SO_NWRITE in particular) that it would be nice to type as invalid for setsockopt, but valid for getsockopt, without duplciating the entire shape

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants