subrequests/synchronous interpreter: Use the detached done callback to free requests

Use unlang_interpret_signal to manage cleaning up detached requests instead of having custom code inside the subrequest code

diff --git a/src/lib/unlang/interpret.c b/src/lib/unlang/interpret.c
index 06e69c5b9a4bc75f61a258ae649bafcdf5cd5f88..fc23e36dc35836e8fe7d35239c9aca9486d81c1c 100644 (file)
--- a/src/lib/unlang/interpret.c
+++ b/src/lib/unlang/interpret.c
@@ -997,7 +997,7 @@ void unlang_interpret_request_done(request_t *request)
                break;
 
        case REQUEST_TYPE_DETACHED:
-               intp->funcs.done_detached(request, stack->result, intp->uctx);
+               intp->funcs.done_detached(request, stack->result, intp->uctx);  /* Callback will usually free the request */
                break;
        }
 }
@@ -1024,8 +1024,12 @@ void unlang_interpret_request_detach(request_t *request)
 
        if (!fr_cond_assert(stack != NULL)) return;
 
+       if (!request_is_detachable(request)) return;
+
        intp = stack->intp;
        intp->funcs.detach(request, intp->uctx);
+
+       if (request_detach(request) < 0) RPEDEBUG("Failed detaching request");
 }
 
 /** Send a signal (usually stop) to a request
@@ -1123,22 +1127,33 @@ void unlang_interpret_signal(request_t *request, fr_state_signal_t action)
 
        switch (action) {
        case FR_SIGNAL_CANCEL:
-               unlang_interpret_request_stop(request);         /* Stop gets the request in a consistent state */
-               unlang_interpret_request_done(request);         /* Done signals the request is complete */
+               /*
+                *      Detach the request from the parent to cleanup 
+                *      any cross-request pointers.  This is a noop
+                *      if the request is not detachable.
+                */
+               if (request_is_detachable(request)) unlang_interpret_request_detach(request);
 
                /*
-                *      If we're cancelling a child, detach it from
-                *      its parent, so we don't leak memory allocated
-                *      in request_detachable_init.
+                *      Get the request into a consistent state,
+                *      removing it from any runnable lists.
                 */
-               if (request_is_detachable(request) && (request_detach(request) < 0)) {
-                       RPEDEBUG("Failed detaching request on cancel");
-               }
+               unlang_interpret_request_stop(request);
+
+               /*
+                *      As the request is detached, we call the done_detached
+                *      callback which should free the request.
+                */
+               unlang_interpret_request_done(request);
                break;
 
        case FR_SIGNAL_DETACH:
-               unlang_interpret_request_detach(request);       /* Tell our caller that the request is being detached */
-               if (request_detach(request) < 0) RPEDEBUG("Failed detaching request");
+               /*
+                *      Cleanup any cross-request pointers, and mark the
+                *      request as detached.  When the request completes it
+                *      should by automatically freed.
+                */
+               unlang_interpret_request_detach(request);
                break;
 
        default:

diff --git a/src/lib/unlang/interpret_synchronous.c b/src/lib/unlang/interpret_synchronous.c
index a8c881f882561ff739a01578444ea4c6772b5855..7df176e2597a7e0558156231f1f899e84fcb6dbb 100644 (file)
--- a/src/lib/unlang/interpret_synchronous.c
+++ b/src/lib/unlang/interpret_synchronous.c
@@ -84,6 +84,15 @@ static void _request_done_internal(request_t *request, UNUSED rlm_rcode_t rcode,
 static void _request_done_detached(request_t *request, UNUSED rlm_rcode_t rcode, UNUSED void *uctx)
 {
        RDEBUG3("Done synchronous detached request");
+
+       /*
+        *      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.
+        */
+       talloc_free(request);
 }
 
 /** We don't need to do anything for internal -> detached
@@ -278,14 +287,11 @@ rlm_rcode_t unlang_interpret_synchronous(fr_event_list_t *el, request_t *request
                RDEBUG4("<<< interpreter (iteration %i) - %s", iterations,
                        fr_table_str_by_value(rcode_table, sub_rcode, "<INVALID>"));
 
-               if (sub_request == request) {
-                       rcode = sub_rcode;
                /*
-                *      Free detached, resumable requests
+                *      If the request being run was the original request
+                *      then update the rcode we're returning.
                 */
-               } else if (!sub_request->parent && !unlang_interpret_is_resumable(sub_request)) {
-                       talloc_free(sub_request);       /* Free detached requests */
-               }
+               if (sub_request == request) rcode = sub_rcode;
 
                DEBUG3("%u runnable, %u yielded", fr_heap_num_elements(intps->runnable), intps->yielded);
        }

diff --git a/src/lib/unlang/subrequest_child.c b/src/lib/unlang/subrequest_child.c
index 2186c7d7ad078caf8e3ec801ede804b2c1066614..2ae15804e5349895e878939a703103f7a58162df 100644 (file)
--- a/src/lib/unlang/subrequest_child.c
+++ b/src/lib/unlang/subrequest_child.c
@@ -56,21 +56,7 @@ static void unlang_detached_max_request_time(UNUSED fr_event_list_t *el, UNUSED
 
        RDEBUG("Reached Request-Lifetime.  Forcibly stopping request");
 
-       fr_assert(!request->parent);    /* must be detached */
-
-       /*
-        *      The request is scheduled and isn't running.  Remove it
-        *      from the backlog.
-        */
-       if (unlang_request_is_scheduled(request)) {
-               unlang_stack_t          *stack = request->stack;
-               unlang_interpret_t      *intp = stack->intp;
-
-               fr_assert(request_is_internal(request));
-               intp->funcs.done_internal(request, stack->result, intp->uctx);  /* Should free the request */
-       } else {
-               talloc_free(request);
-       }
+       unlang_interpret_signal(request, FR_SIGNAL_CANCEL);     /* Request should now be freed */
 }
 
 /** Initialize a detached child
@@ -127,6 +113,12 @@ static void unlang_subrequest_child_signal(request_t *request, fr_state_signal_t
 {
        unlang_frame_state_subrequest_t *state;
 
+       /*
+        *      We're already detached so we don't
+        *      need to notify the parent we're
+        *      waking up, and we don't need to detach
+        *      again...
+        */
        if (!request->parent) return;
 
        state = talloc_get_type_abort(frame_current(request->parent)->state, unlang_frame_state_subrequest_t);
@@ -155,7 +147,7 @@ static void unlang_subrequest_child_signal(request_t *request, fr_state_signal_t
                 *      Indicate to the parent there's no longer a child
                 */
                state->child = NULL;
-
+               
                /*
                 *      Tell the parent to resume
                 */