implement sd-notify status updates when systemd is used

gdamjan commented 4 years ago

this allows swayidle to be used in a systemd service with Type=notify it also updates the status line message to either: "active state" or "idle state"

ps. sd_notify(3) is a no-op when the program is not started as a service

markstos commented 3 years ago

Why is using Type=notify a better approach than starting swayidle with a systemd service type of "simple" as the sway-services project does?

https://github.com/xdbob/sway-services/blob/master/systemd/swayidle.service.in

boucman commented 3 years ago

Type= is about readiness. It tells systems at what point swayidle is ready to do its job. Once swayidle is ready, systemd is allowed to start any other service that is meant to be started after swayidle

Type=simple is the default type, it means we have no idea when swayidle is ready, so we start dependencies right after the call to exec()
Type=notify means that swayidle will explicitely tell systemd when it is ready (via a call to sd_notify() ) so it's the "best" type since systemd doesn't have to guess readiness and the readiness point is entirely under the control of the swayidle developers.

markstos commented 3 years ago

@boucman Thanks. Looks like a great contribution to me.

markstos commented 3 years ago

As part of this, it would be nice to also ship a systemd service file for standardization and convenience. The following service file could be shipped with swayidle:

[Unit]
Description=Idle manager for Wayland
Documentation=man:swayidle(1)
# ConditionEnvironment is introduced in systemd v246, causing this to only be launched in a Wayland session
ConditionEnvironment=WAYLAND_DISPLAY
PartOf=graphical-session.target
Requires=graphical-session.target
After=graphical-session.target

[Service]
Type=notify
Exec=/usr/bin/swayidle
Restart=always

[Install]
WantedBy=graphical-session.target

What do you think @boucman?

gdamjan commented 3 years ago

I'd be glad to add the unit file too, if there's any chance of this PR getting merged :)

boucman commented 3 years ago

I don't know user-session integration very well, so I might not have the knowledge to do a proper review... related documentation is here : https://systemd.io/DESKTOP_ENVIRONMENTS/ but I never had to look at it so far.

it seems strange to require graphical-session.target I'm not sure what the proper way is but that sounds strange.

I'll try to review it when I have a little more time...

Any way, the service probably deserves a separate PR rather than being part of this one...

gdamjan commented 3 years ago

it seems strange to require graphical-session.target I'm not sure what the proper way is but that sounds strange.

it doesn't require it, it's just the default target it would start in if the service is enabled.

The idea is,

if swayidle.service is enabled; and
graphical-session.target gets activated; only then swayidle would start

and graphical-session.target should only be activated when sway is running (is active).

boucman commented 3 years ago

you have Requires=graphical-session.target so

if swayidle.service is enabled, then graphical-session will be activated (by swayidle)

what you describe is the WantedBy part (which is correct) I have doubts about the other lines in [Unit] PartOf means that if graphical-session.target is stopped, swayidle will be stopped too (that sounds fine)

After means that that swayidle will be started after graphical--session is started, that sounds weird because swayidle should be part of graphical-session, so it should be ordered before. My guess is that you want swayidle to be ordered After=display-manager.service (which is the service that represents the display manager, or in our case, the compositor)

Requires means that if someone start swayidle, then graphical-session.target would be started too. That sounds strange, because, in theory graphical-session should be started by the DM, and only by the DM...

please check the man pages, in particular systemd.special, but i'm not completely clear on that part, so that's my understanding of how things are supposed to work.

markstos commented 3 years ago

@boucman I based my proposed systemd unit file based on what the sway-services project is using:

https://github.com/xdbob/sway-services/blob/master/systemd/swayidle.service.in

But I agree with your careful reading of it that it could be improved. Here's a second attempt, with annotations to note the value of each line. I took your advice to remove the Requires= and After= directives which are not necessary.

[Unit]
Description=Idle manager for Wayland
Documentation=man:swayidle(1)

# ConditionEnvironment is introduced in systemd v246, causing this to only be launched in a Wayland session
ConditionEnvironment=WAYLAND_DISPLAY

# Stop / Restart swayidle as "part of" a user's graphical session stop / restart
PartOf=graphical-session.target

[Service]
# swayidle will notify systemd via D-Bus when it's ready.
Type=notify
Exec=/usr/bin/swayidle
Restart=always

[Install]
# When enabled, swayidle will be started with the user's graphical session.
# The session will continue to start even if swayidle fails.
WantedBy=graphical-session.target

boucman commented 3 years ago

That's look good. the Restart=always means that swayidle will always be restarted, except when explicitely stopped by systemd itself.

in particular, it will be restarted if it stops "cleanly" I am not sure if that's the behaviour we want or if we want to let swayidle alone when it finishes cleanly. I don't remember exactly how swayidle behaves...

markstos commented 3 years ago

It's my understanding that swayidle is intended to run continuously because it's part of the security of the laptop-- it is in charge of locking the laptop when it is idle. So "Restart=always" is a valuable feature, ensuring that swayidle is maximally running.

This is feature alone is a reason to let systemd manage swayidle.

I currently run some tasks using Sway's exec that seem die and not get restarted. Sway's simple "exec" mechanism has no way to recover from exec tasks crashing. Restart=always solves that case.

On Thu, Nov 19, 2020 at 1:50 PM Boucman notifications@github.com wrote:

