From 87c4bd50535aa4a179da4d5c7acb04e8dce465eb Mon Sep 17 00:00:00 2001 From: "Alan T. DeKok" Date: Tue, 15 Jul 2025 11:36:31 -0400 Subject: [PATCH] correct behavior of parallel it helps to actually set p_result when pushing the child. Then, update the tests to match the documented behavior. --- src/lib/unlang/parallel.c | 12 ++-- src/lib/unlang/parallel_priv.h | 3 +- src/tests/keywords/parallel-child-timeout | 39 +++++++----- .../parallel-parent-and-child-timeout | 41 ------------- src/tests/keywords/parallel-rcode | 59 +++++++------------ 5 files changed, 50 insertions(+), 104 deletions(-) delete mode 100644 src/tests/keywords/parallel-parent-and-child-timeout diff --git a/src/lib/unlang/parallel.c b/src/lib/unlang/parallel.c index 992f402c65..4eb0e21fc5 100644 --- a/src/lib/unlang/parallel.c +++ b/src/lib/unlang/parallel.c @@ -169,14 +169,14 @@ static unlang_action_t unlang_parallel_resume(unlang_result_t *p_result, request /* * Do priority over-ride. */ - if (cr->result.priority > p_result->priority) { + if (cr->result.priority > state->result.priority) { RDEBUG4("** [%i] %s - overwriting existing result (%s %d) from higher priority to (%s %d)", stack_depth_current(request), __FUNCTION__, - fr_table_str_by_value(mod_rcode_table, p_result->rcode, ""), - p_result->priority, + fr_table_str_by_value(mod_rcode_table, state->result.rcode, ""), + state->result.priority, fr_table_str_by_value(mod_rcode_table, cr->result.rcode, ""), cr->result.priority); - *p_result = cr->result; + state->result = cr->result; } } @@ -193,6 +193,7 @@ static unlang_action_t unlang_parallel_resume(unlang_result_t *p_result, request state->children[i].state = CHILD_FREED; } + *p_result = state->result; return UNLANG_ACTION_CALCULATE_RESULT; } @@ -224,8 +225,7 @@ static unlang_action_t unlang_parallel(unlang_result_t *p_result, request_t *req } (void) talloc_set_type(state, unlang_parallel_state_t); - state->result = RLM_MODULE_NOOP; - state->priority = MOD_ACTION_NOT_SET; /* as-yet unset */ + state->result = UNLANG_RESULT_NOT_SET; state->detach = gext->detach; state->clone = gext->clone; state->num_children = g->num_children; diff --git a/src/lib/unlang/parallel_priv.h b/src/lib/unlang/parallel_priv.h index a980f5b2ef..eb71d1cd6a 100644 --- a/src/lib/unlang/parallel_priv.h +++ b/src/lib/unlang/parallel_priv.h @@ -34,8 +34,7 @@ extern "C" { #endif typedef struct { - rlm_rcode_t result; - int priority; + unlang_result_t result; //!< our result unsigned int num_children; //!< How many children are executing. unsigned int num_runnable; //!< How many children are complete. diff --git a/src/tests/keywords/parallel-child-timeout b/src/tests/keywords/parallel-child-timeout index 818d9d527b..3d7cc2f50b 100644 --- a/src/tests/keywords/parallel-child-timeout +++ b/src/tests/keywords/parallel-child-timeout @@ -1,23 +1,30 @@ # # PRE: parallel # -try { - parallel { - timeout 1s { - %time.advance(2s) - parent.control.NAS-Port += 1 - } - # Ensure a timeout in one child does not affect the other - group { - parent.control.NAS-Port += 1 - ok - } +parallel { + timeout 1s { + %time.advance(2s) + parent.control.Class := 0x01 } -} -catch timeout { - if (!(%{control.NAS-Port[#]} == 1)) { - test_fail + # Ensure a timeout in one child does not affect the other + group { + parent.control.Class := 0x02 + ok } +} - success +# +# This assignment should not have been run +# +if (control.Class == 0x01) { + test_fail } + +# +# But this assignment should have run. +# +if (control.Class != 0x02) { + test_fail +} + +success diff --git a/src/tests/keywords/parallel-parent-and-child-timeout b/src/tests/keywords/parallel-parent-and-child-timeout deleted file mode 100644 index cf668e75d5..0000000000 --- a/src/tests/keywords/parallel-parent-and-child-timeout +++ /dev/null @@ -1,41 +0,0 @@ -# -# PRE: parallel -# -try { - parallel { - # This child should get cancelled by the timeout section - timeout 1s { - parallel { - group { - delay_10s - outer.control += { - NAS-Port = 1 - } - ok - } - group { - outer.control += { - NAS-Port = 1 - } - # Trigger the parent's timeout section so we don't have to wait - %time.advance(2s) - ok - } - } - } - # This child should complete - group { - outer.control += { - NAS-Port = 1 - } - ok - } - } -} -catch timeout { - if (!(%{control.NAS-Port[#]} == 2)) { - test_fail - } - - success -} diff --git a/src/tests/keywords/parallel-rcode b/src/tests/keywords/parallel-rcode index e44f572325..eae154eb4e 100644 --- a/src/tests/keywords/parallel-rcode +++ b/src/tests/keywords/parallel-rcode @@ -2,66 +2,47 @@ # PRE: parallel # -# -# Ensure that a failing item causes the parallel section to return by default [1/2] -# -group { - parallel { - fail # Runs - ok # Should not run (would override fail - same priority) - ok - } - - actions { - fail = 1 - } -} -if (!fail) { - test_fail +noop { + noop = 1 } # -# Ensure that a failing item causes the parallel section to return by default [2/2] +# Ensure that a failing item causes the parallel section to return by default [1/2] # group { parallel { - fail - group { # This should *NOT* be dispatched - Filter-Id := 'foo' + fail { + fail = 2 + } + updated { + updated = 3 + } + ok { + ok = 4 } - } - actions { - fail = 1 } } -if (!fail) { - test_fail -} -if (Filter-Id) { +if (!ok) { test_fail } # -# If the rcode for the failing item is no longer action return, all items should be -# executed. +# Ensure that a failing item does NOT causes the parallel section to +# return. Instead, we run all children to the end. The result is +# the same as if all children had run sequentially. # group { parallel { - fail { - fail = 10 # Higher priority than ok and no longer action return - } - group { # This should now be dispatched - parent.request.Filter-Id := 'foo' + fail + group { # This SHOULD be dispatched + parent.control.Class := 0x01 } } - actions { - fail = 1 - } } -if (!fail) { +if (!ok) { test_fail } -if (!(Filter-Id == 'foo')) { +if (control.Class != 0x01) { test_fail } -- 2.47.2