Tcl Source Code

A command is typically available while its deletion is in progress:

namespace eval ns1 {
    proc p1 {oldname newname op} {
        puts [list $oldname signing off!]
    }
    trace add command p1 delete [namespace which p1]
    rename p1 {}
}

Output:

::ns1::p1 signing off!

However, in the case of an imported alias, the result is instead an infinite loop in command dispatch:

namespace eval ns2 {
    namespace export *

    proc p1 {oldname newname op} {
        puts [list $oldname is being deleted!]
    }
    interp alias {} [namespace current]::p2 {} [namespace which p1]
}


namespace eval ns3 {
    namespace import ::ns2::p2
}


proc ondelete {oldname newname op} {
    catch {
        ::ns3::p2 $oldname $newname $op
    } cres
    puts $cres
}


trace add command ::ns2::p2 delete [namespace which ondelete]
rename ns2::p2 {}

Output:

too many nested evaluations (infinite loop?)

What's happening here is that tclNamesp.c:InvokeImportedNRCmd extracts the command structure and passes it to tclBasic.c:TclNREvalObjv, along with the words of the original command, but then tclBasic.c:EvalObjvCore uses the CMD_IS_DELETED flag to decide to discard the command structure that was passed to it, and instead resolves the command name again, which leads back to tclNamesp.c:InvokeImportedNRCmd where the cyle repeats itself.

CMD_IS_DELETED seems to be intended to mean "command deletion in progress", so perhaps the check for it can simply be removed from tclBasic.c:EvalObjvCore.

A script-level workaround is to lookup up the alias and call it directly:

proc ondelete {oldname newname op} {
    set alias [interp alias {} $oldname]
    {*}$alias $oldname $newname $op
}

To put the difference succinctly, sebres' fix treats a deletion trace on a
routine as a post-deletion action, and my fix treats it as a pre-deletion
action.

Although I've "fixed" this new test in the manner how the branch work - invocation of deleting command in trace is prohibited; test case namespace-57.1 is covering "attempt to invoke a deleted command" now, I'm confused a bit.

Related to Tcl 8.6 (before [7eed2baf73fbb1f9]) the fix that Nathan provided for this is rather a regression, because this test-case works exactly so as in my branch ("attempt to invoke a deleted command").

But related to Tcl 8.5 rather old 8.6th handling (and my branch) would be a regression (there it's possible to invoke the command in trace during a deletion)...

So I'd really like to listen the opinion of TCT about the issue.

If the caller of TclNREvalObjv is expected to do its own due diligence, then you are right and there is no need for TclNREvalObjv to perform the checks itself. On the other hand, your fix incorrectly relates the TCL_EVAL_NORESOLVE and CMD_IS_DEAD flags. In [9641d70a9caa2017] I've added a new test to your branch that illustrates the issue: Because it happens to take a path through TclNRInvoke, both the TCL_EVAL_NORESOLVE and and CMD_IS_DEAD flags are set, causing EvalObjvCore to reject the command when it should dispatch it.

I'm agree, just I'm trying to follow where is this new flag CMD_DEAD really expected. For instance see my alternative branch (without CMD_DEAD at all). Everything works, the new test "namespace-57.0" is passed too. So either CMD_DEAD is completely superfluous at the moment or the test "namespace-57.0" is missing something yet (so would work within your commit and would fail in my branch bug-29e8848eb976-alt).

At the moment I can still not follow from the test why this CMD_DEAD is expected at all.

I think my initial report answers that question: A command should be available for calling in any deletion traces that are evaluated during the course of its deletion. CMD_IS_DELETED should have been named CMD_DYING. Before the fix for this ticket, EvalObjvCore interpreted it to mean that the command is no longer available, and that's wrong. During a deletion trace the command is still available, even though it is already flagged as CMD_IS_DELETED. Whem EvalObjvCore makes this incorrect intrepretation, errors like the one illustrated in the initial report can occur.

> I think the new flag is warranted because the Command structure includes a refCount flag, implying that is is meant to be shared and that there should be a way for something holding it to know whether it is still valid.

Sure, just I don't see why in EvalObjvCore it would be important to distinguish between supplied command will be deleted or command is ultimately deleted. As well I don't follow why in case of CMD_IS_DELETED the command should be considered as still valid.

Yes, I created the new CMD_DEAD flag and then inadvertently used the CMD_IS_DELETED instead. Fixed in [68a1f5598fe5b281]. I think the new flag is warranted because the Command structure includes a refCount flag, implying that is is meant to be shared and that there should be a way for something holding it to know whether it is still valid. Maybe instead of the new flag, Tcl_DeleteCommandFromToken could increment the cmdEpoch. That might be a little more costly to evaluate in EvalObjvCore though.

Here is alternative branch without new (unneeded?) flag CMD_DEAD.

@Nathan: Is the fix [7eed2baf73] really fulfilled? Looks a bit strange.

Because a new flag CMD_DEAD checking in tclBasic (EvalObjvCore) now will be nowhere set :) Either you wanted set it in 3271 (instead of CMD_IS_DELETED)... Or it is even enough, and a new flag (as well as a check for deleted) is unneeded at all.

In this case alternative diff (doing the same as your code without new flag CMD_DEAD) relative previous version would be something like that:

@@ -4331,13 +4331,9 @@ EvalObjvCore(
          * Caller gave it to us.
          */

-	if (!(preCmdPtr->flags & CMD_IS_DELETED)) {
-	    /*
-             * So long as it exists, use it.
-             */
-
-	    cmdPtr = preCmdPtr;
-	} else if (flags & TCL_EVAL_NORESOLVE) {
+	if ( (flags & TCL_EVAL_NORESOLVE)
+	  && (preCmdPtr->flags & CMD_IS_DELETED)
+	) {
 	    /*
 	     * When it's been deleted, and we're told not to attempt resolving
 	     * it ourselves, all we can do is raise an error.
@@ -4348,6 +4344,7 @@ EvalObjvCore(
 	    Tcl_SetErrorCode(interp, "TCL", "EVAL", "DELETEDCOMMAND", NULL);
 	    return TCL_ERROR;
 	}
+	cmdPtr = preCmdPtr;
     }
     if (cmdPtr == NULL) {
 	cmdPtr = TEOV_LookupCmdFromObj(interp, objv[0], lookupNsPtr);

Just...
@all: it looks like this code is not covered at all (or I can simply not find the test case covering that).