That's look good. the Restart=always means that swayidle will always be restarted, except when explicitely stopped by systemd itself.

in particular, it will be restarted if it stops "cleanly" I am not sure if that's the behaviour we want or if we want to let swayidle alone when it finishes cleanly. I don't remember exactly how swayidle behaves...

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/swaywm/swayidle/pull/83#issuecomment-730568330, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAGJZJLT6CD4CBLCR7SQKTSQVSG7ANCNFSM4SZPLZLQ .

-- Mark Stosberg Director of Systems and Security RideAmigos

boucman commented 3 years ago

i'm no expert, and i'm not sure how swayidle acts exactly... but the convention is that when a program terminates with a code of zero, it terminates correctly (no crash, no error) In other word the program has terminated on purpose, which usually means this is a normal thing.

so my question is: is there any cases where swayidle terminates voluntarely, and maybe Restart=on-failure would make more sense ?

markstos commented 3 years ago

@boucman I would flip the question around. Is there ever case when you would set swayidle to protect your laptop by locking the screen, but yet ou are OK with swayidle stopping and disabling the screen locking? Restart=always ensures the device remains always protected and the user is in control of that-- not whether if swayidle decides to exit with a 1 or a 0.

If the user really wants to stop swayidle, the user can stop the systemd service.

Since the proposed systemd file is optional, I expect users who opt to use it will be somewhat familar with systemd service management.

boucman commented 3 years ago

I don't think the service file should override the intended way for swayidle to work. Eihter swayidle should never terminate and Restart=always, or there is some case where it should (for example when receiving SIGTERM) in which case it should be Restart=on-failure

in any case, it is not the unit file that should force a behaviour. The unit file is here to explain to systemd how swayidle works. So again, the question is for the swayidle devs: is there a case where swayidle is meant to terminate ? (I didn't find any info on the man page)

markstos commented 3 years ago

@boucman From reading the source code, swayidle responds to SIGINT and SIGTERM by shutting down. SIGUSR1 is documented as immediately triggering the idle state.

It's normal for well-behaved services to respond to SIGTERM by shutting down. That's exactly how systemctl stop works-- it first sends a SIGTERM to processes so they can shutdown cleanly. Only If processes don't shut down cleanly after 90 seconds, a SIGKILL is sent.

https://stackoverflow.com/questions/42978358/how-systemd-stop-command-actually-works

If there were some other reason that swayidle should suddenly stop on its own, that would be a security concern and would presumably be documented in man swayidle.

boucman commented 3 years ago

if a user (not systemd) sends SIGTERM, then sway idle will terminate properly... and systemd will restart it right away. I am not sure that's the right behaviour. that's why I suggest Restart=on-failure

gdamjan commented 3 years ago

if a user (not systemd) sends SIGTERM, then sway idle will terminate properly... and systemd will restart it right away. I am not sure that's the right behaviour. that's why I suggest Restart=on-failure

that seems ok to me, since there's no reason for the user to send sigterm to swayidle and not actually do a systemctl stop. so if that's an some random error, swayidle should be restarted.

Anyway, I think this might be getting of-topic here? Maybe open a new PR when/if this one is merged.

boucman commented 3 years ago

Ok for a separate PR. I think we disagree on what should be the "desired behaviour"

So at this point, we need a dev to intervene...

emersion commented 1 year ago

I'd rather not add a systemd-specific startup notification mechanism. Closing because we're planning to drop logind support anyways.

WhyNotHugo commented 1 year ago

Note that you can use Type=forking so that systemd will consider the service started once the main process has [forked and] exited. Combine this with ExecStart=/usr/bin/swaylock -f, and you will effectively get what you're looking for: systemd will only consider the service started once the screen is effectively locked.

gdamjan commented 1 year ago

Note that you can use Type=forking so that systemd will consider the service started once the main process has [forked and] exited. Combine this with ExecStart=/usr/bin/swaylock -f, and you will effectively get what you're looking for: systemd will only consider the service started once the screen is effectively locked.

indeed, alas, I don't think the forking was in the proper place compared to the notify

boucman commented 1 year ago

I don't like Type=forking... I do'nt know about swayidle specifically, but it needs some very fragile synchronisation between the main process and the forked process

I.e the forked process has to tell the main process when it's ready and the main process has to wait for that signal and terminate only after that.

I'm not sure if swayidle does that....

WhyNotHugo commented 1 year ago

Ah, my bad, I mixed up swayidle and swaylock. What I said above applied to swaylock.

emersion commented 1 year ago

I.e the forked process has to tell the main process when it's ready and the main process has to wait for that signal and terminate only after that.

If the initialization is done before forking, there's no need for that.

But yeah, swayidle has no forking/daemonize flag.

boucman commented 1 year ago

does swayidle have a dbus API ?

If it takes a well-known name on the bus, systemd can use that as a readyness indicator

emersion commented 1 year ago

swayidle doesn't have a D-Bus API.

boucman commented 1 year ago

ok, last chance... is there a command line that we could launch next to swayidle and that would block until swayidle is ready ?

emersion commented 1 year ago

Currently, no.

emersion commented 1 year ago

For an init-independant way of doing readiness notifications, see https://github.com/swaywm/swaylock/pull/281.

swaywm / swayidle

implement sd-notify status updates when systemd is used #83