Skip to content
This repository has been archived by the owner on Jul 25, 2024. It is now read-only.

Simplify port handling and discourage listening on a port. #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

FireMasterK
Copy link
Member

No description provided.

main.go Outdated
defer listener.Close()
srv.Serve(listener)
} else {
fmt.Println("WARNING: You are listening on a HTTP Socket, you are strongly recommended to use a Unix Domain Socket for performance reasons.")

Choose a reason for hiding this comment

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

I wouldn't say it's quite necessary to be this strong. Especially given that in many use cases, using unix sockets is impossible. To a novice user, this could be quite harmful.

Copy link

Choose a reason for hiding this comment

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

Notice or Note would be more appropriate than a warning.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the official guides, https://piped-docs.kavin.rocks/docs/self-hosting/#docker-compose-caddy-aio-script and https://piped-docs.kavin.rocks/docs/self-hosting/#docker-compose-with-nginx, it's always been recommended to use a Unix socket, so I don't see why this is unreasonable. Never should you run a public instance this way.

Choose a reason for hiding this comment

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

Suggested change
fmt.Println("WARNING: You are listening on a HTTP Socket, you are strongly recommended to use a Unix Domain Socket for performance reasons.")
fmt.Println("Note: You are listening on a HTTP Socket. Listening on a Unix Domain Socket may increase performance.")

Choose a reason for hiding this comment

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

Never should you run a public instance this way.

There's a difference between running with a port, and exposing that port to the internet directly without a reverse proxy infront of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a difference between running with a port, and exposing that port to the internet directly without a reverse proxy infront of it.

That's not the point here, even when using TCP/IP within localhost / between the reverse proxy and ytproxy, you're still wasting a significant amount of CPU resources unnecessarily.

Copy link

@3nprob 3nprob Aug 12, 2022

Choose a reason for hiding this comment

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

@FireMasterK In deployment scenarios with a service mesh (a la Istio or Consul Connect) or if one has particular reasons to keep the two processes separated - maybe they are even on different physical machines - it can be perfectly fine to prefer HTTP sockets.

Agreed that this is too strong and risks hurting more than it helps (unless there is some particular nuance specific to ytproxy I am not aware of)

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

Successfully merging this pull request may close these issues.

4 participants