Hopefully fixed for SC in change 23048.  Assigning to Andy for SN fix.

S::U::Timers can fire timers early if time travel is detected, something I hadn't realised.  Change 23323 rejects alarms that are triggered more than 10 seconds early.  They will then be rescheduled if appropriate.

This happens in the case of an actual system time change - S::U::Timers spots the change and adjusts the timers, firing them early/late as necessary.  However, DST changes would not be picked up by S::U::Timers and so are caught by the timeChecker task that I introduced with change 23048.

Note that at no point does the alarm code cause a system time change - it simply reacts to them.

Change 23530 fixes a small bug whereby the count of scheduled alarms would be wrong if an alarm was fired immediately rather than being scheduled.

This bug just doesn't sound right. The alarm code should be constructing UTC-based date/time objects such that DST changes shouldn't cause problems unless
A) the timezone data for the underlying OS changed after the alarm was scheduled (e.g. you have an everyday 8 am alarm and don't update your OS' timezone data until after 8 am on Saturday before a DST change, at which point SC/SN has already scheduled the next alarm.)
B) You have a Boom and it loses its connection to SC/SN, since Boom's internal clock has no concept of DST changes.
C) the alarm was set for the ~2am DST change trouble zone

Max, is this really a problem for systems that have correct tz data?

Here's how I tend to deal with "next day at 8am" math:

1) get the current time()
2) use localtime() to break that into a time struct
3) change the hour component of the time struct to 12 noon (so we're guaranteed that step 5 will move us to the next day, even on "fall back" DST changes that give us 24-hour days)
4) convert that back to a time() second count (Time::Local)
5) add 86400 seconds to get to noon (or 11am, or 1pm) plus some minutes & seconds tomorrow (for day-of-week stuff, you look at the wday component of the struct from step 2 and do some simple modulo & multiplication calcs)
6) use localtime() to break that into a time struct
7) reset the hour/min/sec components of the struct from step 6 as appropriate (e.g. hour = 8, min = 0, sec = 0 for 8am)
8) convert that back to a time() value suitable for setting a timer

This looks a little complicated, but I think it beats running code every single minute! (per your code comments, "On SC, this is called every minute and handles DST and system clock changes")

But we need to handle your A and C situations as well as situations where the system time changes in any other way.

Andy, 

I think I found a very easy fix for this problem.
You'll find some analysis in the following thread:
http://forums.slimdevices.com/showthread.php?t=89292

but to make it short: I realized that in Slim/Utils/Timers.pm 
EV::timer is used to schedule any timer.

If EV::periodic is used instead, then SBS would use a much more robust timer implemention.

So I suggest changing the line 
$w = EV::timer( $when - $now, 0, sub {
in Slim/Utils/Timers.pm to
$w = EV::periodic($when, 0,0, sub {

Then the timers will adopt to:
ntdp changes (as happening to the people on the thread above)
very likely also DST changes.

From the docs of EV (http://search.cpan.org/~mlehmann/EV-4.03/EV.pm) about EV::timer

"The timer does his best not to drift, but it will not invoke the timer more often then once per event loop iteration, and might drift in other cases. If that isn't acceptable, look at EV::periodic, which can provide long-term stable timers.

The timer is based on a monotonic clock, that is, if somebody is sitting in front of the machine while the timer is running and changes the system clock, the timer will nevertheless run (roughly) the same time."

The docs for 'periodic' say:

"Similar to EV::timer, but is not based on relative timeouts but on absolute times. Apart from creating "simple" timers that trigger "at" the specified time, it can also be used for non-drifting absolute timers and more complex, cron-like, setups that are not adversely affected by time jumps (i.e. when the system clock is changed by explicit date -s or other means such as ntpd). It is also the most complex watcher type in EV."

Sounds exactly like what we want in SBS and for alarms ??

could you give a short summary of the results of your bug meeting?

Really, this fix proposed by Stefan seems too good to ignore. May I suggest it gets merged into the 7.6 trunk so that beta testers of the 7.6.2 can report if there are any side effects?

On further investigation, I discover that setTimer is called at 206 places in the 7.6.1 code, almost always to create a time delay of a second or two. These are natural candidates for EV::timer, where montonicity is desired more than absolute wall times. 

Side note: it is odd that _makeTimer {} disguises EV::timer as an absolute time mechanism by my $now = EV::now; $w = EV::timer( $when - $now, 0, sub {...}) but almost all cases the calling code wants a relative delay and uses Slim::Utils::Timers::setTimer( $client, Time::HiRes::time() + nn, \&someCode, @args ).  Net result is that we are making the nn second delay into an absolute time by adding Time::HiRes::time() in the calling function and then (approximately!) undoing the addition by subtracting EV::now when setting up the timer.  EV::now is the EV timestamp last time it checked, not the very latest system time unless you also first call EV::now_update.

It seems that, ideally, we would use EV::timer when delays relative to the current epoch are needed, and EV::periodic when we want absolute wall time (primarily alarms). This would require making some new functions like _makeAbsTimer using EV::periodic and _makeRelTimer using EV::timer and using them as appropriate. I'd keep _makeTimer as it is in order not to break the 206 invocations that expect the current functionality.

Created attachment 7450 [details]
Patch to add setAbsTimer and setRelTimer 

Use setAbsTimer when you want something to happen at a specific wall clock time (like an alarm)
Use setRelTimer when you want a delay of a few seconds relative to now, (or a snooze timer.)

Left legacy setTimer in place since much SBS code depends on it.

Created attachment 7451 [details]
refactored Alarm.pm to use new timer functions

This patch illustrates how the new setXxxTimer functions would be used in Alarm.pm. I tested this in my SBS installation and it works for me.

Hello,

I just replace
$w = EV::timer( $when - $now, 0, sub {

by
$w = EV::periodic($when, 0,0, sub {

and all is good !!

Thanks

(In reply to comment #12)
> Hello,
> 
> I just replace
> $w = EV::timer( $when - $now, 0, sub {
> 
> by
> $w = EV::periodic($when, 0,0, sub {
> 
> and all is good !!
> 
> Thanks

In the file : Timers.pm, function _makeTimer ....

Regards

I've been dealing with this for years!  Please fix it!!!!