Eliminating lingering timers

Correct.

Yep, correct again. Either it gets rescheduled by itself every 19:th minutes or it gets rescheduled when there’s a new sensor reading.

Well, in my opinion Timers should never be garbage collected, that kinda forfeits their purpose. You should be able to trust them actually being executed. You think the info I’ve put in this thread is enough to put in a bug ticket or is there anything more I should mention?

Well spotted. Actually, the timing isn’t important at all, so I guess I’ll simply change to using this model instead. It will generate a touch of extra network traffic, but I guess it’s worth it :slight_smile:

I think the info in this thread is enough. In particularly put in your code and fully describe the behavior you are after (i.e. reschedule a Timer over and over forever) and that it disappears. You can provide a link to this thread in the issue as well.

I will say though that the issue most likely ultimately lies in a third party library that is used to schedule the Timer and not something that ESH will be able to fix unilaterally. I could be wrong, that is just a gut feel.

I do agree, the Timer should never be garbage collected and I’m not certain that is what is actually going on.

My second approach above “the slightly less kludgey” one will give you exactly the same network traffic as your Timer example with only the addition of a single new Item. And it doesn’t use Expire binding so you can do everything inside of PaperUI. It is slightly more complex but reproduces your original code’s behavior exactly, assuming you care about the log statement when the timer goes off and the reduction in network traffic.

For completeness, here is what the code would look like with the Expire binding:

Switch Utetemperature_Timer {expire="19m,command=OFF"}
rule "Utetemperatur changed"
when
    Item Utetemperatur changed or
    Item Utetemperature_Timer received command OFF
then
    sendHttpGetRequest("http://www.temperatur.nu/rapportera.php?hash=supersecret&t=" + Utetemperatur.state)
    logDebug("temprule", "Sending " + Utetemperatur.state + " to temperatur.nu")

    if(Utetemperature_Timer.state == OFF) 
        logDebug("temprule", "Resending " + Utetemperatur.state + " to temperatur.nu and rescheduling timer")

    Utetemperature_Timer.sendCommand(ON)
end

With the Expire binding, we get the exact same behavior as your original with one additional Item and a reduction from 19 lines of code to 12 (including the line for the new Item), more than a 33% reduction in lines and code with fewer branches and nesting, and no duplication.

This is why I really really like the Expire binding.

However, it is impossible to compete with the simplicity of the corn triggered rule. :slight_smile:

2 Likes

Posted an ESH ticket: https://github.com/eclipse/smarthome/issues/4208

Totally agree with @rlkoshak on the utility of the EXPIRE binding. It takes a bit of “mental conversion” at first, but, in essence it lets you “abstract out” timers as just another kind of item. Write some rules when that Item changes to OFF and you are off to the races. Much cleaner than maintaining “routine” timers in-line in your rules.

After some more testing and logging I have come to the conclusion that is simply isn’t possible rescheduling a Timer that has already been executed, they won’t ever execute again. Maybe it’s simply a fact that a Timer only executes once? I now have some test rules with one Timer that does the following:

var Timer timer_1
var Timer timer_2

rule "reschedule"
when
        Time cron "0 */5 * * * ?"
then
        if(timer_1 == null) {
                logDebug("timertest", "Creating timer 1")
                timer_1 = createTimer(now.plusMinutes(2)) [|
                        logDebug("timertest", "Timer 1 executing")
                ]
        } else {
                logDebug("timertest", "Rescheduling timer 1")
                timer_1.reschedule(now.plusMinutes(2))
        }
end

rule "recreate"
when
        Time cron "0 */5 * * * ?"
then
        if(timer_2 == null) {
                logDebug("timertest", "Creating timer 2")
                timer_2 = createTimer(now.plusMinutes(2)) [|
                        logDebug("timertest", "Timer 2 executing")
                ]
        } else {
                logDebug("timertest", "Recreating timer 2")
                timer_2.cancel
                timer_2 = createTimer(now.plusMinutes(2)) [|
                        logDebug("timertest", "Timer 2 executing")
                ]
        }
end

…and it turns out that after timer_1 has been executed once it never executes again. However timer_2 which I repeatedly cancel and recreate works exactly as intended. So obviously rescheduling a Timer is not the same thing as cancelling it and recreating it exactly the same. I guess maybe this isn’t a bug but intended behaviour…

