From: Arran Cudbard-Bell Date: Mon, 13 May 2024 18:53:59 +0000 (-0600) Subject: Fix time tracking assert when the server is blocked processing children X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=33c59091930de01d83c9bad23f24de6726c72905;p=thirdparty%2Ffreeradius-server.git Fix time tracking assert when the server is blocked processing children --- diff --git a/src/bin/radiusd.c b/src/bin/radiusd.c index 903d40a4e5c..5c947964b09 100644 --- a/src/bin/radiusd.c +++ b/src/bin/radiusd.c @@ -1223,7 +1223,7 @@ static void sig_fatal(int sig) * Suppress duplicate signals. * * For some reason on macOS we get multiple signals - * for the same event (SIGTERM). + * for the same event (SIGINT). * * ...this also fixes the problem of the user hammering * Ctrl-C and causing ungraceful exits as we try and diff --git a/src/lib/server/request.h b/src/lib/server/request.h index b11f5648140..1b2934f4a4f 100644 --- a/src/lib/server/request.h +++ b/src/lib/server/request.h @@ -58,9 +58,9 @@ extern "C" { #endif typedef enum { - REQUEST_ACTIVE = 1, - REQUEST_STOP_PROCESSING, - REQUEST_COUNTED + REQUEST_ACTIVE = 1, //!< Request is active (running or runnable) + REQUEST_STOP_PROCESSING, //!< Request has been signalled to stop + REQUEST_DONE, //!< Request has completed } request_master_state_t; #define REQUEST_MASTER_NUM_STATES (REQUEST_COUNTED + 1) @@ -221,13 +221,14 @@ struct request_s { char const *component; //!< Section the request is in. char const *module; //!< Module the request is currently being processed by. - fr_packet_t *packet; //!< Incoming request. - fr_packet_t *reply; //!< Outgoing response. + fr_packet_t *packet; //!< Incoming request. + fr_packet_t *reply; //!< Outgoing response. fr_client_t *client; //!< The client that originally sent us the request. request_master_state_t master_state; //!< Set by the master thread to signal the child that's currently //!< working with the request, to do something. + bool counted; //!< Set if the request has been counted in the stats. rlm_rcode_t rcode; //!< Last rcode returned by a module diff --git a/src/lib/server/stats.c b/src/lib/server/stats.c index 82c71e75b63..0349e3853f6 100644 --- a/src/lib/server/stats.c +++ b/src/lib/server/stats.c @@ -47,7 +47,7 @@ fr_stats_t radius_acct_stats = FR_STATS_INIT; void request_stats_final(request_t *request) { - if (request->master_state == REQUEST_COUNTED) return; + if (request->counted) return; #if 0 if (!request->listener) return; @@ -143,7 +143,7 @@ void request_stats_final(request_t *request) break; } - request->master_state = REQUEST_COUNTED; + request->counted = true; } void radius_stats_init(int flag) diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c index 3873376d28a..a8ba0eaa51a 100644 --- a/src/lib/unlang/interpret.c +++ b/src/lib/unlang/interpret.c @@ -1081,6 +1081,8 @@ void unlang_interpret_request_done(request_t *request) intp->funcs.done_detached(request, stack->result, intp->uctx); /* Callback will usually free the request */ break; } + + request->master_state = REQUEST_DONE; } static inline CC_HINT(always_inline) @@ -1312,6 +1314,13 @@ bool unlang_request_is_cancelled(request_t const *request) return (request->master_state == REQUEST_STOP_PROCESSING); } +/** Return whether a request has been marked done + */ +bool unlang_request_is_done(request_t const *request) +{ + return (request->master_state == REQUEST_DONE); +} + /** Check if a request as resumable. * * @param[in] request The current request. diff --git a/src/lib/unlang/interpret.h b/src/lib/unlang/interpret.h index 8e47bd45338..12740dbb456 100644 --- a/src/lib/unlang/interpret.h +++ b/src/lib/unlang/interpret.h @@ -152,6 +152,8 @@ bool unlang_request_is_scheduled(request_t const *request); bool unlang_request_is_cancelled(request_t const *request); +bool unlang_request_is_done(request_t const *request); + void unlang_interpret_request_done(request_t *request); void unlang_interpret_mark_runnable(request_t *request); diff --git a/src/lib/unlang/subrequest.c b/src/lib/unlang/subrequest.c index bc1a93851ce..105a806e07d 100644 --- a/src/lib/unlang/subrequest.c +++ b/src/lib/unlang/subrequest.c @@ -35,8 +35,8 @@ RCSID("$Id$") /** Send a signal from parent request to subrequest * */ -static void unlang_subrequest_parent_signal(UNUSED request_t *request, unlang_stack_frame_t *frame, - fr_signal_t action) +static void unlang_subrequest_signal_child(UNUSED request_t *request, unlang_stack_frame_t *frame, + fr_signal_t action) { unlang_frame_state_subrequest_t *state = talloc_get_type_abort(frame->state, unlang_frame_state_subrequest_t); request_t *child = talloc_get_type_abort(state->child, request_t); @@ -50,6 +50,27 @@ static void unlang_subrequest_parent_signal(UNUSED request_t *request, unlang_st */ fr_assert(action != FR_SIGNAL_DETACH); + /* + * If the server is stopped, inside a breakpoint, + * whilst processing a child, on resumption both + * requests (parent and child) may need to be + * cancelled as they've both hit max request_time. + * + * Sometimes the child will run to completion before + * the cancellation is processed, but the parent + * will still be cancelled. + * + * When the parent is cancelled this function is + * executed, which will signal an already stopped + * child to cancel itself. + * + * This triggers asserts in the time tracking code. + * + * ...so we check to see if the child is done before + * sending a signal. + */ + if (unlang_request_is_done(child)) return; + /* * Forward other signals to the child */ @@ -277,7 +298,7 @@ int unlang_subrequest_op_init(void) &(unlang_op_t){ .name = "subrequest", .interpret = unlang_subrequest_parent_init, - .signal = unlang_subrequest_parent_signal, + .signal = unlang_subrequest_signal_child, .debug_braces = true, .frame_state_size = sizeof(unlang_frame_state_subrequest_t), .frame_state_type = "unlang_frame_state_subrequest_t", diff --git a/src/lib/unlang/subrequest_child.c b/src/lib/unlang/subrequest_child.c index 9e8bd9f9758..365c98c1f2e 100644 --- a/src/lib/unlang/subrequest_child.c +++ b/src/lib/unlang/subrequest_child.c @@ -143,6 +143,8 @@ static void unlang_subrequest_child_signal(request_t *request, fr_signal_t actio FALL_THROUGH; case FR_SIGNAL_CANCEL: + RDEBUG3("Removing subrequest from parent, and marking parent as runnable"); + /* * Indicate to the parent there's no longer a child */