swaylock icon indicating copy to clipboard operation
swaylock copied to clipboard

Support fork-free locking notification mechanism

Open CameronNemo opened this issue 5 years ago • 23 comments

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.

CameronNemo avatar Feb 04 '19 05:02 CameronNemo

Can you gate this behind a meson option?

ddevault avatar Feb 05 '19 01:02 ddevault

Yeah sure. Not sure why that would be necessary considering there are no new dependencies but it can't hurt :)

CameronNemo avatar Feb 05 '19 01:02 CameronNemo

I'd just like to see this get more popular among init systems and/or downstream distributions before we start shipping it for everyone.

ddevault avatar Feb 05 '19 01:02 ddevault

Added the meson option, defaulting to false.

CameronNemo avatar Feb 05 '19 02:02 CameronNemo

How would you feel about a similar option in sway itself, by the way?

CameronNemo avatar Feb 05 '19 02:02 CameronNemo

How would you feel about a similar option in sway itself, by the way?

Sway users could just put exec ready in their config file.

emersion avatar Feb 05 '19 08:02 emersion

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 exec ready as you said, just not ready fd... that way no fd to lose track of.

CameronNemo avatar Feb 05 '19 14:02 CameronNemo

Disappointed that this was not considered for the recent release. Back to patching I guess.

CameronNemo avatar May 04 '19 08:05 CameronNemo

I'd really prefer this to be backed by service managers.

emersion avatar May 04 '19 09:05 emersion

There are two existing.

CameronNemo avatar May 04 '19 09:05 CameronNemo

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?

emersion avatar May 04 '19 09:05 emersion

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.

CameronNemo avatar May 04 '19 09:05 CameronNemo

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?)

Do you know of anyone else who works on service managers?

https://alternativeto.net/software/systemd/

emersion avatar May 04 '19 09:05 emersion

(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.

CameronNemo avatar May 04 '19 09:05 CameronNemo

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?

emersion avatar May 04 '19 09:05 emersion

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 https://github.com/apple/cups/pull/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.

CameronNemo avatar May 04 '19 09:05 CameronNemo

Thanks for the feedback!

CameronNemo avatar May 04 '19 09:05 CameronNemo

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.

emersion avatar May 04 '19 09:05 emersion

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 env READY_FD=n ... if n is the value contained in the notification-fd file.

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.

skarnet avatar May 04 '19 17:05 skarnet

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 env READY_FD=n ... if n is the value contained in the notification-fd file.

Is there a reason why it doesn't work out-of-the-box?

emersion avatar May 04 '19 17:05 emersion

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.

CameronNemo avatar May 04 '19 20:05 CameronNemo

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 notification-fd file) what number you have chosen.

skarnet avatar May 04 '19 20:05 skarnet

You certainly could. It would require linking to libsystemd,

this is actually not true. the api is trivially implementable in anything. look for ex. this pure-python implementation https://github.com/bb4242/sdnotify/blob/master/sdnotify/init.py

gdamjan avatar Mar 03 '20 23:03 gdamjan

If I rebased, would this (possibly) be accepted? Or should I just close it?

CameronNemo avatar Dec 16 '22 20:12 CameronNemo

To summarize on the s6 side:

  • s6 allows the administrator to specify which file descriptor the daemon will use to notify its readiness. This is done via an unsigned integer in a config file. This value is called notification-fd.
  • An administrator can specify environment variables for services. This includes a variable READY_FD=.
  • An administrator can configure a service in a way that notification-fd matches the value of READY_FD=.

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 notification-fd must be defined explicitly. That is, support for readiness notification must be explicitly opted into by the administrator. There is no approach that will work "out of the box" because the feature is disabled by default.

Source: https://skarnet.org/software/s6/notifywhenup.html Source: https://skarnet.org/software/s6/servicedir.html

WhyNotHugo avatar Jan 13 '23 10:01 WhyNotHugo

Here's a rebased version of this branch:

https://github.com/swaywm/swaylock/commit/0b7bc2a2af6e4d008bfaa3a6d649e391c9e56e4f

Conflicts were trivial.

WhyNotHugo avatar Jan 13 '23 10:01 WhyNotHugo

Related: #281

emersion avatar Jan 27 '23 12:01 emersion

Superseded by #281

emersion avatar Feb 02 '23 09:02 emersion