Remove UNLANG_ACTION_STOP_PROCESSING

It's not needed anymore.  Fatal errors should use `RETURN_UNLANG_ACTION_FATAL` to signal the request to stop.

Fix issue where finally-timeout test wasn't actually running because the dummy request got cancalled during the virtual server call (oops).

Add regression tests for module calls being immediately cancelled in finally sections.

diff --git a/src/lib/io/control.h b/src/lib/io/control.h
index c82782bbbfad0f10cf1b9db838006f90bb8cc4c3..fde6a55b1b66063b6777ecac8ae47739ac22469a 100644 (file)
--- a/src/lib/io/control.h
+++ b/src/lib/io/control.h
@@ -72,7 +72,7 @@ ssize_t fr_control_message_pop(fr_atomic_queue_t *aq, uint32_t *p_id, void *data
 int fr_control_callback_add(fr_control_t *c, uint32_t id, void *ctx, fr_control_callback_t callback) CC_HINT(nonnull(1,4));
 int fr_control_callback_delete(fr_control_t *c, uint32_t id) CC_HINT(nonnull);
 
-int fr_control_same_thread(fr_control_t *c);
+int fr_control_same_thread(fr_control_t *c) CC_HINT(nonnull);
 
 #ifdef __cplusplus
 }

diff --git a/src/lib/unlang/action.h b/src/lib/unlang/action.h
index 6212cfd017ce7f2f20332e3a2f72ed55cbb6cbbc..c88e16ddf7d22581d6ab6746c8ae30a33edd1d39 100644 (file)
--- a/src/lib/unlang/action.h
+++ b/src/lib/unlang/action.h
@@ -39,9 +39,14 @@ typedef enum {
        UNLANG_ACTION_PUSHED_CHILD,             //!< #unlang_t pushed a new child onto the stack,
                                                //!< execute it instead of continuing.
        UNLANG_ACTION_YIELD,                    //!< Temporarily pause execution until an event occurs.
-       UNLANG_ACTION_STOP_PROCESSING           //!< Break out of processing the current request (unwind).
 } unlang_action_t;
 
+#define RETURN_UNLANG_ACTION_FATAL \
+{ \
+       unlang_interpret_signal(request, FR_SIGNAL_CANCEL); \
+       return UNLANG_ACTION_FAIL; \
+}
+
 #ifdef __cplusplus
 }
 #endif

