Design Pattern: Cancel Activity

See Design Pattern: What is a Design Pattern and How Do I Use Them

Warning: The use of long Thread::sleep() times (anything more than 100 msec) like in the original version of this DP is considered an antipattern and should be avoided. Consequently this DP is deprecated. Please use Design Pattern: Looping Timers to solve this problem.

Problem Statement

Occasionally one has a long running rule that includes a while or for loop to slowly adjust an Item or Items until some criteria is reached. For example, implementing a gradually dimming of a light or flashing of a strobe light in an alarm. However, because these rules are long running one often wants to have the option of cancelling the rule before it reaches the end state.

6 Likes

Reviving this as it has been referenced today… Thread::sleep should not be used, see the issue at the smarthome issues list:

This is actually pretty frustrating. In many cases, including the above, not using Thread::sleep is vastly more complicated and potentially eliminates certain possibilities.

For example, I’m not entirely certain this design pattern can even be implemented using Timers.

For instance, the Timer only gets the context (i.e. the vars and vals) at the time that it is created. I’m not 100% positive that it will receive changes to the cancelled val which would stop the process from executing.

I think it has to do with the implementation of java scheduling in ESH! The thread sleep might be something that can get to garbage collection after a certain time frame, or it pauses the entire thread pool (I’m guessing) - never really had the time to try it. I am not sure about this, but reading some documentation makes me think that this is the case.

That is the reported behavior.

I’m a little concerned about leaving this as a won’t fix (I’ve stated as such on the issue). Even if we say that tiny sleeps are kosher, if someone has a bunch of tiny sleeps scattered throughout the code or tiny sleeps in rules that trigger lots in rapid succession (e.g. received update on a group) even these little sleeps will add up to big problems.

Rather than just ignoring the problem it seems like it should either be fixed or eliminate Thread::sleep as an option in the Rules DSL, or at a minimum we need to update the Docs (and pretty much ALL of the Design Patterns, not sure how I’m going to discover the triggering Item from a Group) to say never to use it.

1 Like

This might be indeed a huge problem later on!

I agree to the elimination of this from the docs - I think it is easier!

I do not use Thread::sleep for this! What do you mean? Are you referring to the fact that you wait for a feedback from the device?

Easier for you maybe. More than half of my Design Patterns use a Thread::sleep. I bet the majority of the users out there are currently using Thread::sleep. It is less work for the maintainers but massively more work (in aggregate) for the users.

Also, eliminating it from the docs is not sufficient. I’m not sure it is even in the current docs. It needs to be added to the docs in big bold letters:

Do Not Use Thread::sleep() in Rules.

or some other appropriately worded statement.

A breaking change announcement also needs to be made to inform the users of this change.

Finally, it would need to be added to the change log for 2.2 when it gets released.

Ideally, the Rules DSL itself would be modified to not allow Thread::sleep(), though that might be a major headache if it is even possible.

See the first example rule in the Working with Groups in Rules Design Pattern for an example.

If you are not on a super fast machine persistence takes a little bit of time to catch up so that lastUpdate returns the actual most recent update time. The amount of time required depends on the system OH is running on. Your system may be fast enough that persistence is able to keep up without a sleep.

Just fyi, although I am sure that you saw that already, this issue https://github.com/eclipse/smarthome/issues/4103#issuecomment-324871684 got re-opened.

1 Like

Ok, I get you now! And you are right!

Yes, should be stuck on the wall for a while!

Yes, it should be like that!

I agree that the work is far less for maintainers changing the code compared to the users changing their work (it is redundant as a speech), but I also think that we should move on. The thread thing is not good for the framework keeping up with sheer performance!
Just my two cents!

If one cannot use it, shouldn’t some kind of error message be generated when in code rules ?
Glad I ran into this topic.

AFAIK, the problem is deeper than the openHAB logs, so unfortunately, you do not get any clue about it! @rlkoshak is right when screaming about it! It basically pauses every single rule! I am testing it right now. I’ll keep you posted!

Keep an eye on the issue linked above. The actual problem the user ran into may be related to something else. In any case, this is a recently discovered problem, not something that was baked into OH on purpose. So there was never a need to generate an error. Until now it was thought to be OK. And it may be OK again.

