Page MenuHomePhabricator

[nix] Ensure that fd 3 is closed before we fork `redis-server` and `mariadbd`
ClosedPublic

Authored by atul on Mar 8 2023, 11:02 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 23 2024, 11:47 AM
Unknown Object (File)
Feb 23 2024, 11:10 AM
Unknown Object (File)
Feb 23 2024, 10:31 AM
Unknown Object (File)
Feb 23 2024, 9:35 AM
Unknown Object (File)
Feb 23 2024, 9:35 AM
Unknown Object (File)
Feb 23 2024, 5:28 AM
Unknown Object (File)
Feb 21 2024, 4:07 AM
Unknown Object (File)
Feb 21 2024, 2:46 AM
Subscribers

Details

Reviewers
ashoat
tomek
jon
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rCOMMa245d2e2f38f: [nix] Ensure that fd 3 is closed before we fork `redis-server` and `mariadbd`
Summary

It looks like fd 3 needs to be closed IN ADDITION to STDIN/STDOUT/STDERR for direnv to return control to the user.

By closing fd 3 after we fork but before we exec redis-server and mariadbd, we can ensure that they don't cause direnv to hang.

Kind of annoying, but direnv is not designed to handle launching of daemons.. and we're doing some custom stuff instead of launchd.

Test Plan

I can use direnv seamlessly without any issue.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Owners added a reviewer: Restricted Owners Package.Mar 8 2023, 11:02 AM
atul published this revision for review.Mar 8 2023, 11:07 AM

and we're doing some custom stuff instead of launchd

Launchd doesn't have a good way to do user services (.plist file configuring a service doesn't have a way to dynamically resolve paths, e.g. $HOME). For system services, this is "easier" because you can use the traditional /etc, /var, /tmp, and /opt unix directories for the services. The reason for preferring user services was that I wanted to avoid prompting the user for sudo similar to how homebrew will ask you for a sudo password when doing brew services commands.

But if someone is more familiar with launchd and able to make them into a launchd user service then that would also be great. Might be possible of instead of trying to defer to launchd to resolve paths, just render the .plist file with user paths already hardcoded. In other words, resolve paths like $HOME before writing the launchd file, and just assume that a user won't change their home directory (very unlikely).

Again, it's very hard to find good documentation on launchd 2.0.

Clashing with homebrew services will still be relevant even if we move to launchd as certain ports will overlap.

nix/mariadb-up-mac.nix
20 ↗(On Diff #23546)

Do you mind adding a comment so the context is not lost

jon requested changes to this revision.Mar 8 2023, 11:19 AM
This revision now requires changes to proceed.Mar 8 2023, 11:19 AM

Thanks for the explanation, make sense. I'll add a comment (and restart macOS build).

address feedback

nix/redis-up-mac.nix
22

Snuck in a little typo fix here too

reposition comment in mariadb-up-mac for symmetry

This revision is now accepted and ready to land.Mar 8 2023, 11:40 AM

Have you checked if Redis and MariaDB work correctly after this change? Are we sure that closing this descriptor doesn't cause any issue for a process that opened it?

In D7009#208076, @tomek wrote:

Have you checked if Redis and MariaDB work correctly after this change? Are we sure that closing this descriptor doesn't cause any issue for a process that opened it?

Yeah they work correctly after this change. fd3 is opened by direnv and isn't used/needed by mariadbd or redis-server at all. fd3 wouldn't be "passed" to mariadbd or redis-server if we just launched them manually from an interactive shell, for example.

For additional context, fd3 is used by direnv dump to "export the inner bash state at the end of execution." Here's the source from the direnv repo:

e6803e.png (1×1 px, 237 KB)

(https://github.com/direnv/direnv/blob/646fdc0183fe626661ce659e423f4bcce2d6e0a8/internal/cmd/cmd_dump.go#L33)

Basically searched through the repo for usages of os.Open/os.OpenFile/os.NewFile to see what resources were being created/opened and narrowed it down to this. Also makes sense that direnv is creating an "escape hatch" for logs from the new bash process it starts up. For context, here's how direnv "works":

11437e.png (398×1 px, 76 KB)


It's still possible that this change might cause issues in other unexpected ways, I'll keep an eye out. I also know at least @bartek is also using Redis/MariaDB via Nix, so please let me if you run into anything!