Bugfix/#69 fix action pipeline deadlocks and other issues that cause random action cancelations. #89

DivineThreepwood · 2022-12-11T16:38:18Z

📜 Description

Changes proposed in this pull request:

fix potential deadlock between actionDescriptionBuilderLock and actionTaskLock in ActionImpl.
make sure reschedule is not triggered once the unit controller is shutting down to avoid lots of exceptions.
Make sure required state observer is shutdown in case the scene state changes to not interfere with future scene activations. Improve required state validation by filtering by validate the initial action to avoid scene cancelation based on an outdated action. Minor typo fixes.
fix logic issue in equalServiceStates check.
Implement validateInitialAction service method for Actions.
Make sure remote action cleanup task does not cleanup the state of other actions that are later observed by the same remote instance, by canceling the cleanup task once the remote is reinitialized.
make sure remote action list is always locked once accessed.
Code readability improvements.
clearly define start and end of an bco test to enable the separation of test prints vs. setup / cleanup prints.
improve test logging.

…nTaskLock in ActionImpl.

…tting down to avoid lots of exceptions.

… changes to not interfear with future scene activations. Improve required state validation by filtering by validate the initial action to avoid scene cancelation based on an outdated action. Minor typo fixes.

…her actions that are later observed by the same remote instance, by canceling the cleanup task once the remote is reinitialized.

…of test prints vs. setup / cleanup prints

…pipeline_deadlocks_and_other_pipeline_issues

pLeminoq

Overall, a really nice PR. I just have suggestions for a few minor changes and I think the actionTask issue has to be discussed.

pLeminoq · 2022-12-13T09:20:57Z

module/dal/control/src/main/java/org/openbase/bco/dal/control/action/ActionImpl.java

+
+    // any code that accesses the data lock while holding the action task lock should previously lock the data lock.
+    // otherwise deadlocks can occur.


This comment is not entirely clear to me, especially with regard to the proposed change below. There are two possibilities:

We always need to acquire the data lock if we need the the action task lock and the data lock has to be acquired first.

We don't need to acquire the data lock in every case we need the action task look. But if we do, the data lock has to be acquired first.

I think you mean option 2. But in light of the proposed change below, this is not easily visible since the requirement to acquire the data lock is hidden in the isTerminating() call.

In general, I think the usage of such a comment is sub-optimal as you will not see this comment during coding. Would it be possible to implement a structure where when you acquire the action task lock, you have to consciously decide if you also need to acquire the data lock?

...control/src/main/java/org/openbase/bco/dal/control/layer/unit/scene/SceneControllerImpl.java

pLeminoq · 2022-12-13T09:26:27Z

...control/src/main/java/org/openbase/bco/dal/control/layer/unit/scene/SceneControllerImpl.java

                        }

                        // register an observer which will deactivate the scene if one required action is now longer running
                        // observer will cleanup itself after the action is no longer valid so no need to care fore it.


I guess the second line of this comment is now outdated and should be removed.

pLeminoq · 2022-12-13T09:27:03Z

