ReentrantLock - correct use or not?

Setup, Configuration and Use Scripts & Rules
1

folks,

When using a lambda, are variables passed to it a reference or a copy?
I have the following code - which works - but wondering if it’s wrong.

import java.util.concurrent.locks.ReentrantLock
import org.eclipse.xtext.xbase.lib.Functions
import java.util.concurrent.TimeUnit

val ReentrantLock fronRoomBlindLock = new ReentrantLock


val Functions$Function3 setPosition = [
	GenericItem 	itemBlind,
        String          mac,
        ReentrantLock   fronRoomBlindLock
	| 
                if (fronRoomBlindLock.getHoldCount() > 0){logWarn("Blinds - Front room", "setPosition - Waiting on other locks. Count:" + fronRoomBlindLock.getHoldCount().toString)}
                try{
                        fronRoomBlindLock.tryLock(5, TimeUnit.SECONDS)
                        val req = itemBlind.state

                        sendHttpGetRequest("http://rpiblinds01:8080/setposition/" + mac + "/" + req, 1000)
                }
                catch(Throwable t) { 
                        logError("Blinds - setPosition", t.getMessage())
                }
                finally {
                        fronRoomBlindLock.unlock()
                }        
	]

val Functions$Function3 getPosition = [
	GenericItem 	itemBlind,
        String          mac,
        ReentrantLock   fronRoomBlindLock
	| 
                try{
                        if (fronRoomBlindLock.getHoldCount() > 0){logWarn("Blinds - Front room", "getPosition - Waiting on other locks. Count:" + fronRoomBlindLock.getHoldCount().toString)}
                        fronRoomBlindLock.tryLock(5, TimeUnit.SECONDS)

                        var String value = sendHttpGetRequest("http://rpiblinds01:8080/getposition/" + mac, 1000)
                        val int pos =  Integer.parseInt(transform("JS", "blindsExtractValue.js", value))

                        if (pos < 0){
                                logWarn("Blinds - Check Position", "Position for blind "  + itemBlind.name + " reports less than zero")
                                return;
                        }                                          


                        var iPOS = itemBlind.name.toString + "_Position"

                        sendCommand(itemBlind.name + "_Position", pos)
                        postUpdate(itemBlind.name + "_PC", pos)

                }
                catch(Throwable t) { 
                        logError("Blinds - getPosition", t.getMessage())
                }
                finally {
                        fronRoomBlindLock.unlock()
                }        
	]


rule "Blinds - Front Room - Right - PC" 
when 
        Item FrontRoomBlindsRight_PC        received command 
then
        setPosition.apply(FrontRoomBlindsRight_PC, "C1:03:74:91:05:6C", fronRoomBlindLock) 
end

rule "Check Blind Position"
when
        Time cron "0/30 * * ? * * *"
then
//  timer = Timer.createTimer(now.plusSeconds(3) [|
//         sendCommand(EnergiseGD, OFF)
//         timer = null   // reset the timer
//     ])        

        getPosition.apply(FrontRoomBlindsLeft, "F3:95:30:3E:DD:4A", fronRoomBlindLock)
        Thread::sleep(500)
        getPosition.apply(FrontRoomBlindsRight, "C1:03:74:91:05:6C", fronRoomBlindLock)

end

I have to use the lock because the call is ultimately made by a BT device. This throws an error if there are two simultaneous calls. With multiple items in a group, this happens a lot.

What I am seeing through is from another call - getPosition, which uses the same lock but is printing warnings that it is waiting on the lock. What is odd is that the last operation was a while ago so not use why it is.
I do have some exceptions taking place - still trying to understand - but I think it’s them which are tripping up and not doing the unlock.

So, correct use or not? Better way?

thanks.

1 Like
2

Note, if your function does throw an error - it does not release the lock

3

How so? Each try has a finally.

4

Quite right - my misunderstanding !!

I don’t fully understand your sequence and timing here, but something that does strike me:
When an Item receives a command, do not rely on autoupdate changing its state before triggering ‘received command’ rules. Looks like you are using .state in the lambda in that way?
The way round that is to use receivedCommand in the rule.

5

Objects like ReentrantLocks are passed by reference. I’m not certain how primitives are handled.

But this also means, for example, you set a passend in variable to null within the lambda, it will only be set to null inside the lambda, not in the calling code.

I’d recommend Design Pattern: Gate Keeper over lambdas for this.

It centralizes the checking and sleeps/delays and eliminated any issues from using lambdas. I think it will result in simpler code in the end.

6

Rich,
In that example, is there a risk of “lost” changes where the item changes state quickly, it holds at the .lock point but by the time it gets to execute the value could have changed (with the follow-up change queued).

Come to think of it, my code my have the same condition - I use the .state inside the .lock so it could well have changed. The fact that my code is “isolated” by function mitigates the risk of your example?

The code works flawlessly unless the up/down (for blinds) is stabbed multiple times by people. It’s then when it seems to “freeze” and get queued up. Trying to work that out.

7

@rossko57 - I am not sure I follow your concern. You saying the received command could trigger before the .state is updated internally?

8

That’s right. When a command arrives on the OH event bus, it triggers any rules looking for ‘received command’. It triggers any bindings that might pass the command to external devices. It also triggers the autoupdate feature that generates an update of the Item’s state (unless inhibited). The race is on … what happens first is indeterminate, but autoupdate effect is always a finite time later.

So, a rule triggered by received command that examines the Item.state may get state from before or after autoupdate changes it. It may change during the rule execution.

