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 unix socket support #543

Merged
merged 4 commits into from
Apr 7, 2023
Merged

Conversation

clarfonthey
Copy link
Contributor

Enables listening on unix sockets by specifying a file path for the bind address. Fixes #542.

  • I have signed the CLA

@thebaer
Copy link
Member

thebaer commented Apr 29, 2022

Great addition -- thanks for submitting this! I'll test as soon as I can. In the meantime, anyone else should feel free to review!

@clarfonthey
Copy link
Contributor Author

I did run the automated tests and everything passed, although I haven't gotten around to testing the actual server yet. Technically other options for unix sockets could be added, but I figured I'd choose the option with the minimum surface area for now since I'm not sure how much that's desired.

Will add some extra remarks as needed once I get around to that.

@clarfonthey
Copy link
Contributor Author

clarfonthey commented May 11, 2022

Okay so, I finally got around to testing this, noticed a few issues, and fixed them. Notable changes:

  1. Error handling in golang is awful and I accidentally shadowed err, which is now fixed
  2. The server needs to delete any leftover socket files in order to actually listen on the socket; if it doesn't have permission
    to do this, it will just throw an error
  3. You need to update the permissions of the socket after listening to ensure any other than the writefreely user can access the socket (while not strictly required, good practice)
  4. I found out that http.Serve(listener, r) exists so I did that instead of (&http.Server{Handler: r}).Serve(listener)

I assume you still want to do some testing of your own, but at least as far as I'm concerned, this should be ready for review now.

Enables listening on unix sockets by specifying a file path for the bind address
@thebaer
Copy link
Member

thebaer commented Dec 26, 2022

I went ahead and cleaned the code up to be a little more idiomatic, and made it so the socket file is deleted on server shutdown.

I probably undid your error-naming changes -- I'm not sure what the error shadowing issue was that you mentioned, since past code was overwritten by force-push. But let me know if this causes any issues! Otherwise I tested and it works well, so I'll merge this soon.

@thebaer thebaer added this to the 1.0 milestone Dec 26, 2022
@clarfonthey
Copy link
Contributor Author

clarfonthey commented Dec 26, 2022

I don't remember what the error change was, but I was particularly frustrated with the compiler in that moment and the brute force approach worked. So, if your code works, that's good.

I have a particular problem with force-pushing, so, I should get better about that.

I didn't remove the socket initially since I figured it didn't matter much since it'd be overwritten on the next time it starts, but it is an improvement. Thank you.

Everything looks good to me. Appreciate you tidying this up for me.

@thebaer
Copy link
Member

thebaer commented Apr 7, 2023

Merging this now. Thanks again for working on this!

@thebaer thebaer merged commit ddabab0 into writefreely:develop Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Listening on unix socket
2 participants