From: Arran Cudbard-Bell Date: Mon, 29 Mar 2021 14:40:48 +0000 (+0100) Subject: Use a single code path when stopping requests X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=4a4e921b3eaabc8a8b9ee2d8758e856a7ba22ca6;p=thirdparty%2Ffreeradius-server.git Use a single code path when stopping requests Instead of a bunch of them, and miss critical cleanup phases --- diff --git a/src/lib/io/worker.c b/src/lib/io/worker.c index c28228b1163..6420e99984e 100644 --- a/src/lib/io/worker.c +++ b/src/lib/io/worker.c @@ -369,16 +369,25 @@ static void worker_nak(fr_worker_t *worker, fr_channel_data_t *cd, fr_time_t now * Modules and unlang keywords explicitly register signal handlers to deal * with their yield points being cancelled/interrupted via this function. * - * @param[in] request to cancel. + * The caller should assume the request is no longer viable after calling + * this function. + * + * @param[in] request_p Pointer to the request to cancel. + * Will be set to NULL. * @param[in] now The current time. */ -static void worker_stop_request(request_t *request, fr_time_t now) +static void worker_stop_request(request_t **request_p, fr_time_t now) { - if (request->async->tracking.state == FR_TIME_TRACKING_YIELDED) { - fr_time_tracking_resume(&request->async->tracking, now); + if ((*request_p)->async->tracking.state == FR_TIME_TRACKING_YIELDED) { + fr_time_tracking_resume(&(*request_p)->async->tracking, now); } - unlang_interpret_signal(request, FR_SIGNAL_CANCEL); + /* + * Also marks the request as done and runs + * the internal/external callbacs. + */ + unlang_interpret_signal(*request_p, FR_SIGNAL_CANCEL); + *request_p = NULL; } /** Enforce max_request_time @@ -394,7 +403,7 @@ static void worker_stop_request(request_t *request, fr_time_t now) static void worker_max_request_time(UNUSED fr_event_list_t *el, UNUSED fr_time_t when, void *uctx) { fr_time_t now = fr_time(); - request_t *request; + request_t *request; fr_worker_t *worker = talloc_get_type_abort(uctx, fr_worker_t); /* @@ -414,17 +423,7 @@ static void worker_max_request_time(UNUSED fr_event_list_t *el, UNUSED fr_time_t * Waiting too long, delete it. */ REDEBUG("Request has reached max_request_time - signalling it to stop"); - worker_stop_request(request, now); - - /* - * Tell the network side that this request is - * done. Also make sure that each time stamp is - * unique. - */ - worker_send_reply(worker, request, 1, now); - - DEBUG3("Freeing request %s", request->name); - talloc_free(request); + worker_stop_request(&request, now); } /* @@ -441,7 +440,7 @@ static void worker_max_request_time(UNUSED fr_event_list_t *el, UNUSED fr_time_t static void worker_max_request_timer(fr_worker_t *worker) { fr_time_t cleanup; - request_t *request; + request_t *request; /* * No more requests, delete the timer. @@ -656,7 +655,6 @@ static inline CC_HINT(always_inline) void worker_request_init(fr_worker_t *worker, request_t *request, fr_time_t now) { request->el = worker->el; - request->backlog = worker->runnable; MEM(request->packet = fr_radius_packet_alloc(request, false)); request->packet->timestamp = now; @@ -819,11 +817,8 @@ nak: */ RWARN("Got conflicting packet for request (%" PRIu64 "), telling old request to stop", old->number); - worker_stop_request(old, now); - fr_assert(worker->num_active > 0); - worker->num_active--; + worker_stop_request(&old, now); worker->stats.dropped++; - talloc_free(old); insert_new: (void) rbtree_insert(worker->dedup, request); @@ -901,8 +896,7 @@ void fr_worker_destroy(fr_worker_t *worker) DEBUG("Worker is exiting - telling request %s to stop", request->name); count++; } - worker_stop_request(request, now); - talloc_free(request); + worker_stop_request(&request, now); } fr_assert(fr_heap_num_elements(worker->runnable) == 0); @@ -945,6 +939,7 @@ static void _worker_request_internal_init(request_t *request, void *uctx) /** Internal request (i.e. one generated by the interpreter) is now complete * + * Whatever generated the request is now responsible for freeing it. */ static void _worker_request_internal_done(request_t *request, UNUSED rlm_rcode_t rcode, void *uctx) { @@ -953,8 +948,17 @@ static void _worker_request_internal_done(request_t *request, UNUSED rlm_rcode_t worker_request_time_tracking_end(worker, request, now); - DEBUG3("Freeing request %s", request->name); - talloc_free(request); + fr_assert(!fr_heap_entry_inserted(request->runnable_id)); + fr_assert(!fr_heap_entry_inserted(request->time_order_id)); + + /* + * Detached requests have to be freed by us + * as nothing else can free them. + * + * All other requests must be freed by the + * code which allocated them. + */ + if (!request->parent) talloc_free(request); } /** External request is now complete @@ -990,9 +994,7 @@ static void _worker_request_external_done(request_t *request, UNUSED rlm_rcode_t RDEBUG("Done request"); - worker_send_reply(worker, request, 0, fr_time()); - - DEBUG3("Worker - Request freed (%s)", request->name); + worker_send_reply(worker, request, request->master_state == REQUEST_STOP_PROCESSING ? 1 : 0, fr_time()); talloc_free(request); } @@ -1003,7 +1005,7 @@ static void _worker_request_runnable(request_t *request, void *uctx) { fr_worker_t *worker = uctx; - RDEBUG3("Worker - Request marked as runnable"); + RDEBUG3("Request marked as runnable"); fr_heap_insert(worker->runnable, request); } @@ -1012,7 +1014,7 @@ static void _worker_request_runnable(request_t *request, void *uctx) */ static void _worker_request_yield(request_t *request, UNUSED void *uctx) { - RDEBUG3("Worker - Request yielded"); + RDEBUG3("Request yielded"); fr_time_tracking_yield(&request->async->tracking, fr_time()); } @@ -1021,7 +1023,7 @@ static void _worker_request_yield(request_t *request, UNUSED void *uctx) */ static void _worker_request_resume(request_t *request, UNUSED void *uctx) { - RDEBUG3("Worker - Request resuming"); + RDEBUG3("Request resuming"); fr_time_tracking_resume(&request->async->tracking, fr_time()); } @@ -1071,8 +1073,7 @@ static inline CC_HINT(always_inline) void worker_run_request(fr_worker_t *worker * just stop the request and free it. */ if (request->async->channel && !fr_channel_active(request->async->channel)) { - worker_stop_request(request, now); - talloc_free(request); + worker_stop_request(&request, now); return; } diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c index 9a091614717..9fe02ce9742 100644 --- a/src/lib/unlang/interpret.c +++ b/src/lib/unlang/interpret.c @@ -108,6 +108,19 @@ static void stack_dump(request_t *request) #define DUMP_STACK #endif +static inline CC_HINT(always_inline) +void request_done(request_t *request) +{ + unlang_stack_t *stack = request->stack; + unlang_interpret_t *intp = stack->intp; + + if (request_is_internal(request)) { + intp->funcs.done_internal(request, stack->result, intp->uctx); + } else { + intp->funcs.done_external(request, stack->result, intp->uctx); + } +} + /** Push a new frame onto the stack * * @param[in] request to push the frame onto. @@ -683,13 +696,8 @@ CC_HINT(hot) rlm_rcode_t unlang_interpret(request_t *request) * This usually means the request is complete in its * entirety. */ - if (stack->depth == 0) { - if (request_is_internal(request)) { - intp->funcs.done_internal(request, stack->result, intp->uctx); - } else { - intp->funcs.done_external(request, stack->result, intp->uctx); - } - } + if (stack->depth == 0) request_done(request); + return rcode; } @@ -860,13 +868,9 @@ void unlang_interpret_signal(request_t *request, fr_state_signal_t action) /* * If we're stopping, then mark the request as stopped. - * Then, call the frame signal handler. The keyword will - * then take care of calling us for any children, which - * will then be marked as STOP_PROCESSING. + * Then, call the frame signal handler. */ - if (action == FR_SIGNAL_CANCEL) { - request->master_state = REQUEST_STOP_PROCESSING; - } + if (action == FR_SIGNAL_CANCEL) request->master_state = REQUEST_STOP_PROCESSING; /* * Requests that haven't been run through the interpret @@ -874,6 +878,8 @@ void unlang_interpret_signal(request_t *request, fr_state_signal_t action) * need to do anything. */ if (stack && (stack->depth > 0)) frame_signal(request, action, 0); + + if (action == FR_SIGNAL_CANCEL) request_done(request); } /** Return the depth of the request's stack