From 93e6e396e41fb84a1e5424c7ddaf92593fd37ccb Mon Sep 17 00:00:00 2001 From: Sam Moran Date: Fri, 13 Sep 2024 04:07:10 +0000 Subject: [PATCH 1/5] * Reactively checked child returnning running will interrupt a previously running child --- src/runtime/forester.rs | 18 ++++- src/runtime/forester/flow.rs | 27 ++++++- src/tests/flow.rs | 76 +++++++++++++++++++ .../r_fallback_halted_by_running/main.tree | 15 ++++ .../r_sequence_halted_by_running/main.tree | 13 ++++ 5 files changed, 142 insertions(+), 7 deletions(-) create mode 100644 tree/tests/flow/r_fallback_halted_by_running/main.tree create mode 100644 tree/tests/flow/r_sequence_halted_by_running/main.tree diff --git a/src/runtime/forester.rs b/src/runtime/forester.rs index 57e1575..016d92f 100644 --- a/src/runtime/forester.rs +++ b/src/runtime/forester.rs @@ -230,15 +230,27 @@ impl Forester { debug!(target:"flow[run]", "tick:{}, {tpe}. stay with the new state: {}",ctx.curr_ts(),&ns); ctx.new_state(id, ns)?; } - FlowDecision::Halt(..) => { - return Err(RuntimeError::Unexpected("A child returning running should not trigger a flow node halt decision.".to_string())); + FlowDecision::Halt(new_state, halting_child_cursor) => { + // A reactively-checked child returning running will halt any other running child. + // Pop ourselves then halt the previously running child. + let halting_child_id = children[halting_child_cursor]; + debug!(target:"flow[run]", "tick:{}, {tpe}. Reactively checked child '{child}' has returned running. Halting previously running child '{halting_child_id}', then go up with new state: {}.", ctx.curr_ts(), new_state); + + ctx.new_state(id, new_state)?; + ctx.pop()?; + + ctx.force_to_halting_state(halting_child_id)?; + ctx.push(halting_child_id)?; } } } } // Halting must be an atomic process, it is never spread over multiple ticks so should never be seen in this flow. RNodeState::Halting(_) => { - return Err(RuntimeError::Unexpected("A child returning running should not trigger a flow node halt decision.".to_string())); + return Err(RuntimeError::Unexpected( + "A flow node child should never return a state of Halting." + .to_string(), + )); } // child is finished, thus the node needs to make a decision how to proceed. // this stage just updates the status and depending on the status, diff --git a/src/runtime/forester/flow.rs b/src/runtime/forester/flow.rs index 5767cad..f692a85 100644 --- a/src/runtime/forester/flow.rs +++ b/src/runtime/forester/flow.rs @@ -298,10 +298,29 @@ pub fn monitor( ) -> RtResult { match tpe { FlowType::RSequence | FlowType::RFallback => { - let cursor = read_cursor(tick_args.clone())?; - Ok(PopNode(RNodeState::Running( - tick_args.with(RUNNING_CHILD, RtValue::int(cursor)), - ))) + // RSequence and RFallback don't use P_CURSOR + // let's get the cursor manually so P_CURSOR doesn't accidentially poison our result. + let cursor = tick_args + .find(CURSOR.to_string()) + .and_then(RtValue::as_int) + .unwrap_or(0); + let previous_running_child = tick_args + .find(RUNNING_CHILD.to_string()) + .and_then(RtValue::as_int); + + let new_state = + RNodeState::Running(tick_args.with(RUNNING_CHILD, RtValue::int(cursor))); + + // Check there isn't another child already being tracked by the RUNNING_CHILD key. + // If there is we'll need to halt that before going any further. + if let Some(prev_running_child_cursor) = previous_running_child { + if prev_running_child_cursor > cursor { + return Ok(Halt(new_state, prev_running_child_cursor as usize)); + } + } + + // There was no other previously running child, continue as normal + Ok(PopNode(new_state)) } FlowType::Sequence | FlowType::MSequence | FlowType::Fallback => { let cursor = read_cursor(tick_args.clone())?; diff --git a/src/tests/flow.rs b/src/tests/flow.rs index ef89f45..3da3a75 100644 --- a/src/tests/flow.rs +++ b/src/tests/flow.rs @@ -322,6 +322,44 @@ fn r_sequence_halt_on_interrupt() { assert_eq!(x, 7) } +#[test] +fn r_sequence_halted_by_running() { + // See comment in the tree file for what this is testing. + let mut fb = fb("flow/r_sequence_halted_by_running"); + + fb.register_sync_action( + "incr", + GenerateData::new(|v| RtValue::int(v.as_int().unwrap_or(0) + 1)), + ); + + let mut f = fb.build().unwrap(); + assert_eq!(f.run(), Ok(TickResult::success())); + + let a = + f.bb.lock() + .unwrap() + .get("a".to_string()) + .ok() + .flatten() + .unwrap() + .clone() + .as_int() + .unwrap(); + assert_eq!(a, 10); + + let b = + f.bb.lock() + .unwrap() + .get("b".to_string()) + .ok() + .flatten() + .unwrap() + .clone() + .as_int() + .unwrap(); + assert_eq!(b, 9); +} + #[test] fn fallback() { let mut fb = fb("flow/fallback"); @@ -489,6 +527,44 @@ fn r_fallback_halt_on_interrupt() { assert_eq!(x, 7) } +#[test] +fn r_fallback_halted_by_running() { + // See comment in the tree file for what this is testing. + let mut fb = fb("flow/r_fallback_halted_by_running"); + + fb.register_sync_action( + "incr", + GenerateData::new(|v| RtValue::int(v.as_int().unwrap_or(0) + 1)), + ); + + let mut f = fb.build().unwrap(); + assert_eq!(f.run(), Ok(TickResult::success())); + + let a = + f.bb.lock() + .unwrap() + .get("a".to_string()) + .ok() + .flatten() + .unwrap() + .clone() + .as_int() + .unwrap(); + assert_eq!(a, 10); + + let b = + f.bb.lock() + .unwrap() + .get("b".to_string()) + .ok() + .flatten() + .unwrap() + .clone() + .as_int() + .unwrap(); + assert_eq!(b, 9); +} + #[test] fn parallel_simple() { turn_on_logs(); diff --git a/tree/tests/flow/r_fallback_halted_by_running/main.tree b/tree/tests/flow/r_fallback_halted_by_running/main.tree new file mode 100644 index 0000000..9b245bc --- /dev/null +++ b/tree/tests/flow/r_fallback_halted_by_running/main.tree @@ -0,0 +1,15 @@ +import "std::actions" +impl incr(key:string,default:num); + +// We're testing a reactively-checked child returning running will halt any other running children. +// If a running child does halt other running children, repeat(5) will be reset and b will be incremented a total of 9 times. If no halt is called, b will only be incremented 5 times. +root main repeat(2) sequence { + r_fallback { + sequence { + incr("a",0) + equal(a,5) + repeat(2) success() + } + repeat(5) incr("b",0) + } +} diff --git a/tree/tests/flow/r_sequence_halted_by_running/main.tree b/tree/tests/flow/r_sequence_halted_by_running/main.tree new file mode 100644 index 0000000..2ed86d5 --- /dev/null +++ b/tree/tests/flow/r_sequence_halted_by_running/main.tree @@ -0,0 +1,13 @@ +import "std::actions" +impl incr(key:string,default:num); + +// We're testing taht a reactively-checked child returning running will halt any other running children. +// If a running child does halt other running children, repeat(5) will be reset and b will be incremented a total of 9 times. If no halt is called, b will only be incremented 5 times. +root main retry(2) r_sequence { + fallback { + inverter incr("a",0) + inverter equal(a,5) + retry(2) fail_empty() + } + repeat(5) incr("b",0) +} From 6d18ed5fcfef02da6531b62032f03916cdf03530 Mon Sep 17 00:00:00 2001 From: Sam Moran Date: Fri, 13 Sep 2024 04:21:37 +0000 Subject: [PATCH 2/5] * Update documentation --- docs/src/falls.md | 4 +++- docs/src/seq.md | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/src/falls.md b/docs/src/falls.md index 687c9af..54bfc75 100644 --- a/docs/src/falls.md +++ b/docs/src/falls.md @@ -76,6 +76,8 @@ root main { The node `action` returns `running` and the whole sequence returns `running` but on the next tick it starts from the node `needs_to_charge` again. -`r_fallback` will halt the `running` child to allow a graceful shutdown if a prior child changes from `failure` to `success`. In the above example, if `needs_to_change` returned `success` on the second tick then `action` would be halted before `r_fallback` returned `success` itself. +`r_fallback` will halt the `running` child to allow a graceful shutdown if a prior child changes from `failure` to `success` or `running`. In the above example, if `needs_to_change` returned `success` on the second tick then `action` would be halted before `r_fallback` returned `success` itself. + +If `needs_to_charge` returned `running` on the second tick then `action` would also be halted, before potentially being restarted if `needs_to_charge` then returned `failure` again. Limiting the `r_fallback` node to a single `running` child avoids undefined behaviour in nodes that assume they are being run synchronously. Halting must be performed as quickly as possible. Note that currently only build-in flow, built-in decorator and sync action nodes are halted, async and remote actions are not. diff --git a/docs/src/seq.md b/docs/src/seq.md index 9d6e1ce..4e5ed1b 100644 --- a/docs/src/seq.md +++ b/docs/src/seq.md @@ -98,6 +98,8 @@ root main { The node `perform_action` returns `running` and the whole sequence returns `running` but on the next tick it starts from the node `store` again. -`r_sequence` will halt the `running` child to allow a graceful shutdown if a prior child changes from `success` to `failure`. In the above example, if `store` returned `failure` on the second tick then `perform_action` would be halted before `r_sequence` returned `failure` itself. +`r_sequence` will halt the `running` child to allow a graceful shutdown if a prior child changes from `success` to `failure` or `running`. In the above example, if `store` returned `failure` on the second tick then `perform_action` would be halted before `r_sequence` returned `failure` itself. + +If `store` returned `running` on the second tick then `perform_action` would also be halted, before potentially being restarted if `store` then returned `success` again. Limiting the `r_sequence` node to a single `running` child avoids undefined behaviour in nodes that assume they are being run synchronously. Halting must be performed as quickly as possible. Note that currently only build-in flow, built-in decorator and sync action nodes are halted, async and remote actions are not. From 723504cebd3071519ade67499c94adfc2eb8bed3 Mon Sep 17 00:00:00 2001 From: Sam Moran Date: Fri, 13 Sep 2024 04:23:07 +0000 Subject: [PATCH 3/5] * Update version number --- Cargo.lock | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 95547ea..73ee63a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -452,7 +452,7 @@ dependencies = [ [[package]] name = "forester-rs" -version = "0.4.0" +version = "0.4.1" dependencies = [ "axum", "chrono", diff --git a/Cargo.toml b/Cargo.toml index f12f3a1..5754d23 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ description = "Workflow framework based on the behavior trees" authors = ["BorisZhguchev "] homepage = "https://github.com/besok/forester" repository = "https://github.com/besok/forester" -version = "0.4.0" +version = "0.4.1" edition = "2021" license-file = "LICENSE" From a19a2a43bc295db0fa9ec057af83cbced4745837 Mon Sep 17 00:00:00 2001 From: Sam Moran Date: Fri, 13 Sep 2024 05:45:43 +0000 Subject: [PATCH 4/5] * comment tweaks --- tree/tests/flow/r_fallback_halted_by_running/main.tree | 2 +- tree/tests/flow/r_sequence_halted_by_running/main.tree | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tree/tests/flow/r_fallback_halted_by_running/main.tree b/tree/tests/flow/r_fallback_halted_by_running/main.tree index 9b245bc..69e1088 100644 --- a/tree/tests/flow/r_fallback_halted_by_running/main.tree +++ b/tree/tests/flow/r_fallback_halted_by_running/main.tree @@ -1,7 +1,7 @@ import "std::actions" impl incr(key:string,default:num); -// We're testing a reactively-checked child returning running will halt any other running children. +// We're testing that a reactively-checked child returning running will halt any other running children. // If a running child does halt other running children, repeat(5) will be reset and b will be incremented a total of 9 times. If no halt is called, b will only be incremented 5 times. root main repeat(2) sequence { r_fallback { diff --git a/tree/tests/flow/r_sequence_halted_by_running/main.tree b/tree/tests/flow/r_sequence_halted_by_running/main.tree index 2ed86d5..f56ace8 100644 --- a/tree/tests/flow/r_sequence_halted_by_running/main.tree +++ b/tree/tests/flow/r_sequence_halted_by_running/main.tree @@ -1,7 +1,7 @@ import "std::actions" impl incr(key:string,default:num); -// We're testing taht a reactively-checked child returning running will halt any other running children. +// We're testing that a reactively-checked child returning running will halt any other running children. // If a running child does halt other running children, repeat(5) will be reset and b will be incremented a total of 9 times. If no halt is called, b will only be incremented 5 times. root main retry(2) r_sequence { fallback { From 97946c6113e1a637b1044ac25a709e9470ad7eda Mon Sep 17 00:00:00 2001 From: Sam Moran Date: Fri, 13 Sep 2024 06:34:27 +0000 Subject: [PATCH 5/5] * Simplified test tree by removing unnecessary sequence --- .../flow/r_fallback_halted_by_running/main.tree | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tree/tests/flow/r_fallback_halted_by_running/main.tree b/tree/tests/flow/r_fallback_halted_by_running/main.tree index 69e1088..c456ebf 100644 --- a/tree/tests/flow/r_fallback_halted_by_running/main.tree +++ b/tree/tests/flow/r_fallback_halted_by_running/main.tree @@ -3,13 +3,11 @@ impl incr(key:string,default:num); // We're testing that a reactively-checked child returning running will halt any other running children. // If a running child does halt other running children, repeat(5) will be reset and b will be incremented a total of 9 times. If no halt is called, b will only be incremented 5 times. -root main repeat(2) sequence { - r_fallback { - sequence { - incr("a",0) - equal(a,5) - repeat(2) success() - } - repeat(5) incr("b",0) +root main repeat(2) r_fallback { + sequence { + incr("a",0) + equal(a,5) + repeat(2) success() } + repeat(5) incr("b",0) }