diff --git a/src/lib/unlang/function.c b/src/lib/unlang/function.c
index 7c8dc1174b0cb1ccc14088ac2ed07cffc5aac71b..17489614eae6e4de2adae23523f96da7ba3f4125 100644 (file)
--- a/src/lib/unlang/function.c
+++ b/src/lib/unlang/function.c
@@ -125,9 +125,6 @@ again:
        ua = func(p_result, request, state->uctx);
        if (REPEAT(state)) { /* set again by func */
                switch (ua) {
-               case UNLANG_ACTION_STOP_PROCESSING:
-                       break;
-
                case UNLANG_ACTION_CALCULATE_RESULT:
                        goto again;
 
@@ -161,9 +158,6 @@ static unlang_action_t call_with_result(unlang_result_t *p_result, request_t *re
        state->func_name = NULL;
        if (REPEAT(state)) {
                switch (ua) {
-               case UNLANG_ACTION_STOP_PROCESSING:
-                       break;
-
                case UNLANG_ACTION_CALCULATE_RESULT:
                        ua = call_with_result_repeat(p_result, request, frame);
                        break;
@@ -208,9 +202,6 @@ again:
        ua = func(request, state->uctx);
        if (REPEAT(state)) { /* set again by func */
                switch (ua) {
-               case UNLANG_ACTION_STOP_PROCESSING:
-                       break;
-
                case UNLANG_ACTION_CALCULATE_RESULT:
                        goto again;
 
@@ -253,9 +244,6 @@ static unlang_action_t call_no_result(UNUSED unlang_result_t *p_result, request_
        state->func_name = NULL;
        if (REPEAT(state)) {
                switch (ua) {
-               case UNLANG_ACTION_STOP_PROCESSING:
-                       break;
-
                case UNLANG_ACTION_CALCULATE_RESULT:
                        ua = call_no_result_repeat(p_result, request, frame);
                        break;
@@ -568,7 +556,7 @@ void unlang_function_init(void)
                        .dump = unlang_function_dump,
 
                        .unlang_size = sizeof(unlang_group_t),
-                       .unlang_name = "unlang_group_t",        
+                       .unlang_name = "unlang_group_t",
 
                        .frame_state_size = sizeof(unlang_frame_state_func_t),
                        .frame_state_type = "unlang_frame_state_func_t",

diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c
index c2ebfb2f39ec725ca0cf6606f263023ffffba313..4d84f689eb0412ee2fe621ea5e236cc302f9a059 100644 (file)
--- a/src/lib/unlang/interpret.c
+++ b/src/lib/unlang/interpret.c
@@ -46,7 +46,6 @@ static fr_table_num_ordered_t const unlang_action_table[] = {
        { L("calculate-result"),        UNLANG_ACTION_CALCULATE_RESULT },
        { L("next"),                    UNLANG_ACTION_EXECUTE_NEXT },
        { L("pushed-child"),            UNLANG_ACTION_PUSHED_CHILD },
-       { L("stop"),                    UNLANG_ACTION_STOP_PROCESSING },
        { L("yield"),                   UNLANG_ACTION_YIELD }
 };
 static size_t unlang_action_table_len = NUM_ELEMENTS(unlang_action_table);
@@ -379,7 +378,7 @@ static int _local_variables_free(unlang_variable_ref_t *ref)
  * @return
  *     - UNLANG_ACTION_PUSHED_CHILD on success.
  *     - UNLANG_ACTION_EXECUTE_NEXT do nothing, but just go to the next sibling instruction
- *     - UNLANG_ACTION_STOP_PROCESSING, fatal error, usually stack overflow.
+ *     - UNLANG_ACTION_FAIL, fatal error, usually stack overflow.
  */
 unlang_action_t unlang_interpret_push_children(unlang_result_t *p_result, request_t *request,
                                               rlm_rcode_t default_rcode, bool do_next_sibling)
@@ -405,7 +404,7 @@ unlang_action_t unlang_interpret_push_children(unlang_result_t *p_result, reques
 
        if (unlang_interpret_push(p_result, request, unlang_list_head(&g->children),
                                  FRAME_CONF(default_rcode, UNLANG_SUB_FRAME), do_next_sibling) < 0) {
-               return UNLANG_ACTION_STOP_PROCESSING;
+               RETURN_UNLANG_ACTION_FATAL;
        }
 
        if (!g->variables) return UNLANG_ACTION_PUSHED_CHILD;
@@ -831,15 +830,6 @@ unlang_frame_action_t frame_eval(request_t *request, unlang_stack_frame_t *frame
                if (is_unwinding(frame)) goto calculate_result;
 
                switch (ua) {
-               case UNLANG_ACTION_STOP_PROCESSING:
-                       /*
-                        *      This marks all the cancellable
-                        *      frames with the unwind flag,
-                        *      and starts popping them.
-                        */
-                       unlang_interpret_signal(request, FR_SIGNAL_CANCEL);
-                       return UNLANG_FRAME_ACTION_POP;
-
                /*
                 *      The operation resulted in additional frames
                 *      being pushed onto the stack, execution should

diff --git a/src/lib/unlang/load_balance.c b/src/lib/unlang/load_balance.c
index 2b723d3a6efede124c00bf15216c0693e8cc3b6a..94e7d8687422dbde4ae7cffe62a9ca6a361164cf 100644 (file)
--- a/src/lib/unlang/load_balance.c
+++ b/src/lib/unlang/load_balance.c
@@ -93,7 +93,7 @@ push:
         */
        if (unlang_interpret_push(&redundant->result, request, redundant->child,
                                  FRAME_CONF(RLM_MODULE_NOT_SET, UNLANG_SUB_FRAME), UNLANG_NEXT_STOP) < 0) {
-               return UNLANG_ACTION_STOP_PROCESSING;
+               RETURN_UNLANG_ACTION_FATAL;
        }
 
        return UNLANG_ACTION_PUSHED_CHILD;
@@ -123,7 +123,7 @@ static unlang_action_t unlang_load_balance(unlang_result_t *p_result, request_t
        uint32_t                        count = 0;
 
 #ifdef STATIC_ANALYZER
-       if (!g || unlang_list_empty(&g->children)) return UNLANG_ACTION_STOP_PROCESSING;
+       if (!g || unlang_list_empty(&g->children)) return UNLANG_ACTION_FAIL;
 #else
        fr_assert(g != NULL);
        fr_assert(!unlang_list_empty(&g->children));
@@ -220,7 +220,7 @@ static unlang_action_t unlang_load_balance(unlang_result_t *p_result, request_t
        if (frame->instruction->type == UNLANG_TYPE_LOAD_BALANCE) {
                if (unlang_interpret_push(p_result, request, redundant->start,
                                          FRAME_CONF(RLM_MODULE_NOT_SET, UNLANG_SUB_FRAME), UNLANG_NEXT_STOP) < 0) {
-                       return UNLANG_ACTION_STOP_PROCESSING;
+                       RETURN_UNLANG_ACTION_FATAL;
                }
                return UNLANG_ACTION_PUSHED_CHILD;
        }

diff --git a/src/lib/unlang/map.c b/src/lib/unlang/map.c
index a25cb00f2919c50c33b40f4b17080451cd62c80c..2256577dce96b4324b774c3552cc724b70b38228 100644 (file)
--- a/src/lib/unlang/map.c
+++ b/src/lib/unlang/map.c
@@ -186,14 +186,14 @@ static unlang_action_t unlang_map_state_init(unlang_result_t *p_result, request_
        case TMPL_TYPE_EXEC:
                if (unlang_tmpl_push(map_proc_state, NULL, &map_proc_state->src_result,
                                     request, inst->src, NULL, UNLANG_SUB_FRAME) < 0) {
-                       return UNLANG_ACTION_STOP_PROCESSING;
+                       RETURN_UNLANG_ACTION_FATAL;
                }
                return UNLANG_ACTION_PUSHED_CHILD;
 
        case TMPL_TYPE_XLAT:
                if (unlang_xlat_push(map_proc_state, NULL, &map_proc_state->src_result,
                                     request, tmpl_xlat(inst->src), false) < 0) {
-                       return UNLANG_ACTION_STOP_PROCESSING;
+                       RETURN_UNLANG_ACTION_FATAL;
                }
                return UNLANG_ACTION_PUSHED_CHILD;
 

diff --git a/src/lib/unlang/module.c b/src/lib/unlang/module.c
index 22e706f2bd82a8a8638ce93bbccf9f5be2fe1280..6ab8e0068ce2939edcae6cd9b6082f03b6b412ce 100644 (file)
--- a/src/lib/unlang/module.c
+++ b/src/lib/unlang/module.c
@@ -178,7 +178,7 @@ unlang_action_t unlang_module_yield_to_xlat(TALLOC_CTX *ctx, unlang_result_t *p_
        /*
         *      Push the xlat function
         */
-       if (unlang_xlat_push(ctx, p_result, out, request, exp, false) < 0) return UNLANG_ACTION_STOP_PROCESSING;
+       if (unlang_xlat_push(ctx, p_result, out, request, exp, false) < 0) RETURN_UNLANG_ACTION_FATAL;
 
        return UNLANG_ACTION_PUSHED_CHILD;
 }
@@ -285,7 +285,9 @@ unlang_action_t unlang_module_yield_to_section(unlang_result_t *p_result,
        (void) unlang_module_yield(request, resume, signal, sigmask, rctx);
 
        if (unlang_interpret_push_section(p_result, request, subcs,
-                                         FRAME_CONF(default_rcode, UNLANG_SUB_FRAME)) < 0) return UNLANG_ACTION_STOP_PROCESSING;
+                                         FRAME_CONF(default_rcode, UNLANG_SUB_FRAME)) < 0) {
+               RETURN_UNLANG_ACTION_FATAL;
+       }
 
        return UNLANG_ACTION_PUSHED_CHILD;
 }
@@ -615,17 +617,7 @@ static unlang_action_t unlang_module_resume(unlang_result_t *p_result, request_t
                    state->env_data, state->rctx), request);
        safe_unlock(m->mmc.mi);
 
-       if (request->master_state == REQUEST_STOP_PROCESSING) ua = UNLANG_ACTION_STOP_PROCESSING;
-
        switch (ua) {
-       case UNLANG_ACTION_STOP_PROCESSING:
-               RWARN("Module %s or worker signalled to stop processing request", m->mmc.mi->name);
-               state->thread->active_callers--;
-               request->module = state->previous_module;
-               p_result->rcode = RLM_MODULE_NOT_SET;
-               p_result->priority = MOD_ACTION_NOT_SET;
-               return UNLANG_ACTION_STOP_PROCESSING;
-
        case UNLANG_ACTION_YIELD:
                /*
                 *      The module yielded but didn't set a
@@ -888,18 +880,7 @@ static unlang_action_t unlang_module(unlang_result_t *p_result, request_t *reque
                               request);
        safe_unlock(m->mmc.mi);
 
-       if (request->master_state == REQUEST_STOP_PROCESSING) ua = UNLANG_ACTION_STOP_PROCESSING;
-
        switch (ua) {
-       /*
-        *      It is now marked as "stop" when it wasn't before, we
-        *      must have been blocked.
-        */
-       case UNLANG_ACTION_STOP_PROCESSING:
-               RWARN("Module %s became unblocked", m->mmc.mi->name);
-               request->module = state->previous_module;
-               return UNLANG_ACTION_STOP_PROCESSING;
-
        case UNLANG_ACTION_YIELD:
                state->thread->active_callers++;
 

diff --git a/src/lib/unlang/switch.c b/src/lib/unlang/switch.c
index df8ab6fc5eab1bad272382342149ab64cbf6c8f4..b0783e87206403233800b592c2cc58f492bd80b0 100644 (file)
--- a/src/lib/unlang/switch.c
+++ b/src/lib/unlang/switch.c
@@ -113,7 +113,7 @@ do_null_case:
        if (!found) return UNLANG_ACTION_EXECUTE_NEXT;
 
        if (unlang_interpret_push(NULL, request, found, FRAME_CONF(RLM_MODULE_NOT_SET, UNLANG_SUB_FRAME), UNLANG_NEXT_STOP) < 0) {
-               return UNLANG_ACTION_STOP_PROCESSING;
+               RETURN_UNLANG_ACTION_FATAL;
        }
 
        return UNLANG_ACTION_PUSHED_CHILD;
@@ -524,7 +524,7 @@ void unlang_switch_init(void)
 
                        .compile = unlang_compile_switch,
                        .interpret = unlang_switch,
-                               
+
                        .unlang_size = sizeof(unlang_switch_t),
                        .unlang_name = "unlang_switch_t",
 

diff --git a/src/lib/unlang/timeout.c b/src/lib/unlang/timeout.c
index 4d259bb333fe64f80adf9e24b8fb0690f824c39e..664e339ff1feda7ed641f82a12d893a2fee858dc 100644 (file)
--- a/src/lib/unlang/timeout.c
+++ b/src/lib/unlang/timeout.c
@@ -117,7 +117,7 @@ static unlang_action_t unlang_timeout_set(UNUSED unlang_result_t *p_result, requ
        if (fr_timer_in(state, unlang_interpret_event_list(request)->tl, &state->ev, state->timeout,
                        false, unlang_timeout_handler, state) < 0) {
                RPEDEBUG("Failed inserting event");
-               return UNLANG_ACTION_STOP_PROCESSING;
+               RETURN_UNLANG_ACTION_FATAL;
        }
 
        frame_repeat(frame, unlang_timeout_resume_done);

diff --git a/src/lib/unlang/tmpl.c b/src/lib/unlang/tmpl.c
index c1bf1062c16337698665f576a359c1136ecf99ad..7a7d7dbeaa8ec60f93ec0314bb42683fbc0c0400 100644 (file)
--- a/src/lib/unlang/tmpl.c
+++ b/src/lib/unlang/tmpl.c
@@ -252,7 +252,7 @@ static unlang_action_t unlang_tmpl(unlang_result_t *p_result, request_t *request
 push:
        if (unlang_xlat_push(state->ctx, &state->xlat_result, &state->list, request, tmpl_xlat(ut->tmpl), UNLANG_SUB_FRAME) < 0) {
        fail:
-               return UNLANG_ACTION_STOP_PROCESSING;
+               RETURN_UNLANG_ACTION_FATAL;
        }
 
        return UNLANG_ACTION_PUSHED_CHILD;

diff --git a/src/lib/unlang/try.c b/src/lib/unlang/try.c
index aa1be92f8d61909a1a4d7c7280b0dd1a976d15d6..e38ca69def2bf741223ea82e0c89461504b85a3a 100644 (file)
--- a/src/lib/unlang/try.c
+++ b/src/lib/unlang/try.c
@@ -42,7 +42,7 @@ static unlang_action_t skip_to_catch(UNUSED unlang_result_t *p_result, request_t
        if (gext->catch[rcode]) {
                if (unlang_interpret_push(NULL, request, gext->catch[rcode],
                                          FRAME_CONF(RLM_MODULE_NOT_SET, UNLANG_SUB_FRAME), false) < 0) {
-                       return UNLANG_ACTION_STOP_PROCESSING;
+                       RETURN_UNLANG_ACTION_FATAL;
                }
 
                return UNLANG_ACTION_PUSHED_CHILD;

diff --git a/src/lib/unlang/xlat.c b/src/lib/unlang/xlat.c
index 07fddf053734b745b197c555d1bf0a7489da47b2..4008a1060b90385be4b7b891b60126ac86d323c0 100644 (file)
--- a/src/lib/unlang/xlat.c
+++ b/src/lib/unlang/xlat.c
@@ -334,7 +334,7 @@ static unlang_action_t unlang_xlat_repeat(unlang_result_t *p_result, request_t *
                fr_value_box_list_talloc_free(&state->out);
                if (unlang_xlat_push(state->ctx, p_result, &state->out, request, child, false) < 0) {
                        REXDENT();
-                       return UNLANG_ACTION_STOP_PROCESSING;
+                       RETURN_UNLANG_ACTION_FATAL;
                }
                return UNLANG_ACTION_PUSHED_CHILD;
 
@@ -396,7 +396,7 @@ static unlang_action_t unlang_xlat(UNUSED unlang_result_t *p_result, request_t *
                fr_value_box_list_talloc_free(&state->out);
                if (unlang_xlat_push(state->ctx, p_result, &state->out, request, child, false) < 0) {
                        RINDENT_RESTORE(request, state);
-                       return UNLANG_ACTION_STOP_PROCESSING;
+                       RETURN_UNLANG_ACTION_FATAL;
                }
                return UNLANG_ACTION_PUSHED_CHILD;
 
@@ -510,7 +510,7 @@ static unlang_action_t unlang_xlat_resume(unlang_result_t *p_result, request_t *
                fr_value_box_list_talloc_free(&state->out);
                if (unlang_xlat_push(state->ctx, state->p_result, &state->out, request, child, false) < 0) {
                        RINDENT_RESTORE(request, state);
-                       return UNLANG_ACTION_STOP_PROCESSING;
+                       RETURN_UNLANG_ACTION_FATAL;
                }
                return UNLANG_ACTION_PUSHED_CHILD;
 

diff --git a/src/tests/keywords/finally b/src/tests/keywords/finally
index 56933d56a6dc575b5a106c2e7235942e7023e004..ca019a79a02de03a50378a1103f3261f80c3399d 100644 (file)
--- a/src/tests/keywords/finally
+++ b/src/tests/keywords/finally
@@ -13,5 +13,4 @@ if Tmp-String-0 != 'bye-from-packet-fin' {
        test_fail
 }
 
-# This should timeout, so it must call success in the finally section
-call timeout-fin { }
+success

diff --git a/src/tests/keywords/finally-timeout b/src/tests/keywords/finally-timeout
new file mode 100644 (file)
index 0000000..663a226
--- /dev/null
+++ b/src/tests/keywords/finally-timeout
@@ -0,0 +1,4 @@
+# This should timeout, so it must call success in the finally section
+# Reply attributes are set in the finally section of the server we're calling
+# and checked with the response filter, as this request will get cancelled too.
+call timeout-fin { }

diff --git a/src/tests/keywords/finally-timeout-module b/src/tests/keywords/finally-timeout-module
new file mode 100644 (file)
index 0000000..fa2dd49
--- /dev/null
+++ b/src/tests/keywords/finally-timeout-module
@@ -0,0 +1,4 @@
+# This should timeout, so it must call success in the finally section
+# Reply attributes are set in the finally section of the server we're calling
+# and checked with the response filter, as this request will get cancelled too.
+call module-timeout-fin { }

diff --git a/src/tests/keywords/finally-timeout-module.attrs b/src/tests/keywords/finally-timeout-module.attrs
new file mode 100644 (file)
index 0000000..cbbbdc9
--- /dev/null
+++ b/src/tests/keywords/finally-timeout-module.attrs
@@ -0,0 +1,6 @@
+Packet-Type = Access-Request
+
+# Should probably be Do-Not-Respond
+Result-Status == success
+Packet-Type == 0
+Reply-Message == bye-from-timeout-module

diff --git a/src/tests/keywords/finally-timeout-module.servers b/src/tests/keywords/finally-timeout-module.servers
new file mode 100644 (file)
index 0000000..ee07bc7
--- /dev/null
+++ b/src/tests/keywords/finally-timeout-module.servers
@@ -0,0 +1,22 @@
+# Regression test  We'd set the request master state to "Stop Processing" and this
+# would result in module calls in finally being skipped.
+server module-timeout-fin {
+       namespace = radius
+
+       recv Access-Request {
+               # Trigger the overall timeout for the test binary
+               %time.advance(61s)
+               ok
+       }
+
+       finally {
+               if (timeout) {
+                       # This previously resulted in the request processing terminating early
+                       reschedule
+                       reply.Reply-Message := 'bye-from-timeout-module'
+                       success
+               } else {
+                       fail
+               }
+       }
+}

diff --git a/src/tests/keywords/finally-timeout.attrs b/src/tests/keywords/finally-timeout.attrs
new file mode 100644 (file)
index 0000000..71e8875
--- /dev/null
+++ b/src/tests/keywords/finally-timeout.attrs
@@ -0,0 +1,6 @@
+Packet-Type = Access-Request
+
+# Should probably be Do-Not-Respond
+Result-Status == success
+Packet-Type == 0
+Reply-Message == bye-from-timeout-fin

diff --git a/src/tests/keywords/finally-timeout.servers b/src/tests/keywords/finally-timeout.servers
new file mode 100644 (file)
index 0000000..9009da1
--- /dev/null
+++ b/src/tests/keywords/finally-timeout.servers
@@ -0,0 +1,18 @@
+server timeout-fin {
+       namespace = radius
+
+       recv Access-Request {
+               # Trigger the overall timeout for the test binary
+               %time.advance(61s)
+               ok
+       }
+
+       finally {
+               if (timeout) {
+                       reply.Reply-Message := 'bye-from-timeout-fin'
+                       success
+               } else {
+                       fail
+               }
+       }
+}

diff --git a/src/tests/keywords/finally.servers b/src/tests/keywords/finally.servers
index 7c6bcee18cb9e92eca1c625acbb7f78d2eecb851..9224decb87e21e1bdddad573029350ba34e7ea2e 100644 (file)
--- a/src/tests/keywords/finally.servers
+++ b/src/tests/keywords/finally.servers
@@ -26,22 +26,3 @@ server packet-fin {
                fail
        }
 }
-
-server timeout-fin {
-       namespace = radius
-
-       recv Access-Request {
-               # Trigger the overall timeout for the test binary
-               %time.advance(61s)
-               ok
-       }
-
-       finally {
-               if (timeout) {
-                       Tmp-String-0 := 'bye-from-timeout-fin'
-                       success
-               } else {
-                       fail
-               }
-       }
-}