-
Notifications
You must be signed in to change notification settings - Fork 187
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
Support fork-free locking notification mechanism #42
Conversation
Can you gate this behind a meson option? |
Yeah sure. Not sure why that would be necessary considering there are no new dependencies but it can't hurt :) |
I'd just like to see this get more popular among init systems and/or downstream distributions before we start shipping it for everyone. |
Added the meson option, defaulting to false. |
How would you feel about a similar option in sway itself, by the way? |
Sway users could just put |
|
||
env = getenv("READY_FD"); | ||
|
||
if (!env || env[0] == '\0') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really needed to have an argument for READY_FD? Can't we just enable it if the env variable is defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could do that. I don't in this case for two reasons:
-
While I hope this does not happen, the variable could be leaked from a parent process. In such a case the contents are likely invalid or at least not intended to be used by swaylock.
-
If a user configures their service with an older version of swaylock, the supervisor will be twiddling its thumbs while it waits for a notification that will never come. This would lead to some unexpected (although not particularly nasty) behavior. A flag would make such a misconfiguration glaringly obvious (
swaylock: unrecognized option '--readyfd'
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a blocker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
Regarding sway, that is a good stop gap and I may personally use it in the near term (particularly since I don't expect such changes to be included in the 1.0 cycle). However, it is not ideal because care would have to be taken to ensure that sway does not leak the variable and file descriptor to a significant number of child processes. Edit: I have found a better solution: don't use a ready fd at all with sway, and just notify the supervisor via other means. Like |
4f7187e
to
1fd061d
Compare
Disappointed that this was not considered for the recent release. Back to patching I guess. |
I'd really prefer this to be backed by service managers. |
There are two existing. |
What are they? I don't consider that s6 supports it (it supports a similar mechanism, but not READY_FD). Isn't it possible to discuss with service manager maintainers and ask what they think about the spec? |
Unfortunately no HTML documentation, but the manpage describes the support for it in startup. Indeed s6 does not support the protocol (it supports a more flexible mechanism... which is why it can wrap READY_FD easily), but it will function with this patch. Do you know of anyone else who works on service managers? Lennart shared his thoughts about the spec on twitter already. Not particularly interested in interacting with him further on this subject. |
For reference, discussion on Twitter: https://twitter.com/pid_eins/status/1094611367938674688 (Why shouldn't we add support for NOTIFY_SOCKET instead, which is almost the same and is widely used?)
|
You certainly could. It would require linking to libsystemd, which you already optionally do. I have some criticisms of it for other use cases, but it makes some sense here. |
Makes sense. Moving forward, I'd be interested in what OpenRC, s6 and runit maintainers think about this. Would it be possible to get in touch and ask their thoughts? |
runit maintainer is gone, has been for years. I asked some of the Void members for reviews of the reference implementation. @skarnet stopped by on the cups patch apple/cups#5507. They did not seem to be opposed to the protocol, but then again they did not explicitly comment on the protocol but instead focused on s6 itself. edit: I think container runtime maintainers are important stakeholders, but I would rather come to them with a patch than some docs and example code. |
Thanks for the feedback! |
I think it would be useful to send a message on the s6 mailing list and open an issue on the OpenRC repo to ask if they would be interested in merging a patch adding READY_FD support. |
There is no need to patch s6 for READY_FD support, it will work out of the box. Just have the run script start with I don't know about OpenRC. @emersion: I very much recommend against supporting NOTIFY_SOCKET. NOTIFY_SOCKET is much more complex (the extra features add nothing of value, those people don't understand YAGNI) and, as the name implies, it forces the notification IPC to be a socket, which favours a centralized architecture such as systemd and is actively hostile to decentralized architectures such as s6. Not making any assumptions on the nature of the file descriptor used for notification is much more flexible and much better. Note that I provide a wrapper for people who want to use the s6 notification mechanism under systemd; whereas to my knowledge, systemd doesn't provide anything for people who want to use the NOTIFY_SOCKET mechanism under s6. So, supporting the s6 mechanism (or READY_FD, which is almost equivalent) is more generic and can still work with systemd. |
Is there a reason why it doesn't work out-of-the-box? |
Probably because I came later, and allowing the service to dictate the fd number requires less work for the service. (No parsing env variables needed, just write to 3 or whatever). Basically it is a trade off between flexibility on the service manager side and simplicity on the service side. |
It does work out-of-the-box. s6 lets the service choose any notification fd it wants; you, the user, have more choice, so you just need to pick a number, and tell both the service (via the READY_FD variable) and the supervisor (via the |
3840fdd
to
b13d1c3
Compare
this is actually not true. the api is trivially implementable in anything. |
Add Gentoo package
If I rebased, would this (possibly) be accepted? Or should I just close it? |
To summarize on the s6 side:
Essentially, the implementation on this PR has no incompatibilities. It merely requires being set up correctly. It doesn't work out of the box because Source: https://skarnet.org/software/s6/notifywhenup.html |
Here's a rebased version of this branch: Conflicts were trivial. |
Related: #281 |
This implements a readiness notification mechanism which works on both systemd and s6. References: swaywm#42 References: swaywm#275
Superseded by #281 |
This allows for service/task managers to be notified when the lock screen has been displayed. It is particularly useful when locking the screen before closing the lid. The daemonization option can also be used, but then you either lose supervision of the swaylock process or resort to hacks like ptrace or cgroups.
Documentation for READY_FD can be found here.
It is loosely based on s6's service readiness notification mechanism, and compatible with s6.