Skip to content
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

Add the ability to listen on UNIX sockets #5112

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Caian
Copy link

@Caian Caian commented Dec 14, 2024

This is a continuation of #2111 that was closed.

As stated in the original PR, using unix sockets can have a beneficial impact on performance. I found that particularly relevant when running my invidious instance in a rootless container.

This PR adds the socket_binding option to config that specifies a socket to listen to. The socket_binding option will disable listening to TCP. I also added socket_permissions because often one needs to control who accesses the socket.

@Caian Caian requested review from SamantazFox, unixfox and a team as code owners December 14, 2024 00:58
@Caian
Copy link
Author

Caian commented Dec 14, 2024

Tested and running on my personal invidious behind a Caddy proxy with https enabled. Now videojs buffers the video much faster and the user-mode networking on the server consumes a less CPU and RAM.

Copy link
Member

@unixfox unixfox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can combine the two parameters into one.

And make sure to comment them otherwise socket binding will be on by default!

@Caian
Copy link
Author

Caian commented Dec 14, 2024

Maybe you can combine the two parameters into one.

And make sure to comment them otherwise socket binding will be on by default!

Ops, I fixed the configuration file to leave it disable. I prefer the idea of leaving separate parameters for clarity, but if the parameters were to be combined I was thinking of doing something like:

socket_binding: /tmp/invidious.socket,777

What do you think?

@unixfox
Copy link
Member

unixfox commented Dec 14, 2024

Maybe you can combine the two parameters into one.

And make sure to comment them otherwise socket binding will be on by default!

Ops, I fixed the configuration file to leave it disable. I prefer the idea of leaving separate parameters for clarity, but if the parameters were to be combined I was thinking of doing something like:

socket_binding: /tmp/invidious.socket,777

What do you think?

You can also do it this way:

socket_binding:
path: /tmp/invidious.socket
permission: 777

@Caian
Copy link
Author

Caian commented Dec 14, 2024

Maybe you can combine the two parameters into one.
And make sure to comment them otherwise socket binding will be on by default!

Ops, I fixed the configuration file to leave it disable. I prefer the idea of leaving separate parameters for clarity, but if the parameters were to be combined I was thinking of doing something like:
socket_binding: /tmp/invidious.socket,777
What do you think?

You can also do it this way:

socket_binding: path: /tmp/invidious.socket permission: 777

I like this solution, implemented.

@Caian Caian requested a review from unixfox December 15, 2024 14:09
@@ -177,7 +191,7 @@ https_only: false
## Configuration for using a HTTP proxy
##
## If unset, then no HTTP proxy will be used.
##
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't modify the other parameters please.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'll revert that, my editor automatically removed a trailing space...

@@ -138,6 +145,8 @@ class Config
property port : Int32 = 3000
# Host to bind (overridden by command line argument)
property host_binding : String = "0.0.0.0"
# Path and permissions to make Invidious listen on a UNIX socket instead of a TCP port - Example: /tmp/invidious.sock,777
property socket_binding : SocketBindingConfig? = nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could add some sort of checks. Like is the path correctly configured. And if the permission parameter entered are correct (numbers + is it a valid combination of numbers)

There are some examples here: https://github.com/iv-org/invidious/blob/master/src/invidious/config.cr#L225-L233

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll take a look!

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

Successfully merging this pull request may close these issues.

2 participants