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

Unix socket cleanup #4533

Closed

Conversation

ForestJohnson
Copy link
Contributor

I am using Caddy to serve Unix domain sockets on the internet. When I restart Caddy, I see this error:

{"error":"loading new config: http app module: start: unix: listening on //var/run/caddy-serve-http.sock: listen unix //var/run/caddy-serve-http.sock: bind: address already in use"} 

Unix Socket listeners in golang are supposed to unlink the socket file when closed.

However they will not be unlinked in the case that the caddy process is killed instead of terminated (SIGKILL vs SIGTERM) because .Close() is never called on the unix domain socket listener. That's why I was seeing this error: my setup was not cleanly exiting caddy, it was .Kill()ing it.

In this case, the next time we try to listen, we'll get an "address already in use error" because the file already exists.

I propose this change to fix this issue:

  • If there already is a file when we are about to try to listen, check if it is a unix domain socket file, and if it is, then unlink it before attempting to listen

As an alternative, it may be possible to somehow begin listening on the already created file without unlinking and creating a new one? I know some client applications can have issues with this "socket file flickers in and out of existence" behavior.

I saw this: stackoverflow.com -- golang-accept-on-already-opened-fd but I'm not sure if that would work, as it's just a file on disk not a currently open file descriptor, and I haven't tried it yet.

Another possible alternative: simply log a better error message explaining that the reason this error is happening is probably because the caddy process was not exited cleanly.

@CLAassistant
Copy link

CLAassistant commented Jan 18, 2022

CLA assistant check
All committers have signed the CLA.

@francislavoie francislavoie requested a review from mholt January 18, 2022 20:21
@francislavoie francislavoie added the bug 🐞 Something isn't working label Jan 18, 2022
@francislavoie francislavoie added this to the v2.5.0 milestone Jan 18, 2022
@francislavoie
Copy link
Member

Hmm, interesting. What about the case where someone (maybe accidentally/incorrectly) starts a 2nd instance of Caddy which might try to bind to the same unix socket? I'm not sure it makes sense to unlink it in that case because it might actually (correctly) be in use.

@ForestJohnson
Copy link
Contributor Author

Might be best to simply do the error message instead ? I was also pondering the possibility of caddy running as root and being capable of breaking things by deleting some unix sockets used by other processes. Sort of a security edge case, but still something that came to mind.

When I first made this PR I did not even know that I could fix the issue by exiting caddy cleanly. Doing the error message certainly doesn't hurt anything

@mholt
Copy link
Member

mholt commented Jan 18, 2022

Always exit Caddy cleanly, just like any other server. 💯

We can improve the error message, but I'll close this PR as this isn't really a solution (and I don't think there's anything really to fix here). Thank you! Feel free to open a different PR if you want to contribute a better error message.

@mholt mholt closed this Jan 18, 2022
@mholt mholt added declined 🚫 Not a fit for this project and removed bug 🐞 Something isn't working labels Jan 18, 2022
ForestJohnson added a commit to ForestJohnson/caddy that referenced this pull request Jan 19, 2022
mholt pushed a commit that referenced this pull request Jan 19, 2022
* explain cryptic unix socket listener error related to process kill

#4533

* less ambiguous wording: clean up -> delete

* shorten error message explanation

* link back to pull request in comment for later archeaology
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
declined 🚫 Not a fit for this project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants