Finally not called -> deadlock

I have following lamba code snipplet (cut out of context):

var java.util.concurrent.locks.ReentrantLock lockLightChange  = new java.util.concurrent.locks.ReentrantLock()
......
lockLightChange passed as parameter to lambda
......

	mtgLock.lock()

    try{
		if (isBedTime.state == ON) return true
		if ((ilumination.state as Number) > ilmLimit) return true
...........
lambda code deleted
...........
		
  	} catch(Exception e) {
        logInfo("toggleLight", " Exception : " + e.getLocalizedMessage)
 	    sendCommand(light, "0")
  	}
 	finally {mtgLock.unlock()}
    return true
]

I seems that when return is called inside lambda, the “finally” is not executed.
I got a deadlock.

Set a variable to true in the lock and return the variable at the end on the lambda
Why use lock in the first place?

Uh-oh, we just had that topic here.
Long story short: don’t use lambdas with locks.

I by purpose removed the lambda code.

The pure question is :
“Is the finally block executed when returning out of a try block inside a lambda?”

this is suboptimal :slight_smile:

It that a new character in Transformers?

and the pure answer is: it isn’t guaranteed to be.

I’d suggest you completely redesign your event processing scheme so you don’t need any locks (and lambdas, eventually).
Yes, a harsh statement.
But your safest bet is to bite the bullet early rather than to dig deeper and still get hung and disappointed in the end, nonetheless having to redesign it anyway.

Lamdas and locks do not play nice. See my very long thread here:

I have since rewritten the code to not use lamdas and, while the locks still get hit and do their job, I have yet to have one give me a problem.

See “proof” in this post where it does not lock / hit the finally

What other alternatives to lambdas we have in order to structure code.
Functions and classes are not supported too.

I have defines lambdas each for:

  • switch light on/off, with timing out when light left on for 5h
  • dimm light, with timeout when left on for 5h
  • react on motion, with different timeouts

There would be a lot of code to be duplicated if rules segmented across topics (dining, living, entry, bedroom, guestroom, stdy, pantry, locker, …) or having one rules with a biiiiiig when, and a biiiiig then too.

I will think about the latter.

I could live with limitations with lambdas, though there should be reliability arround this limitations

No. That is why the recommendation is to avoid using locks in lambdas.

The finally block is also not executed when there is a break or a type exception in a lambda.

finally is pretty much useless in lambdas and therefore using a lock inside a lambda is a really dangerous idea.

I completely agree. Look at the Design Pattern postings. Most of them will provide techniques you can use that should let you avoid the need for lambdas and locks.

Alternatively, you can move to JSR223 Rules and code in Jython, JavaScript, or Groovy. Given that JSR223 Rules and the new Experimental Next-Gen Rules engine both use the same underlying Rules Engine, this would position you fairly well to adopt the NGRE once the Experimental is removed from it’s name.

NOTE: documentation of the NGRE has been started here.

But beware, JSR223 documentation quantity and quality is hit and miss at the moment, but there are efforts starting up to address that as well.

It depends on the specific problem:

Design Pattern Use
Design Pattern: Separation of Behaviors Great for centralizing cross cutting code like sending of alerts. Essentially you are creating Rules that trigger other Rules through an Item.
Design Pattern: Working with Groups in Rules Can be used to combine lots of similar Rules into a single Rule. If you only have one Rule to process all the lights, for example, you don’t need lambdas. Pay particular attention to Member of and triggeringItem which addresses your concern with a “big when”.
Design Pattern: Associated Items Great when used in combination with the previous one, provides a way to access Items that are related to each other making it so you can further consolidate many Rules into one. This addresses much of concern about the “big then”.
Design Pattern: Expire Binding Based Timers Reduces the need for Timers in some cases.

For example, here is my code that sends me an alert when any door is left open for more than an hour.

rule "Keep track of the last time a door was opened or closed"
when
  Member of gDoorSensors changed