...control/src/main/java/org/openbase/bco/dal/control/layer/unit/scene/SceneControllerImpl.java


        private void verifyAllStates() {
            try {
+                // logger.trace("verify "+unitAndRequiredServiceStateMap.entrySet().size()+ " states of "+ getLabel("?"));


Maybe remove commented debug log

pLeminoq · 2022-12-13T09:28:51Z

...control/src/main/java/org/openbase/bco/dal/control/layer/unit/scene/SceneControllerImpl.java

        @Override
        public void update(ServiceStateProvider<Message> serviceStateProvider, Message serviceState) throws Exception {
            try {
+                // logger.error("Incomming service state update from "+ serviceStateProvider.getServiceProvider().getId() + " triggered from "+ serviceState.toString());


Maybe remove commented log

module/dal/remote/src/main/java/org/openbase/bco/dal/remote/action/Actions.java

module/dal/remote/src/main/java/org/openbase/bco/dal/remote/action/RemoteAction.java

pLeminoq · 2022-12-13T09:52:17Z

module/dal/remote/src/main/java/org/openbase/bco/dal/remote/action/RemoteAction.java

+        if (description == null && actionReference != null) {
+            description = ActionDescriptionProcessor.toString(actionReference);
+        }

-            // resolve via parameter
-            if (actionParameter != null) {
-                return ActionDescriptionProcessor.toString(actionParameter);
-            }
+        // resolve via parameter
+        if (description == null && actionParameter != null) {
+            description = ActionDescriptionProcessor.toString(actionParameter);


Since the description is set to an empty string above you description.isEmpty() should be used instead of the null checks.

pLeminoq · 2022-12-13T09:54:44Z

module/dal/test/src/test/java/org/openbase/bco/dal/test/layer/unit/scene/SceneRemoteTest.java


+            LOGGER.warn("turn off...");
            waitForExecution(sceneRemoteOff.setActivationState(State.ACTIVE, SCENE_ACTION_PARAM));
+            LOGGER.warn("turn off... continue");


Are these debugging logs still required?

…layer/unit/scene/SceneControllerImpl.java Co-authored-by: pLeminoq <thuxohl@techfak.uni-bielefeld.de>

Co-authored-by: pLeminoq <thuxohl@techfak.uni-bielefeld.de>

…SceneController.

…line_issues' of https://github.com/openbase/bco into bugfix/#69_fix_action_pipeline_deadlocks_and_other_pipeline_issues

DivineThreepwood · 2022-12-13T23:08:27Z

Thanks for the detailed review. All conversations are addressed, so PR is ready to review again.

DivineThreepwood added 10 commits December 11, 2022 16:32

fix potential deadlock between actionDescriptionBuilderLock and actio…

c9ea807

…nTaskLock in ActionImpl.

make sure reschedule is not triggered once the unit controller is shu…

196bdf8

…tting down to avoid lots of exceptions.

Make sure required state observer is shutdown in case the scene state…

defea93

… changes to not interfear with future scene activations. Improve required state validation by filtering by validate the initial action to avoid scene cancelation based on an outdated action. Minor typo fixes.

fix logic issue in equalServiceStates check.

3726dd7

Implememnt validateInitialAction service method for Actions.

5db7a66

Make sure remote action cleanup task does not cleanup the state of ot…

4022d5e

…her actions that are later observed by the same remote instance, by canceling the cleanup task once the remote is reinitialized.

make sure remote action list is always locked once accessed.

942e460

Code readability improvements.

66e9eb6

clearly define start and end of an bco test to enable the seperation …

8937134

…of test prints vs. setup / cleanup prints

improve test logging.

0f84d7e

DivineThreepwood requested a review from pLeminoq December 11, 2022 16:38

DivineThreepwood assigned pLeminoq Dec 11, 2022

DivineThreepwood added the ready to review label Dec 11, 2022

DivineThreepwood removed a link to an issue Dec 11, 2022

AbstractEnergySavingAgent causes Deadlock as follows #84

Closed

Merge remote-tracking branch 'origin/dev' into bugfix/#69_fix_action_…

5bbfc42

…pipeline_deadlocks_and_other_pipeline_issues

DivineThreepwood added preview Auto build a docker image and deploy it as a preview version. and removed preview Auto build a docker image and deploy it as a preview version. labels Dec 11, 2022

DivineThreepwood added 3 commits December 11, 2022 18:40

remove debug logging

43240e4

cleanup code

36f2407

log cleanup

a25bbb6

DivineThreepwood mentioned this pull request Dec 11, 2022

Try to establish evergreen dev branch again #90

Merged

pLeminoq requested changes Dec 13, 2022

View reviewed changes

DivineThreepwood added the changes requested label Dec 13, 2022

Update module/dal/control/src/main/java/org/openbase/bco/dal/control/…

f2cf075

…layer/unit/scene/SceneControllerImpl.java Co-authored-by: pLeminoq <thuxohl@techfak.uni-bielefeld.de>

DivineThreepwood removed the preview Auto build a docker image and deploy it as a preview version. label Dec 13, 2022

DivineThreepwood and others added 6 commits December 13, 2022 22:22

Apply suggestions from code review

0f373f8

Co-authored-by: pLeminoq <thuxohl@techfak.uni-bielefeld.de>

switch unit test debug log from warn to debug

08e70b1

improve toString method of RemoteAction.

b515e7e

update jul to get sl4j lambda support.

d5e13a5

improve locking note in action impl and introduce lambda logging for …

151593e

…SceneController.

Merge branch 'bugfix/#69_fix_action_pipeline_deadlocks_and_other_pipe…

d249c68

…line_issues' of https://github.com/openbase/bco into bugfix/#69_fix_action_pipeline_deadlocks_and_other_pipeline_issues

DivineThreepwood requested a review from pLeminoq December 13, 2022 23:08

DivineThreepwood removed the changes requested label Dec 13, 2022

pLeminoq approved these changes Dec 14, 2022

View reviewed changes

DivineThreepwood added code approved and removed ready to review labels Dec 14, 2022

DivineThreepwood merged commit fa3cc42 into dev Dec 14, 2022

DivineThreepwood deleted the bugfix/#69_fix_action_pipeline_deadlocks_and_other_pipeline_issues branch December 14, 2022 22:24

DivineThreepwood mentioned this pull request Dec 14, 2022

Improve Action Task Locking Strategy in ActionImpl to avoid Deadlocks in longterm. #91

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/#69 fix action pipeline deadlocks and other issues that cause random action cancelations. #89

Bugfix/#69 fix action pipeline deadlocks and other issues that cause random action cancelations. #89

Uh oh!

DivineThreepwood commented Dec 11, 2022

Uh oh!

pLeminoq left a comment

Uh oh!

pLeminoq Dec 13, 2022

Uh oh!

Uh oh!

Uh oh!

pLeminoq Dec 13, 2022

Uh oh!

pLeminoq Dec 13, 2022

Uh oh!

pLeminoq Dec 13, 2022

Uh oh!

Uh oh!

Uh oh!

pLeminoq Dec 13, 2022

Uh oh!

pLeminoq Dec 13, 2022

Uh oh!

DivineThreepwood commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		// any code that accesses the data lock while holding the action task lock should previously lock the data lock.
		// otherwise deadlocks can occur.

Bugfix/#69 fix action pipeline deadlocks and other issues that cause random action cancelations. #89

Bugfix/#69 fix action pipeline deadlocks and other issues that cause random action cancelations. #89

Uh oh!

Conversation

DivineThreepwood commented Dec 11, 2022

📜 Description

Uh oh!

pLeminoq left a comment

Choose a reason for hiding this comment

Uh oh!

pLeminoq Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pLeminoq Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

pLeminoq Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

pLeminoq Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pLeminoq Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

pLeminoq Dec 13, 2022

Choose a reason for hiding this comment

Uh oh!

DivineThreepwood commented Dec 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants