This didn’t seem right to file as an issue but I can do is if that seems best.
Why are we completely getting rid of createTimer? setTimeout is not equivalent. We can’t set a name. We have to do additional calculations to schedule one based on an Instant, which is the way timers are scheduled 99% of the time.
I’ve no problem with setTimeout being there. I’ve no worries about it even being the preferred approach or even removing createTimer from the docs even or even just putting up flashing “don’t use this!” warnings all over it.
I do have concerns with:
making a breaking change without providing an equivalent
completely removing access to a core OH API without an equivalent and/or more complex approach
putting roadblocks in the way for people to migrate from Rules DSL to JS Scripting without a good reason (maybe there is one but it’s not obvious)
It’s also a pretty unhappy surprise to get literally hundreds of these deprecation warnings in my logs all of a sudden (about a couple dozen every 30 seconds). Really makes my logs hard to use.
The discussion on this PR lead to a reimplementation of setTimeout and setInterval in the Java layer of the addon to make them conform standard JS (in the past they returned an openHAB timer, now they return a timeoutId/intervalId as defined in the Web APIs).
Timers created by those polyfills are also automatically cancelled when a script file is unloaded.
Reasons to remove access to createTimer from openhab-js
Timers from createTimer are not threadsafe (whilst it is technically possible to make it threadsafe, it is complex and not nice to implement: we don‘t want to change the createTimer method in core, so the only option is to implement it in the add-on. We would need to copy the functionality (by copying code!!) of the core method, modify it to make it threadsafe and then overwrite the core method in some way).
Timers created by createTimer have to be managed manually; if not, the user could end up with unmangaged, scheduled jobs running endless (until openHAB is restarted).
FYI: Access to the core OH APIs can not be really removed, it is still possible to import the ScriptExecution class from core and use createTimer from there. But it is possible to remove the access to this from the openhab-js library, or as in the current case, log a warning.
I am happy to discuss this decision (which on the most part was my decision).
I can imagine to change the logged warning to something that sounds not that dramatic (instead of the deprecation warning, log that the usage of createTimer is not recommended due to possible failure of timers).
But I would like to warn the users in the logs to not use this API — it is better to warn the users with some annoying log messages than having unreliable home automation due to failing timers.
An additional option is to improve the docs how to convert an Instant to milliseconds.
I removed the createTimer method from all docs (the latest docs state that it is not recommended to use it as this can lead to problems).
My big problem with the warning is I’ve a lot of timers in my rules. they probably number in hundreds and I’ve a bunch of monitoring rules that use a timer to see when stuff goes offline or a door is left open and the like.
I’m getting at least three or four of the deprecation warnings per minute in my log, obliterating my ability to follow what else is going on.
I think I can control that logger independently but most of the package name is cut off. I’m guessing it’s org.openhab.addon.script.ui.<ruleuid> but am not sure.
I think the crux of my problem is by sticking strictly to a JavaScript setTimeout we lose a ton of features that I and I’m sure a lot of others depend upon from OH Timers.
There’s no equivalent to isActive
There’s no equivalent to isCancelled
There’s no equivalent to isRunning
There’s no equivalent to hasTerminated
There’s no equivalent to reschedule
You can’t name them so it’s once again hard to figure out which rule or which part of a rule a timer is failing from
setTimeout only takes milliseconds for when to expire but ZonedDateTime likes nanoseconds instead making some operations awkward
setTimeout only takes milliseconds and createTimer only takes Instants meaning every instance of existing code that creates a timer using createTimer has to be expanded to convert an Instant to milliseconds from now.
It’s not clear what setTimeout does when passed a negative milliseconds (error? run immediately?) (createTimer runs immediately).
setTimeout is actually quite primitive and far from being a drop in replacement for createTimer. consequently, anyone relying on anything from the above list is going to have to radically rework their code. And without hasTerminated and rescheduled some use cases currently supported will become impossible.
This is a huge breaking change and in a lot of ways a step backwards for a lot of users (including me) who will be faced with needing to move to another language in order to implement certain use cases at all. Especially without reschedule and hasTerminated I don’t see how I can achieve the same things without radically changing my entire approach. It’d be easier to go back to Rules DSL or move to JRuby. That’s not the sort of dilemma we want to impose on our users.
I agree that the Multithreaded issue needs to be solved, but this really breaks a lot of stuff.
This could also be considered a feature since the ability to manage the timers lets you know when they are running and reschedule them.
Is there an npm library that maybe implements some of this sort of management around timers we could use? If not, maybe we can implement something ourselves so we can at least see if a timer has terminated and reschedule it.
Without something to ease the transition, we are basically going to force all our users to have to rewrite a lot of rules code without giving them the tools they need to do so easily and successfully.
This shouldn’t make much sense to be available, because it is not possible to run JS code from the same context that the timer was created due to the synchronization mechanism that is required to avoid multi-thread issues. I can only imagine one case where it is possible to use it: when managing timers shared with the cache from another script.
You can’t name them so it’s once again hard to figure out which rule or which part of a rule a timer is failing from
Yes, you can’t manually name them. They are automatically named with the UI scripts UID/filename + timeout / interval + the id.
setTimeout only takes milliseconds for when to expire but ZonedDateTime likes nanoseconds instead making some operations awkward
The job is scheduled by multiplying the milliseconds with the right amount to create a ZonedDateTime.now().plusNanos().
setTimeout only takes milliseconds and createTimer only takes Instants meaning every instance of existing code that creates a timer using createTimer has to be expanded to convert an Instant to milliseconds from now.
Yes, that’s a good amount of work.
It’s not clear what setTimeout does when passed a negative milliseconds (error? run immediately?) (createTimer runs immediately).
It runs immediately (I’ve verified this)
Mozillas WebAPI doc about the delay parameter:
The time, in milliseconds that the timer should wait before the specified function or code is executed. If this parameter is omitted, a value of 0 is used, meaning execute “immediately”, or more accurately, the next event cycle.
That is a super strong argument from your side.
What is your opinion about the methods that ScheduledCompletableFuture (openHAB Core 4.2.0-SNAPSHOT API) provides?
The problem with the timer is, that the implementation is internal to core and I heard that originally it was not the plan that anything other then Rules DSL does access ScriptExecution.createTimer. @jpg0 was not so happy with reimplementing the Timer interface in the addon.
I am not aware of any.
I agree on your opinion that we need something to ease migration from createTimer, but the question is what?
The problem we always have is, how much functionality do we want to implement on top of the standard JS APIs and the openHAB Core APIs?
Make createTimer threadsafe by completely reimplementing it in the add-on. I had this implemented for a short time, but removed this complexitivity (@jpg0 also found it overly complex) when I reimplemented the polyfills. Works, but a clean solution?
Introduce some new methods for hasTerminated and reschedule, but this would require add-on changes as we would need access to the ScheduledCompletableFuture returned by the scheduler (that is also used by createTimer).
I would select solution 2, but still recommend to use setTimeout and setInterval.
Fair enough, though It think you mean isRunning. isActive returns true if the Timer is scheduled to run in the future (i.e. hasn’t been cancelled). isRunning means the lambda is actively running.
Right, but in the rule we have a ZonedDateTime. ZonedDateTimes like nanos. So in those cases where we need to, for example, manipulate epoch, we’ll have to convert those to millis first.
It’s not the biggest of deals, but it is a place where things do not line up.
There’s not a lot there. Assuming I understand what the two methods offer, at least one can see if it’s maybe going to run by looking at the scheduled time. We can’t tell if it’s been cancelled. We can’t really tell if it’s already run (can we assume if the time is in the past that it ran?) I don’t see an obvious way to reschedule it.
That would mean that timers could not be created from any other language and that seems unreasonable. I’m not sure why the correct solution wouldn’t be to change it to use Java lambdas instead, or runnable or what ever would be appropriate.
What converts the JS function passed to createTimer now to the Xtend Procedure?
I don’t have insight into the code that implements all of this so my opinion doesn’t count for much.
But it seems unreasonable to me that the createTimer action requires Xtend Procedures in the first place. So I might start there. It’s already been discussed that Rules DSL might become a separate add-on, what happens to createTimer then?
If createTimer used something Java native, would that make 1 more feasible? It seems like it’ll need to be done at some point anyway, why not now (or put it on the list for OH 4)?
If that can’t be done, I don’t know if I like either 2 nor 3.
Keep in mind, I’m not married to the current Timer syntax. If we created a Timer manager class in openhab-js that did the book keeping I’d be happy. What I’m mostly concerned about though is the use cases. As written right now, we eliminate whole classes of use cases that are currently solved by timers with nothing to replace them.
So even if we just created a class that gets instantiated and some wrappers to keep track of the timers as they are created, cancelled, and run, as long as we can do all those things Timers currently do that’s be OK. It doesn’t have to be done on the Timers themselves.
For example, let’s borrow the name/concept of my timerMgr. If we create the timer through the manager and keep track of what’s going on, perhaps wrapping the passed in function so we know when it’s been called then we can:
isActive → timourMgr.isActive(timeoutId) // looks to see if the timeout exists, it’s not cancelled, and it’s scheduled for the future
isCancelled → timeoutMgr.isCancelled(timeoutId) // do we need to expand/wrap clearTimeout() or only make that available through the manager
isRunning → I agree, this doesn’t make sense
hasTerminated → timeoutMgr.hasTerminated(timeoutId) // inverse of isActive (maybe we don’t need both?)
reschedule → timeoutMgr.reschedule(timeoutId)` // cancel existing and create new using the already passed in function, it’ll have a new id so I guess return that and update book keeping
setTimeout only takes milliseconds → since we are creating a manager to create the timeout, we could accept anything supported by time.toZDT() and then calculate the msec from that transparent to the end user
It’ll be tedious book keeping code but I think I can borrow from my existing timerMgr for a lot of it and it has the following advantages:
you can keep setTimeout and clearTimeout and all that you’ve implemented in the add-on unchanged
those who want a pure JavaScript approach to timers are supported, they can call setTimeout and clearTimeout and avoid the manager
we provide a higher level interface to users coming from other languages which, while different from OH timers, at least provides a relatively straight forward mapping making transition easier without needing to radically changing their overall approach
all the use cases I can think of off the top of my head are supported.
not only can we support Instants, we can support all sorts of ways to schedule the timer making for shorter and code that’s easier for the humans to read.
I’ve been pondering this most of the day. For better or worse, this is the best I’ve come up with.
Timers can be used by any language by access the methods from core, but it is not possible to modify or overwrite the createTimer method due to its usage of xtext. I am not sure if xtext is required, it is possible to implement the functionality without using xtext, but there is some difference in the behaviour when using a Java Runnable or Callable instead of an xtext Procedure.
Nothing converts the JS functions passed. The problem of the Procedure is not that we couldn‘t use it from the JS runtime, but to make createTimer threadsafe we need to wrap the callback in the Java layer of add-on, and this is the point where we have the problem with xtext.
Yes, as already said, I don‘t think that it requires an Xtend/Xtext Procedure, instead a Java Runnable or Callable should also work. But changing that could possibly change the behaviour of create timer regarding variables passed, and therefore a core change there could break Rules DSL as well as other engines.
You are talking about the next problem: when Rules DSL becomes its own addon at some time, we don’t have access to the actions anymore (as long as they don’t stay in core), and therefore createTimer is gone as well.
Yes, I think so. It is very likely that it would make it possible.
The overall problem with the createTimer from core is, that it is from core and we have very limited control over it in the addon and it is part of Rules DSL (at least seems to be for me), and the future of Rules DSL is unclear. If it becomes an addon, we don’t have access to its methods anymore.
I agree on your opinion — I think the best solution is to implement something new to manage timers, it should be easy to use, work with JS and provide the functionality needed for advanced users.
Even though this is a larger addition on top of standard JS and the openHAB core APIs, I agree that we need something like that. And to be fair, for the toZDT method of time we also added some stuff in the helper library that provides additional functionality.
I’ve seen your proposal of such a manager, I agree on the functionality that is required.
I’d like to have the following implemented:
Support for retrieving additional information like isCancelled etc. for timers created by setTimeout and setInterval
A builder like API to create timers (for advanced users), the question here is: do we want automatically manage the jobs created by this API, leave it to the user or let the user decide what to do?
Both changes require some changes in the addon, but I can contribute there as well.
I suppose it means what you mean by automatically manage. Anything that lets the user decide is probably a good thing I would say. But anything we can do to take the burden and complexity off of the end users from common use cases and complexity is good too.
IMO the actions should be re-implemented in plain Java and removed from DSL. I guess this was just ported from the OH1 rule engine which did not support anything else.
I don‘t think it‘s very complicated but time may be too short for extensive testing before the release. I can try to check that this weekend, but no promise.
If am not completely wrong it does also break everything in the blockly timers, does it not? If that was true I would also put a veto on that because it would break hundreds of timer rules out there.
Would be great if you could really have a look and reimplement the actions (or as a beginning, at least the ScriptExecution actions) in plain Java.
Then we should be able to wrap the callback in the addon.
I didn’t want to go through installing the library and editing it so I just added the following to the openhab.log file appender in log4j2.xml.
<!-- Rolling file appender -->
<RollingFile fileName="${sys:openhab.logdir}/openhab.log" filePattern="${sys:openhab.logdir}/openhab.log.%i.gz" name="LOGFILE">
<!-- New Element -->
<RegexFilter regex=".*actions.ScriptExecution.createTimer.*" onMatch="DENY" onMismatch="NEUTRAL"/>
<!-- End New Element -->
<PatternLayout pattern="%d{yyyy-MM-dd HH:mm:ss.SSS} [%-5.5p] [%-36.36c] - %m%n"/>
The deprecation warning log will still appear in the karaf console. When all this gets resolved, it’ll be easy enough to remove that one line in the logging config.
Posting here in case there are others who need this temporary work around.
NOTE: Yes, I’m aware that I’ve argued against the use of log filters like this in the past but there are differences in those cases and this one, and I’m not advocating here to make it permanent. In short, I still stand but those past positions.
I hope to open a PR tomorrow or on Saturday, but note that it requires on core so first the core PR needs to be merged.
The PR again requires some changes on the openhab-js library, but it should be possible to publish those changes before the addon is updated as I need to keep backward compatibility, so you can soon get rid of that deprecation warning without modifying logging settings.
Update: We have just published a new version of the openHAB JavaScript library to npm.
The deprecation warning for createTimer has been removed as this API is no longer deprecated, but createTimerWithArgument will stay deprecated.
The new release is backward compatible, but the timers created by createTimer are only thread-safe when the addon PR is merged and you have the latest version of the addon.