then
  if(previousState == NULL) return;

  val name = triggeringItem.name
  val state = triggeringItem.state

  // Double check if the event makes sense with the deadbolt state
  if(name.contains("Front") || name.contains("Back")) {
    val lock = ScriptServiceUtil.getItemRegistry.getItem(name.replace("v", "a") + "_Lock").state
    if(lock == ON) {
      logWarn(logName, name + " appears to be flapping, door changed to " + state + " but deadbolt is locked! Ignoring change.")
      return;
    }
  }

  // Update the time stamp
  postUpdate(name+"_LastUpdate", now.toString)

  // Set the timer if the door is open, cancel if it is closed
  if(state == OPEN) sendCommand(name+"_Timer", "ON")
  else postUpdate(name+"_Timer", "OFF")

  // Set the message
  val msg = new StringBuilder
  msg.append(transform("MAP", "en.map", name) + " was ")
  msg.append(if(state == OPEN) "opened" else "closed")

  var alert = false
  if(vTimeOfDay.state.toString == "NIGHT" || vTimeOfDay.state.toString == "BED") {
    msg.append(" and it is night")
    alert = true
  }
  if(vPresent.state == OFF) {
    msg.append(" and no one is home")
    alert = true
  }
  msg.append("!")

  // Alert if necessary
  if(alert){
    aAlert.sendCommand(msg.toString)
  }
  // Log the message if we didn't alert
  else {
    logInfo(logName, msg.toString)
  }
end

rule "Timer expired for a door"
when
  Member of gDoorsTimers received command OFF
then
  val doorName = transform("MAP", "en.map", triggeringItem.name)

  aAlert.sendCommand(doorName + " has been open for over an hour")

  if(vTimeOfDay.state.toString == "NIGHT" || vTimeOfDay.state.toString == "BED") {
        triggeringItem.sendCommand(ON) // reschedule the timer
  }
end

This example uses pretty much all of the mentioned DPs to create Rules that handle any number of door sensors without locks, lambdas, or duplicated code. I’ve only a little bit of code that handles the special case where the door sensor represents one of the doors that has a deadbolt attached to it.

Notice the bulk of the code is building the message to send as an Alert.
Notice how there is nothing that actually directly references specific door sensor Items. I could have 1 door or a dozen doors/windows and use this same single Rule.

NOTE: lock above is Item representing the door deadbolt, not a ReentrantLock.

Thank you for the valuable input. It is great how much time you spend on helping people like me.

I will eliminate the locks at first, I used them to secure access to HashMaps. seems for me more dangerous to have a deadlock then HasMap concurrency issue.

And, true, lambdas are more the useless for this purpose. For example I have global HasMap for managing timers. These timers I supply to my 3 lambdas (on/of, dimm, motion). So if motion switches light on, other lambda can extend this timer. The problem a timers created inside a lambda is only there not null (re-entrant), in all other lambdas the HasMap has the right size, has the key, but the value is null.

I have to look for an alternative, you suggested the right ones

If you need that use ConcurrentHashMap instead of just HashMap. It will have better performance anyway.

But, along the lines of the DPs I presented above with lambdas, I think you will find that when you apply those and other DPs you will find you don’t need the HashMaps anymore either.

Lambdas, ArrayLists, HashMaps, and ReentrantLocks are all code smells to me. They are indications that you are trying to force the Rules DSL to work in a way it doesn’t support very well. Sometimes they are needed indeed. But most of the time there is a more appropriate approach in Rules DSL, often involving Items and Groups.

Note, this is one of the cases where use of a HashMap (or ConcurrentHashMap) is appropriate and not a code smell. :wink:

for HasMap:

import java.util.Map


val Map<String, Timer> lightTimers = newHashMap

What is the notation for the Concurrent one?

Try newConcurrentHashMap. If that doesn’t work use new ConcurrentHashMap().

unfortunately I am not able to find the right import for it
in the java doc it is stated to be in

java.util.concurrent

which does not work, at least VSCode complains

You need to import the full class

java.util.concurrent.ConcurrentHashMap

And this is a case where even if VSCode complains try running it and see if it works .

VSCode is fine with

val java.util.concurrent.ConcurrentHashMap<String, Timer> lightTimers = new java.util.concurrent.ConcurrentHashMap() 
val java.util.concurrent.ConcurrentHashMap<String, DateTime> lightTimersExpiration = new java.util.concurrent.ConcurrentHashMap()

btw. is there a way to query a timer instance how much time is left till trigger?
I did not find anything in the interface, so implemented a separate HashMap to track that.

No, there isn’t.