And there is a difference between “cannot use” and “should not use” and in all honestly, long Thread::sleeps like above have never been officially recommended or supported.

However, short ones are supposed to be OK but I describe above that even short Thread::sleeps can lead to long Thread::sleeps in a number of circumstances.

1 Like

#1023

Will do!

Yep. The OP on the issue changed his code and is seeing something similar. It looks more like at least on his step up and is seeing that the rule triggers are only being suppressed while the System started rule is active.

It is interesting to see that for you the rules still trigger even when your System started rule is sleeping. What version are you running? If it is before OH2.1.0 stable or after OH2.2.0 snapshot build 1070 this might be an issue that is already fixed.

If not, then there must be something different between your’s and OP’s setup to account for the behavior.

Posting this on the github thread would be informative.

In the mean time I’m going to soften my warning on this design pattern.

1 Like

Seems that Discourse is wrongly caching!
The summary here:

Hi Rich,
Thanks for the design guide, I make below rules for detect “expected” thing get start,
I use while for achieve this detection, I am wonder will it be other way can do the same job and use lower CPU resources? as I use few of these rules for detect home activity and found RPI response getting slower and slower,

Thanks & Regards

rule "Turn CompuR TV Off"
when
	Time cron "0 0 1 * * ?"
then
	while(Pwr_CompuRoom_WifePC_Power.state > 65){
		Thread::sleep(60000)
		if (Pwr_CompuRoom_WifePC_Power.state < 65){
			sendCommand(WeMo, OFF)
			sendBroadcastNotification("Wife goto bed, zzZ")
			logInfo("Save-Power", "Power off Socket")}
	}
end

The way this is written in a really bad idea.

Every night at 1 am the rule triggers. If the power never goes below 65 the rule will never exit.

You should really never use a sleep in a rules larger than 300 msec. Longer sleeps can cause problems.

Why not trigger the rule when the power item changes and check the time and value of the item. Anniversary you can trigger the rule even minute and check the time in the rule. At a minimum, user timers instead of such a long sleep.

my wife get sleep between 1 to 2:30,
the power meter is on main switch, when she turn sleep, I shut down nearby electronic (from standby to no power)

I use lot of sleep thread command in my rules and most of them were few minutes, but few can up to 1-2 hours
so when sleep longer than few minutes, should I use Timer?

still the second question, how should catch a status change only during specific period which will better than my current way?

below code I use another way to achieve for long time count down (I want to wait 3 hours after first command, than 10 minutes wait for every changes made)

import java.util.concurrent.locks.ReentrantLock
var java.util.concurrent.locks.ReentrantLock tempLock  = new java.util.concurrent.locks.ReentrantLock()

rule "BedRoom AirCon tempeture control"
when
	Item Pwr_BedRoom_AirCon changed to ON
