fix!: create IPC socket in user runtime directory

[ThomasFrans]

Each user has their own runtime directory at /run/user/<uid>. Creating the IPC socket in there makes sure it is cleaned up regardless of whether ncspot exits normally.

BREAKING CHANGE: move IPC socket location

[hrkfdn]

Unfortunately this does not apply to all the platforms, i.e. the BSDs or macOS.

[ThomasFrans]

Oh, didn't know that. I'll make sure to install a BSD VM to test these things in the future. I just think that it would be nice to switch to /run/user/<uid> on Linux as all other programs do it that way as well. I looked through my entire home directory and ncspot is the only application that puts a socket file under there. I think it's always nice to follow platform conventions so first time users looking for the IPC socket know where it most likely is. If it would be an accepted change, I could see if I can conditionally change the path during compilation.

[hrkfdn]

I'm okay with this. I think we could probably use XDG_RUNTIME_DIR for this and then fall back to /tmp if it's not set?

[ThomasFrans]

I'll change the PR when I have some more time as I still have to properly set up GhostBSD for testing.

[ThomasFrans]

I'm okay with this. I think we could probably use XDG_RUNTIME_DIR for this and then fall back to /tmp if it's not set?

I think that is indeed a better solution than manually specifying the path. I don't know whether /tmp is the preferred place to put user runtime files when a specific one isn't available. I know the sticky bit prevents deletion by anyone except the root user, but they still have write access. Sure ncspot is only a media player and not a security critical piece of software, but I think we could maybe follow the approach of zellij and the Lua language server. They create their own 'user runtime directory' called /tmp/<program_name>-<uid> with the 0700 permissions which prevents write access to other users (just like the normal Linux runtime directory does).

The importance of the permissions is also something that is mentioned by the XDG Base Directory Specification.

I think if it will be changed, it should be done correctly the first time, so as not to put files all over the user's system after a while. That's why I spent some time looking up all the platform conventions. #[cfg(unix)] compiles on Linux and the BSDs, and probably MacOS (not sure about that last one). Here's what I think could be done:

1. On all platforms, check $XDG_RUNTIME_DIR since some platforms use this as their primary way of specifying directories, and on the systems that don't use them by default, they will either not be set in which case we go with the next option, or they will have been set explicitly by the user, so we try to use it.
2. On Linux, if $XDG_RUNTIME_DIR isn't set, try /run/user/<uid>/ and use that as user runtime directory.
3. On all platforms, try to create /tmp/ncspot-<uid> with 0700 permissions.
4. If none of these work, don't create an IPC socket.

(I'm going deeper and deeper down the rabbit hole of temporary directories... 😵‍💫)

[ThomasFrans]

I've marked this as ready for review as it now works like in the suggestion:

1. If XDG_RUNTIME_DIR is set, create it under $XDG_RUNTIME_DIR/ncspot, fail if not possible.
2. If XDG_RUNTIME_DIR isn't set and we're on Linux, create under /run/user/<uid>/ncspot if /run/user/<uid> exists.
3. In other cases, create under /tmp/ncspot-<uid>

In all these cases, the permissions are set correctly to avoid unauthorized access to the runtime files. This has been tested on both Arch Linux and FreeBSD. If a runtime directory can't be found, the code does panic in this version. I don't expect this to ever happen as every UNIX system should at least have a /tmp/ directory. It also panics when the user incorrectly set their XDG_RUNTIME_DIR. I can change that if needed.

I hope this isn't too complicated for end users. It should work like expected on every platform so users who are used to the platform they work with should easily find the runtime files. I also documented the change in the README. I understand if this PR isn't merged, since it's only a suggestion to try and conform to platform conventions, but it is also a breaking change after all.

[hrkfdn]

Thanks, looks good to me. You brought up one good point, though:

I don't know whether /tmp is the preferred place to put user runtime files when a specific one isn't available.

I didn't do enough research when I suggested that, sorry. I think /tmp would probably work for most systems and is not too uncommon, but you are totally right. Would the old ~/.cache/ncspot/ folder be a better choice?

[ThomasFrans]

I'd argue that using a directory inside /tmp/ is a better choice than XDG_CACHE_DIR/ncspot/.cache/ncspot for a few reasons:

1. /tmp/ is more related to temporary/runtime files than XDG_CACHE_DIR/~/.cache/ is. Personally I would always expect to find a program's runtime files in one of /tmp/, /run/ or /var/. A cache directory seems more fitting for real cache files: files that should persist across reboots and are used to speed up a program.
2. ncspot is currently the only program on my PC that puts a socket file in my entire home directory. While exceptions do exist, I don't think there is a special reason to put this socket file in the user's home directory.

The only reason why I would keep it there would be to keep the user experience the same, since it has been like this for a while now. On the other hand, IF it were to change, this would be the time to do it, before ncspot ever has a 1.0.0 release.

One small problem with the current implementation (and a really hard one to solve correctly) is that the application runtime directory (ncspot/ncspot-<uid>) isn't automatically cleaned up. This is on purpose as it might cause a bug in very unlikely cases where the directory gets deleted by one process as it's about to quit, right after another process has created the directory but not created any files yet. The chance of this happening in the real world is negligible, but I don't want to implement something knowing it's broken. Implementing this correctly would either require coordination between ncspot processes, which is total overkill, or each ncspot process should use its own runtime directory, namespaced by for example the pid. Then it would be possible to remove everything when the process quits.

A final thought about this PR would be that it could be interesting to provide a CLI command to obtain all the used paths (cache, config, runtime...). Some other programs do that as well, and it would make the documentation nicer. It could just say 'run ncspot paths' or include it in the help command or something like that, instead of explaining where to look for all the directories. It tells users where the directory is, and not where it might be, which is just a better user experience.

Sorry for the walls of text, I just want to get this right the first time as it's not nice for users to change it many times 😄

[ThomasFrans]

Temporary file library just for reference

Just discovered this which might clean up the code (no need for manual cleanup logic for temporary files/directories). It also mentions the difficulty of doing this manually, which probably is related to the possible bugs mentioned in the previous comment. It's already an indirect dependency of ncspot so might as well use it. I'm going to leave it here just for reference for now: https://crates.io/crates/tempfile

[hrkfdn]

Sounds good! I'd be fine with merging this for now. Are you OK with this?

One small problem with the current implementation (and a really hard one to solve correctly) is that the application runtime directory (ncspot/ncspot-) isn't automatically cleaned up. This is on purpose as it might cause a bug in very unlikely cases where the directory gets deleted by one process as it's about to quit, right after another process has created the directory but not created any files yet.

From what I can tell that seems like a negligible problem, as ncspot is only create a file socket and /tmp is volatile on most systems anyway. Not saying the improvement isn't welcome, but I wouldn't consider it critical :)

I have created a ticket for the CLI command idea which I really liked.

[ThomasFrans]

Yes, I think it's fine for now. I'll maybe try to find a fix for clearing the directory in the future. I'll also have a look at the CLI feature soon.

[ThomasFrans]

Let me just update the changelog...