Clock is incorrent after suspend

[WGH-]

After computer is resumed from suspend, the time in the bar is often incorrect. Depending on luck, it might show the time just before the suspend for up to one minute.

This's likely caused by the fact that the timer uses scheduler that doesn't advance during sleep. E.g. if you suspend the machine at 18:03:01, and resume at 20:03:59, the clock won't update for about a minute.

This can be fixed by using Linux-specific timerfd: you can arm the timer to be triggered every minute at exactly :00 seconds of real-time clock, and you will only have to integrate the file descriptor into the event loop. If you skip the event (due to suspend as well), it will trigger immediately, and the kernel will even tell how many expirations you have skipped (this's not really relevant to this use case, but still pretty cool).

I used this in my own toy event-driven i3status-replacement, and it worked pretty well: https://github.com/WGH-/myi3status/blob/c738163286ccde1f31bc0cfefbd78141412f3f4b/src/utils.cpp#L11-L30 https://github.com/WGH-/myi3status/blob/1cc1448c25179f4437b14ad447667f57294ed76a/src/widget_time.cpp#L23-L28

If this idea sounds good, I can try to implement a PoC for barista.

[soumya92]

The real fix would be to change

barista/timing/scheduler.go

Line 130 in a29f39a

s.timer = time.AfterFunc(when.Sub(Now()), s.maybeTrigger)

so that it actually triggers at the time being passed in. That will fix not only the clock but timed outputs in general.

Unfortunately it looks like go doesn't offer a way to trigger a function at a specific time, probably because of the complexity of changing clocks and time zones.

I suspect trying to use timerfd in that package will be difficult, but it's definitely worth attempting.

On the other hand, the Resume function is also called whenever the bar is re-displayed, so maybe all we need to do is trigger all schedulers in Resume that would have triggered after the call to Pause. That functionality already works for the non-suspended case (but only because go's timer.AfterFunc works), but maybe it's not too complicated to extend that to handle suspend as well?

[soumya92]

I wonder if maybe the implementation of test mode should really be the implementation for prod as well. It has support for advancing to local times, which would handle Resume, and it has support for computing the next scheduler event, which could be used with time.AfterFunc to handle the regular case...

(edit: Probably not, lock contention will be too high. It would need to be heavily modified to provide decent performance, and is probably not worth the effort)

[WGH-]

I suspect trying to use timerfd in that package will be difficult, but it's definitely worth attempting.

I think naive way with every Scheduler having its own timerfd is very easy to implement. It can also trivially support both "After"/"At" and "Every XX:00" APIs.

The harder and, perhaps, more efficient way would be to have a single timerfd, and maintain heap of pending Schedulers, basically reimplementing what Go runtime scheduler does.

See also related discussions: golang/go#35012 golang/go#24595

[WGH-]

The harder and, perhaps, more efficient way would be to have a single timerfd, and maintain heap of pending Schedulers, basically reimplementing what Go runtime scheduler does.

Huh, apparently, that's exactly what "test mode" timer does (minus timerfd part).

[soumya92]

Huh, so there are already projects out there patching golang itself to fix this issue? 😞

I think naive way with every Scheduler having its own timerfd is very easy to implement. It can also trivially support both "After"/"At" and "Every XX:00" APIs.

Hah. I totally didn't think of that. For some reason I was stuck in the mindset of using only one FD. Yeah, that sounds very straightforward, and the existing Scheduler APIs look like they map 1:1 to timerfd_settime calls, so if you want to attempt it, go for it :)

[WGH-]

I think that Scheduler should be flexible with regard to which clock to use. Clock widgets should certainly use CLOCK_REALTIME with TFD_TIMER_CANCEL_ON_SET (the latter flag apparently can be used for notification about any discontinuous change, which is pretty convenient). Most other widgets should just use CLOCK_BOOTTIME.

[soumya92]

I'm not convinced we need REALTIME anywhere, and using one clock will be simpler.

The majority of Schedulers are used to control how often a module polls for data. If a module is set to poll every 15 minutes, but the system was asleep for 14 of those minutes, we should still poll when the system resumes. So I think BOOTTIME is fine everywhere.

If a usecase for REALTIME shows up later, then this can be revisited, but currently I don't think any of the Schedulers in use would be negatively impacted by switching to BOOTTIME.

[WGH-]

Widgets that monitor something that can only change when the system is running (e.g. network traffic, disk/memory space usage, etc.) should probably continue to use CLOCK_MONOTONIC. Though if polling interval is low enough, it would be practically indistinguishable from CLOCK_BOOTTIME. I honestly don't think it matters much, though.

I'm not convinced we need REALTIME anywhere, and using one clock will be simpler.

Having convenient features to detect all kinds of clock jumps doesn't warrant it, you think?

[soumya92]

What I meant was if we just switch Scheduler to use BOOTTIME everywhere, then we don't need to come up with a new API that requires callers to specify what kind of clock.

Certainly as a first pass, that should be sufficient. I'm not sure we need to detect clock jumps if we can update the output correctly, but I'm also not opposed to adding a new method that would provide a REALTIME scheduler.

If you think REALTIME is useful, then adding timing.NewRealtimeScheduler() sounds fine to me.

[WGH-]

There's also a case unrelated to suspend time skips. User or NTP may forcibly change the current time, and CLOCK_REALTIME will catch this correctly as well.

[soumya92]

User or NTP may forcibly change the current time, and CLOCK_REALTIME will catch this correctly as well.

Could you elaborate? My understanding is that REALTIME will prevent the Scheduler from taking too long to trigger. But it won't immediately trigger the Scheduler when the time changes.

Support for detecting local time changes could be added to the localtz watcher (and extended to be a localtime watcher). Then the clock could update as soon as the time changes (which would be nice)

[WGH-]

I think you're perhaps misunderstanding what CLOCK_REALTIME is. It's the "wall clock time" that can be changed with date and other commands.

Anyway, here's the demo: https://github.com/WGH-/go-timerfd-test

When system is suspended, both timers get it right: they immediately trigger after resuming.

2020/05/11 14:25:00 CLOCK_REALTIME: expirations=1
2020/05/11 14:25:00 CLOCK_BOOTTIME: expirations=1
(suspended)
2020/05/11 14:28:33 CLOCK_BOOTTIME: expirations=1
2020/05/11 14:28:33 CLOCK_REALTIME: cancelled due to discontinious change
2020/05/11 14:28:33 CLOCK_REALTIME: expirations=26486608

However, when you change the real time manually, CLOCK_BOOTTIME never notices it, and also drifts.

2020/05/11 12:05:23 CLOCK_REALTIME: expirations=26486645
2020/05/11 12:06:00 CLOCK_BOOTTIME: expirations=1
2020/05/11 12:06:00 CLOCK_REALTIME: expirations=1
(timedatectl set-time 13:37)
2020/05/11 13:37:00 CLOCK_REALTIME: cancelled due to discontinious change
2020/05/11 13:37:00 CLOCK_REALTIME: expirations=26486737
2020/05/11 13:37:33 CLOCK_BOOTTIME: expirations=1
2020/05/11 13:38:00 CLOCK_BOOTTIME: expirations=1
2020/05/11 13:38:00 CLOCK_REALTIME: expirations=1