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

gdamjan · 2020-10-21T09:04:58Z

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 · 2020-11-18T15:31:51Z

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 · 2020-11-18T17:29:43Z

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 · 2020-11-18T17:36:00Z

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

markstos · 2020-11-18T17:55:08Z

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 · 2020-11-18T18:07:27Z

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

boucman · 2020-11-18T18:40:41Z

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 · 2020-11-18T18:50:36Z

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 · 2020-11-18T20:34:18Z

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 · 2020-11-19T14:41:13Z

@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 · 2020-11-19T18:50:09Z

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 · 2020-11-20T19:59:05Z

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 ***@***.***> 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 <#83 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAGJZJLT6CD4CBLCR7SQKTSQVSG7ANCNFSM4SZPLZLQ> .

-- Mark Stosberg Director of Systems and Security RideAmigos

boucman · 2020-11-20T22:42:51Z

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 · 2020-11-23T15:30:22Z

@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 · 2020-11-23T17:37:04Z

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 · 2020-11-24T15:33:02Z

@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 · 2020-11-24T18:41:26Z

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 · 2020-11-24T23:40:29Z

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 · 2020-11-25T17:19:43Z

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

this allows swayidle to be used in a systemd service with Type=notify it also updates the status line to either: "active state" or "idle state" ps. `sd_notify(3)` is a no-op when the program is not started as a service

emersion · 2022-11-07T11:22:03Z

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

WhyNotHugo · 2022-11-07T11:55:58Z

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 · 2022-11-07T12:02:29Z

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 · 2022-11-07T15:21:05Z

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 · 2022-11-07T15:22:51Z

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

emersion · 2022-11-07T15:23:22Z

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 · 2022-11-07T15:38:08Z

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 · 2022-11-07T15:43:52Z

swayidle doesn't have a D-Bus API.

boucman · 2022-11-07T16:56:55Z

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

emersion · 2022-11-07T17:04:13Z

Currently, no.

emersion · 2023-07-04T09:23:13Z

For an init-independant way of doing readiness notifications, see swaywm/swaylock#281.

markstos mentioned this pull request Nov 18, 2020

Launch swayidle with Type=notify after upstream merge xdbob/sway-services#17

Closed

gdamjan force-pushed the sd-notify branch from 10e21cc to 8ccea65 Compare December 19, 2020 23:38

nikobockerman mentioned this pull request Feb 4, 2021

Memory leaks in parse_timeout? #92

Closed

implement sd-notify status updates when systemd is used

f2ebdf3

this allows swayidle to be used in a systemd service with Type=notify it also updates the status line to either: "active state" or "idle state" ps. `sd_notify(3)` is a no-op when the program is not started as a service

gdamjan force-pushed the sd-notify branch from ed06e65 to f2ebdf3 Compare September 18, 2022 21:31

emersion closed this Nov 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gdamjan commented Oct 21, 2020

markstos commented Nov 18, 2020 •

edited

Loading

boucman commented Nov 18, 2020

markstos commented Nov 18, 2020

markstos commented Nov 18, 2020

gdamjan commented Nov 18, 2020

boucman commented Nov 18, 2020

gdamjan commented Nov 18, 2020

boucman commented Nov 18, 2020

markstos commented Nov 19, 2020 •

edited

Loading

boucman commented Nov 19, 2020

markstos commented Nov 20, 2020 via email

boucman commented Nov 20, 2020

markstos commented Nov 23, 2020

boucman commented Nov 23, 2020

markstos commented Nov 24, 2020

boucman commented Nov 24, 2020 •

edited

Loading

gdamjan commented Nov 24, 2020

boucman commented Nov 25, 2020

emersion commented Nov 7, 2022

WhyNotHugo commented Nov 7, 2022 •

edited

Loading

gdamjan commented Nov 7, 2022

boucman commented Nov 7, 2022

WhyNotHugo commented Nov 7, 2022

emersion commented Nov 7, 2022

boucman commented Nov 7, 2022

emersion commented Nov 7, 2022

boucman commented Nov 7, 2022

emersion commented Nov 7, 2022

emersion commented Jul 4, 2023

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

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

Conversation

gdamjan commented Oct 21, 2020

markstos commented Nov 18, 2020 • edited Loading

boucman commented Nov 18, 2020

markstos commented Nov 18, 2020

markstos commented Nov 18, 2020

gdamjan commented Nov 18, 2020

boucman commented Nov 18, 2020

gdamjan commented Nov 18, 2020

boucman commented Nov 18, 2020

markstos commented Nov 19, 2020 • edited Loading

boucman commented Nov 19, 2020

markstos commented Nov 20, 2020 via email

boucman commented Nov 20, 2020

markstos commented Nov 23, 2020

boucman commented Nov 23, 2020

markstos commented Nov 24, 2020

boucman commented Nov 24, 2020 • edited Loading

gdamjan commented Nov 24, 2020

boucman commented Nov 25, 2020

emersion commented Nov 7, 2022

WhyNotHugo commented Nov 7, 2022 • edited Loading

gdamjan commented Nov 7, 2022

boucman commented Nov 7, 2022

WhyNotHugo commented Nov 7, 2022

emersion commented Nov 7, 2022

boucman commented Nov 7, 2022

emersion commented Nov 7, 2022

boucman commented Nov 7, 2022

emersion commented Nov 7, 2022

emersion commented Jul 4, 2023

markstos commented Nov 18, 2020 •

edited

Loading

markstos commented Nov 19, 2020 •

edited

Loading

boucman commented Nov 24, 2020 •

edited

Loading

WhyNotHugo commented Nov 7, 2022 •

edited

Loading