Add local unix socket support #86
Conversation
|
Hey there, thanks again for contribution! Thanks! |
sundermann
force-pushed
the
local-socket
branch
3 times, most recently
from
7b86104
to
d9ac6ba
Compare
sundermann
force-pushed
the
local-socket
branch
from
d9ac6ba
to
bd81cca
Compare
src/service.c
Outdated
| char* address = service->settings->address; | ||
| if (service->settings->unix_socket) { | ||
| address = "127.0.0.1"; | ||
| } |
There was a problem hiding this comment.
Please move this piece of code / overwrite to main.c, after parse_options was called.
Basically overwriting address in global variable settings by doing settings.address = "127.0.0.1
There was a problem hiding this comment.
That won't work in case of a reconnect.
There was a problem hiding this comment.
Can you explain in more detail in which case it would break?
There was a problem hiding this comment.
unix_socket is just a boolean value so where would the unix socket address be stored if it's already overriden just after parsing options. I can only think of making this work by changing unix_socket to be a char* containing the socket address.
There was a problem hiding this comment.
Why does the target address/path for the socket need to be stored?
On a later call to hyperion_client it will receive the flag unix_socket again and know that it has to do the scanning for path /tmp/hyperhdr-domain again - to see if its still available.
The logic stays the same.
The overwrite to address 127.0.0.1 is really just used to enable communication to the JSON RPC, when unix domain socket is used.
There was a problem hiding this comment.
Yes, I misunderstood your idea at first thinking you wanted to compare the hostname against a list of known sockets. However in fact you want to scan if a specific local socket is available. I agree the second idea can work but still I don't like the idea of scanning against a hardcoded list because the socket address could change or hyperion.ng decides to implement local sockets as well.
There was a problem hiding this comment.
Another issue is that the -a parameter is still mandatory even though in case of local sockets -a is not mandatory and overwritten making it a bit unintuitive to use.
There was a problem hiding this comment.
agreed, so the flow is:
- Set /tmp/hyperhdr-domain in the IP address field
- Set whatever in port or leave it the default by not providing it
Result: unix domain socket is used for flatbuffers, Tcp socket for json rpc.
Sounds good!
There was a problem hiding this comment.
Yes, that is how it is supposed to work.
src/service.c
Outdated
| char* address = service->settings->address; | ||
| if (service->settings->unix_socket) { | ||
| address = "127.0.0.1"; | ||
| } |
| { | ||
| _origin = origin; | ||
| _priority = priority; | ||
| _connected = false; | ||
| _registered = false; | ||
| sockfd = 0; | ||
| struct sockaddr_in serv_addr; |
There was a problem hiding this comment.
Add checking for /tmp/hyperhdr-domain here. Iterate over constant array of possible filelocations, in case Hyperion.NG implements local unix socket support too.
If such unix socket handle is found, use it as parameter to _connect_unix_socket.
There was a problem hiding this comment.
Does this mean we should remove the command line option?
There was a problem hiding this comment.
No, the cmdline is fine and useful in this case. Just when it knows that a "local unix domain socket" is desired, it should check and use such, if available.
Case 1: No unix domain socket desired
- Supplied IP and Port is used as-is
Case 2: Unix domain socket is desired
- Hardcode ddress to 127.0.0.1 for HTTP RPC JSON communication to the daemon
- Before connecting the socket for flatbuffer data, check if known unix domain sockets exist and use them - otherwise fall back to 127.0.0.1.
There was a problem hiding this comment.
I'm not convinced this is the best option. This tightly couples hyperion-webos to a list of unix sockets which are provided by external software and might change in future. As you pointed out if hyperion.ng were to add a local socket this means that hyperion-webos needs changes as well.
sundermann
force-pushed
the
local-socket
branch
3 times, most recently
from
047d0be
to
df27e8b
Compare
|
Anything missing on this PR? |
|
Testing it currently |
|
Yup, works fine. Please cleanly rebase against main and correct the linting, then we should be good to go. cheers |
sundermann
force-pushed
the
local-socket
branch
2 times, most recently
from
a816306
to
f813209
Compare
Local unix socket support was added in HyperHDR in awawa-dev/HyperHDR@1100093 Adds the --unix-socket / -l switch in which case a connection over a local unix socket on the specified address is established. On my TV unix sockets are around 20-30% faster.
sundermann
force-pushed
the
local-socket
branch
from
f813209
to
a94be23
Compare
|
Thanks! |
Local unix socket support was added in HyperHDR in awawa-dev/HyperHDR@1100093
Local unix sockets are automatically used when the hostname is set to "/tmp/hyperhdr-domain" which is hardcoded in HyperHDR. On my TV local sockets are around 20-30% faster.