When listening on a unix domain socket don't print http:// and port #14180
Conversation
|
When investigating launching as a unix domain socket via something like "--host file.sock". I notice we were printing: listening on http://file.sock:8080 |
ericcurtin
force-pushed
the
logging-fix-sock
branch
from
f58ed81 to
afe88b5
Compare
|
A question also... When we terminate llama-server running as a socket, we don't delete the .sock file... Should we attempt to delete that on termination? |
|
Forgot a : here will add soon |
ericcurtin
force-pushed
the
logging-fix-sock
branch
from
afe88b5 to
196223c
Compare
tools/server/server.cpp
Outdated
| std::string full_url; | ||
| if (is_sock) { | ||
| full_url = params.hostname; | ||
| } else { | ||
| full_url = "http://" + params.hostname + ":" + std::to_string(params.port); | ||
| } | ||
|
|
||
| LOG_INF("%s: server is listening on %s - starting the main loop\n", __func__, full_url.c_str()); |
There was a problem hiding this comment.
nits: I prefer the log code to be short, so something like this looks better:
LOG_INF("%s: server is listening on %s - starting the main loop\n", __func__,
is_sock ? params.hostname.c_str() : string_format("http://%s:%d", ...).c_str());There was a problem hiding this comment.
I'm also wondering now is http+unix:// a better prefix for the unix socket case
There was a problem hiding this comment.
it will look weird for most users. trust me, more than 90% users out there don't even know what is a unix socket sorry misunderstood, I think just unix:// maybe enough?
There was a problem hiding this comment.
also the URL won't get highlighted in some terminal UI unless it has http:// or https:// prefix (which allow ctrl+click to open on browser)
In my experience working with unix socket before, I usually has code to detect if the .sock file already exist on application startup. If it exists, delete before creating a new one. The other way (delete upon termination) may fail if the app is killed for example. |
Instead show something like this: main: server is listening on file.sock - starting the main loop Signed-off-by: Eric Curtin <[email protected]>
ericcurtin
force-pushed
the
logging-fix-sock
branch
from
196223c to
168bf92
Compare
|
All done anyway... |
|
And now that I think of it, if we are to make --host an option for both http:// or unix:// , what we should have done is something like, default to http:// if no protocol is specified, if it starts with unix:// it's a socket, if it starts with http:// it's http. I don't know if it's too late to make the breaking change, a unix socket doesn't have to end in .sock |
|
I'm not the one who added this feature. I know unix sockets are not necessary having Is more intuitive and easier to remember than: It's just a minor UX. If you don't absolutely need the option without |
|
Yeah... That's curl-style to a degree... I don't need this option anyway, I'm fine with .sock files :) This PR is good to go for me at least |
Instead show something like this:
main: server is listening on file.sock - starting the main loop