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 c28228b1163e2294afcfe523c7dd0b0034922096..6420e99984e03612a4db8db67176b749bc05e154 100644 (file)
--- 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 9a09161471726bb80f06582c2767c11dfaf3eaad..9fe02ce974252babfadd89635a24a58028109b83 100644 (file)
--- 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