edit: Btw, @rlkoshak, I owe you an apology for not listening to you. I finally got very tired of using Timers so now I’ve replaced my temperature Items with file based Items using Expire. So now my rules are a heck load shorter. I’ve declared them like this now:

Number Utetemp_uteplatsen "Temperatur uteplatsen [%.1f °C]" {channel="zwave:device:6843488a:node7:sensor_temperature", expire="15m,NULL"}

And in the rule I’m just doing my notification stuff if the Item changes to NULL. Works like a charm :blush:

I wish you had asked. I had already run those tests. Indeed. Once a timer triggers you must recreate it. You can only reschedule a timer that has not yet started running.

That is correct. All reschedule does is change the time that a currently scheduled timer will trigger. Once a timer triggers and runs it no longer exists as far as the Timer scheduler is concerned. A lot of people write their code to never reschedule a timer and always cancel and recreate for this very reason.

That is how the library they use to implement the Timers was implemented. I don’t know if it is designed that way or it just happened to be implemented that way.

Glad it’s working well for you now!

I’m somewhat obsessive about making my rules very short but still easy to read and understand by the human so when the Expire binding was released I immediately jumped on it as a means to further shorten my code. It does help a whole lot in in doing that, though it does take a little bit of a change in thinking since the timer body is no longer inline with the rest of the rule.

It was there in the code I posted. I had a reschedule inside the trigger’s body, which I now understand will never be a good idea. Guess this explains all the problems I’ve been having with timers :slight_smile:

Ah yes, I do remember seeing that. I meant to talk about that in one of my replies above. I guess I forgot to mention it. Sorry I made you waste a bunch of time.

Nah, you’ve been of a great help. I’m learning a lot here :blush:

Just reviving this old topic… according to the class documentation on Timer.java (see below), and the code in the reschedule method of TimerImpl.java, there should be no reason why you can’t reschedule a timer, whether or not it has already executed. If you follow the TimerImpl link you will see from the code that the intention seems to be that the timer gets reconfigured to fire again (it explicitly resets cancelled and terminated to false, as well as rescheduling it with the Scheduler service).

I’ve created Issue 6456 which contains my test rule and the logs from a couple of experiments.

One interesting thing that I found from my experiments was that I misunderstood what Timer#isRunning() is for. It returns true while the body of the timer is actually running. I initially thought it would be true while it was still scheduled to run in the future (i.e. a kind of inverse of hasTerminated()). The way it actually works is potentially a lot more useful - not quite thought it through yet but in some circumstances it could be useful to know if the payload of a timer is currently executing.

    /**
     * Reschedules a timer to a new starting time.
     * This can also be called after a timer has terminated, which will result in another
     * execution of the same code.
     * 
     * @param newTime the new time to execute the code
     * @return true, if the rescheduling was done successful
     */
    public boolean reschedule(AbstractInstant newTime);

That isn’t the problem described above. The problem was that when OP reloaded his .rules files, the variables that held the Timer objects were reset but the Timers continued to exist. So there was no longer any access to the Timers in the Rules to call the method to reschedule it.

Sure, I get that and completely agree. I was responding specifically to this:

Once a timer triggers you must recreate it. You can only reschedule a timer that has not yet started running.

and:

Once a timer triggers and runs it no longer exists as far as the Timer scheduler is concerned.

I should have quoted those sections… apologies. I am in the process of setting up a build environment so I can look into this bug.

Which demonstrates the danger of resurrecting year old threads. This has been long since fixed.

If it isn’t working for you then it is a newly introduced bug. At least as of OH 2.4 #1405 the following code works:

rule "Test string"
when
  Item Test changed
then
  logInfo("Test", "Test changed to \""+Test.state.toString+"\"")

  var count = 0
  timer = createTimer(now, [ |
      logInfo("test", "Looping timer with " + count)
      if(count < 10) {
        count = count + 1
        timer.reschedule(now.plusMillis(500))
      }
      else {
        timer = null
      }
  ])
end

Of course hasTerminated probably doesn’t get set to false until after the lambda exits.

But in the future, it would cause less confusion and receive more attention if you post a new thread and if necessary link to this thread then resurrecting a really old thread that contains information that may no longer be relevant.

Noted. Sorry.

Also, your test code shows reschedule() being called from within the “body” of the timer. This works fine, I’ve also tested it in a similar way to your test. The JavaDoc for the reschedule() action definitely says that you can call reschedule() even after a timer has terminated, which is not the case and was the basis of my bug report.