then
	var Number counter = 180
	if (Pwr_BedRoom_AirCon.state == ON) {
	Thread::sleep(5000)
	AirCon_BedRoom_Toshiba.sendCommand("Toshiba_AirCon_17C")
	logInfo("AirCon", "BedR AirCon just turn ON, Current Temp:{} Set BedRoom AirCon Temp:{}",Rpi_BedR_Tempature.state, "17°C")
//logInfo("AirCon", "**>**>>> Bed Room AC auto adjust test")
//logInfo("AirCon", "BedR-Test First ON, Current Temp:{} Set BedRoom AirCon Temp:{}",Rpi_BedR_Tempature.state, "17°C")
		if(!tempLock.isLocked){
			tempLock.lock()
			try {
            while (Pwr_BedRoom_AirCon.state == ON){
            	if (counter == 0){
					if (Ham_iphone.state==ON){
						if (((Rpi_BedR_Tempature.state >= 23) && (Rpi_BedR_Tempature.state <= 24)) && (Rpi_BedR_Tempature.state != Rpi_BedR_Tempature.previousState(true,"influxdb").state)) {logInfo("AirCon", "BedR current Tempature:{}%, Previous Tempature:{}%", Rpi_BedR_Tempature.state, Rpi_BedR_Tempature.previousState(true,"influxdb").state)}
						else if ((Rpi_BedR_Tempature.state < 23) && (Rpi_BedR_Tempature.state != Rpi_BedR_Tempature.previousState(true,"influxdb").state) && (AirCon_BedRoom_Toshiba.state.toString != "Toshiba_AirCon_22C")) {AirCon_BedRoom_Toshiba.sendCommand("Toshiba_AirCon_22C") logInfo("AirCon", "BedR current Temp:{}%, Previous Temp:{}%, Set BedRoom AirCon Temp:{}", Rpi_BedR_Tempature.state, Rpi_BedR_Tempature.previousState(true,"influxdb").state, "22°C")}
						else if ((Rpi_BedR_Tempature.state > 24) && (Rpi_BedR_Tempature.state != Rpi_BedR_Tempature.previousState(true,"influxdb").state) && (AirCon_BedRoom_Toshiba.state.toString != "Toshiba_AirCon_21C")) {AirCon_BedRoom_Toshiba.sendCommand("Toshiba_AirCon_21C") logInfo("AirCon", "BedR current Temp:{}%, Previous Temp:{}%, Set BedRoom AirCon Temp:{}", Rpi_BedR_Tempature.state, Rpi_BedR_Tempature.previousState(true,"influxdb").state, "21°C")}
						logInfo("AirCon", "BedR-Test current Temp:{}%, Previous Temp:{}%, Temp will not change until 10 Minutes later.", Rpi_BedR_Tempature.state, Rpi_BedR_Tempature.previousState().state, counter)
					}
					else if (Ham_iphone.state==OFF){
						if (((Rpi_BedR_Tempature.state >= 24) && (Rpi_BedR_Tempature.state <= 25)) && (Rpi_BedR_Tempature.state != Rpi_BedR_Tempature.previousState(true,"influxdb").state)) {logInfo("AirCon", "BedR current Tempature:{}%, Previous Tempature:{}%", Rpi_BedR_Tempature.state, Rpi_BedR_Tempature.previousState(true,"influxdb").state)}
						else if ((Rpi_BedR_Tempature.state < 24) && (Rpi_BedR_Tempature.state != Rpi_BedR_Tempature.previousState(true,"influxdb").state) && (AirCon_BedRoom_Toshiba.state.toString != "Toshiba_AirCon_22C")) {AirCon_BedRoom_Toshiba.sendCommand("Toshiba_AirCon_22C") logInfo("AirCon", "BedR current Temp:{}%, Previous Temp:{}%, Set BedRoom AirCon Temp:{}", Rpi_BedR_Tempature.state, Rpi_BedR_Tempature.previousState(true,"influxdb").state, "22°C")}
						else if ((Rpi_BedR_Tempature.state > 24.5) && (Rpi_BedR_Tempature.state != Rpi_BedR_Tempature.previousState(true,"influxdb").state) && (AirCon_BedRoom_Toshiba.state.toString != "Toshiba_AirCon_21C")) {AirCon_BedRoom_Toshiba.sendCommand("Toshiba_AirCon_21C") logInfo("AirCon", "BedR current Temp:{}%, Previous Temp:{}%, Set BedRoom AirCon Temp:{}", Rpi_BedR_Tempature.state, Rpi_BedR_Tempature.previousState(true,"influxdb").state, "21°C")}
						logInfo("AirCon", "BedR-Test current Temp:{}%, Previous Temp:{}%, Temp will not change until 10 Minutes later.", Rpi_BedR_Tempature.state, Rpi_BedR_Tempature.previousState().state, counter)
					}
				}
			if (counter == 0){counter=10}
			Thread::sleep(60000)
			counter = counter - 1
			logInfo("AirCon", "BedR-Test current Temp:{}%, Previous Temp:{}%, Temp will not change until {} Minutes later.", Rpi_BedR_Tempature.state, Rpi_BedR_Tempature.previousState().state, counter)
			}} finally  {
            tempLock.unlock()}
		}
	}
end

When you sleep longer than half a second you should use a timer. otherwise you tie up a thread that could be executing other rules sitting and doing nothing. If you have too many sleeps going on no other ruless can execute at all.

Tried the rule based on the state change. Inside the rule use an if to determine if now is a time where the state change matters. Look at my time off day dp for a way to centralize your time calculations.

Use timers, not sleeps. See the Expire Based Timers DP for a relatively painless way to implement timers.

All of these long sleeps is why your performance is poor and getting worse… you are tying up all your execution threads sleeping leaving none left to do actual work.