We’ve all fallen into this pitfall. You can deal with it by using the receivedCommand within the rule, or you can trigger the rule instead from Item changed or Item updated, if you need the update to actually take place.
Passing receivedCommand to a lambda function should ‘capture’ its value during that call.

I find it helpful to think of autoupdate as a psuedo-binding. Like other bindings, it listens for commands and responds by putting an update on the event bus for OH to process - eventually.

1 Like
9

Very interesting reply - thank you.
I don’t recall seeing anything at the moment which would suggest this is causing me a headache but I suspect it’s one of those things that it works often enough that you don’t realise the problem is lurking.
I’ll get things changed over to use than which should hopefully remove some doubt from the whole thing.

If the above is true (sorry, still digging :slight_smile: ) then is Rich’s example not more at risk?


rule "433MHz Controller"
when
    Item WirelessController received command
then
    lock.lock // Ensures only one instance of the Rule can run at a time
    try {
        val results = executeCommandLine(WirelessController.state.toString, 5000)
        logDebug("433", results)
snip

In this example .state really is non-deterministic by the time it actually gets called. It could be ahead or behind of where it actually is in time.

I still think my problem is that the exceptions being raised in the lambda are not hitting the finally and not freeing the lock. Or, there is something odd happening internally with the way the lambda is called and references passed for the locking object.

Either way, with very little effort I can see things crashing into things and the blinds don’t work as well as they should.

I will try create a working example this evening to prove or disprove it. That should be fun… :expressionless:

10

A small risk - maybe. I dunno how the example WirelessController is configured, bear in mind this issue mostly arises where instant autoupdate is assumed.

Obviously there’s a greater risk of confusion if there is a flurry of changes, which seems to be your situation. Putting locks in place to single-thread rule execution does not single-thread the rest of OH acting behind the scenes. it’s easy to imagine an orderly queue of code trying to work with updates that all happened nearly at once.

There are definitely ways to break lambdas without running the ‘finally’ block. I cannot remember the clues for that, @rlkoshak ,might, but of course you can add logInfo() to indicate entry and exit.

11

Is the import of the reentrant lock needed?

I use this in a state machine for my washing machine, but in VS Code i get an error:

The import 'java.util.concurrent.locks.ReentrantLock' is never used.
12

All I can offer is that I’ve seen inconsistent behavior with try/catch/finally inside lambdas and indeed it is possible to exit the lambda without the finally block running. I’ve not seen it in a very long time (I’ve eliminated all of my lambdas) and when I last saw it didn’t have time to fully explore it to figure out under what circumstances it occurs. That is one of the reasons I recommend the Gate Keeper DP approach over lambdas for something like this.

Yes

That means you are importing it but never actually using a ReentrantLock.

13

Here is my rules-file:

import java.util.concurrent.locks.ReentrantLock

// Variablen Waschmaschine + Trockner
val Number MODE_OFF = 0
val Number MODE_STANDBY = 1
val Number MODE_ACTIVE = 2
val Number MODE_FINISHED = 3

var java.util.concurrent.locks.ReentrantLock finishLock  = new java.util.concurrent.locks.ReentrantLock()

rule "Waschmaschine Consumption State Machine (Extended)"
when
    Item Strom_Waschmaschine changed
then
	if(SystemStarting.state == OFF) {
    if (Strom_Waschmaschine.state <= 0.31)
			Waschmaschine_OpState.postUpdate(MODE_OFF)
    if (Strom_Waschmaschine.state >= 0.39) {
			if (Waschmaschine_OpState.state == MODE_OFF) {
					Waschmaschine_OpState.postUpdate(MODE_ACTIVE)
			}
			else if (Waschmaschine_OpState.state == MODE_STANDBY) {
					Waschmaschine_OpState.postUpdate(MODE_ACTIVE)
			}
			else if ((Waschmaschine_OpState.state == MODE_FINISHED) && (Strom_Waschmaschine.state >= 0.40)) {
					Waschmaschine_OpState.postUpdate(MODE_ACTIVE)
					logInfo("RULES", "WaMa war finished - jetzt wieder active ---> Zustand Waschmaschine: " + Waschmaschine_OpState.state + " - PM Technik: " + PM_KG_Technik_Praesenz.state + "Strom: " + Strom_Waschmaschine.state)
			}
	}
    if ((Strom_Waschmaschine.state < 0.39) && (Strom_Waschmaschine.state > 0.31)) {
        	if ((Waschmaschine_OpState.state == MODE_OFF) && (PM_KG_Technik_Praesenz.state == ON)) {
					//if (PM_KG_Technik_Praesenz.state == ON) {
					Waschmaschine_OpState.postUpdate(MODE_STANDBY)
					//}
			}
        	if (Waschmaschine_OpState.state == MODE_ACTIVE) {
            		finishLock.lock()
            		try {
                			Thread::sleep(60000) // Debounce for 60 seconds
                					if (Strom_Waschmaschine.state <= 0.39) Waschmaschine_OpState.postUpdate(MODE_FINISHED)
            		}
					finally {
                			finishLock.unlock()
           			}
        	}
    }
	}
end
14

Once you import it:

import java.util.concurrent.locks.ReentrantLock

You don’t need to use the full package when using it

var java.util.concurrent.locks.ReentrantLock finishLock  = new java.util.concurrent.locks.ReentrantLock()

The whole point of the import is so you can use

var ReentrantLock finishLock = new ReentrantLock()

The warning is telling you the import isn’t doing anything because you never use ReentrantLock without all the package names in front of it.

15

Thanks, now the error is gone.

So i can use your way

or i can use my way without the import - only directly typing the path in the var-line? This would work too?

16

Yes.