Bug 68758 - Should watch systemd suspend signals
Summary: Should watch systemd suspend signals
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: mission-control (show other bugs)
Version: git master
Hardware: All Linux (All)
: medium enhancement
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-08-30 15:36 UTC by Florian Jacob
Modified: 2013-09-04 16:42 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Listen to systemd sleep/shutdown signals, and inhibit until disconnected (23.50 KB, patch)
2013-09-02 14:27 UTC, Simon McVittie
Details | Splinter Review
McdConnectivityMonitor: comment how McdInhibit works (1.32 KB, patch)
2013-09-03 18:17 UTC, Simon McVittie
Details | Splinter Review
Add CONNECTIVITY_NONE, and make state transitions clearer (5.26 KB, patch)
2013-09-03 18:17 UTC, Simon McVittie
Details | Splinter Review

Description Florian Jacob 2013-08-30 15:36:25 UTC
Similar as "Should watch UPower AboutToSleep signal", https://bugs.freedesktop.org/show_bug.cgi?id=28370 , telepathy mission control should watch systemd signals / create inhibitor locks for suspend.

On my system, current Chakra, the UPower AboutToSleep Signal isn't sent anymore as it has completely transitioned to systemd. This results in telepathy not being able to log out prior to suspend, and because XEP-0198 is not implemented in telepathy, I stay online for others in my contact list until the server disconnects me according to his TCP Keepalive settings, which defaults to 4 hours on Debian systems.

During that time, all messages sent to me are getting lost without any notice for the sender's or the receiver's side.

I could reproduce this with the KDE-Version of Fedora using KTelepathy like on my own system.


Information on the way systemd handles this:
http://www.freedesktop.org/wiki/Software/systemd/logind/

Unlike UPower, Systemd doesn't just spit out a signal and waits a second and then shuts down, one can create something called inhibitor locks, which one cand hold and release when one is done with cleaning up. No general one second until everything is suspended, but exactly the time everything needs. Disadvandage is more implementation effort than just listening to a signal.

Implementation in NetworkManager:
http://code.metager.de/source/xref/freedesktop/NetworkManager/NetworkManager/src/nm-sleep-monitor-systemd.c
Comment 1 Florian Jacob 2013-08-30 15:51:14 UTC
(Bug for XEP-0198 is https://bugs.freedesktop.org/show_bug.cgi?id=46700 )
Comment 2 Simon McVittie 2013-09-02 09:11:54 UTC
(In reply to comment #0)
> Unlike UPower, Systemd doesn't just spit out a signal and waits a second and
> then shuts down, one can create something called inhibitor locks, which one
> cand hold and release when one is done with cleaning up.

This is better than the current state - we can ask all connections to disconnect, and retain the inhibitor lock for the duration - but is going to take rather more design.

(In reply to comment #1)
> (Bug for XEP-0198 is https://bugs.freedesktop.org/show_bug.cgi?id=46700 )

I suspect XEP-0198 is going to be rather harder than using inhibitor locks in MC, though; it probably makes sense to do this one first.

Patches/branches for either feature welcome.
Comment 3 Simon McVittie 2013-09-02 14:27:47 UTC
Created attachment 85063 [details] [review]
Listen to systemd sleep/shutdown signals, and inhibit until  disconnected

---

This was easier than I thought. fd-passing in GDBus is a bit of a pain, but everything else is straightforward.
Comment 4 Guillaume Desmottes 2013-09-03 10:09:59 UTC
Comment on attachment 85063 [details] [review]
Listen to systemd sleep/shutdown signals, and inhibit until  disconnected

Review of attachment 85063 [details] [review]:
-----------------------------------------------------------------

I guess systemd doesn't provide a client side helper lib for this?

::: src/connectivity-monitor.c
@@ +50,5 @@
> +#define LOGIN1_MANAGER_PREPARE_FOR_SHUTDOWN "PrepareForShutdown"
> +#define LOGIN1_MANAGER_INHIBIT "Inhibit"
> +
> +struct _McdInhibit {
> +    gsize holds;

Why are you using a gsize here? Also maybe document the semantic of 'holds'?

@@ +358,5 @@
> +
> +      if (sleeping)
> +        {
> +          DEBUG ("about to suspend");
> +          connectivity_monitor_change_states (self, 0, CONNECTIVITY_AWAKE,

Could be clearer to have a CONNECTIVITY_NONE or something rather than 0.
Comment 5 Simon McVittie 2013-09-03 15:04:59 UTC
(In reply to comment #4)
> Why are you using a gsize here?

"It's like a refcount"

> Also maybe document the semantic of 'holds'?

Sure.

> Could be clearer to have a CONNECTIVITY_NONE or something rather than 0.

I suppose so, it'd make "this is a bitfield" explicit in the call (but it'd also make the lines longer).
Comment 6 Simon McVittie 2013-09-03 18:17:08 UTC
Created attachment 85136 [details] [review]
McdConnectivityMonitor: comment how McdInhibit works
Comment 7 Simon McVittie 2013-09-03 18:17:31 UTC
Created attachment 85137 [details] [review]
Add CONNECTIVITY_NONE, and make state transitions  clearer

---

Hopefully those two patches address your feedback?
Comment 8 Guillaume Desmottes 2013-09-04 07:24:17 UTC
Comment on attachment 85136 [details] [review]
McdConnectivityMonitor: comment how McdInhibit works

Review of attachment 85136 [details] [review]:
-----------------------------------------------------------------

++
Comment 9 Guillaume Desmottes 2013-09-04 07:25:23 UTC
Comment on attachment 85137 [details] [review]
Add CONNECTIVITY_NONE, and make state transitions  clearer

Review of attachment 85137 [details] [review]:
-----------------------------------------------------------------

++
Comment 10 Guillaume Desmottes 2013-09-04 07:25:37 UTC
(In reply to comment #7)
> Hopefully those two patches address your feedback?

Yep, go for it!
Comment 11 Simon McVittie 2013-09-04 12:56:16 UTC
Fixed in git master for 5.15.1.
Comment 12 Florian Jacob 2013-09-04 16:42:59 UTC
Thank you very very much for having this fixed in just 5 days after reporting, you're great! :)

And I guess "smcv" who helped me on irc to to find out what the actual problem was with the DBUS signals stands for "Simon McVittie", so thanks once again.

Can't wait for the next version of telepathy to make it into my distro's repositories! No more message losses on suspend, yay! :D


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.