From: SourceForge.net <no...@so...> - 2004-08-26 23:02:07
|
Bugs item #652765, was opened at 2002-12-12 08:49 Message generated for change (Comment added) made by duquette You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=652765&group_id=10894 Category: 45. Traces Group: obsolete: 8.4.1 Status: Open Resolution: None Priority: 5 Submitted By: Nobody/Anonymous (nobody) Assigned to: Don Porter (dgp) Summary: errors disappear in trace callbacks Initial Comment: Errors occuring in "trace add command" callbacks (i.e., rename and delete callbacks) are swallowed by Tcl instead of being propagated to the application. wi...@wj... ---------------------------------------------------------------------- Comment By: Will Duquette (duquette) Date: 2004-08-26 16:02 Message: Logged In: YES user_id=372859 I think Donal's suggestion, that the errors are propagated but that the rename succeeds, makes a lot of sense. IF some background mechanism is used, though, it will need to be clearly documented in the trace.n man page. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2004-08-26 03:26 Message: Logged In: YES user_id=79902 Note that delete traces have the same properties; both kinds are notifications to be done after the fact. ---------------------------------------------------------------------- Comment By: Vince Darley (vincentdarley) Date: 2004-08-26 03:07 Message: Logged In: YES user_id=32170 Totally agree with dkf's summary. Only remaining question is how do we carry out (2)? (Note: this is a very similar problem to that faced by the tclvfs extension, which uses 'vfs::filesystem internalerror $cmdprefix' to specify a command prefix to call whenever there's an error). One possibility would be to use the 'tclLog' proc which appears to be built into Tcl. I'm not really sure what 'tclLog' is documented to do, but it does exist! (In fact it might be a good idea of tclvfs to use it instead of its own ad-hoc approach). ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2004-08-26 02:02 Message: Logged In: YES user_id=79902 Summing up: 1) Do we propagate errors? 2) If we propagate errors, do we do this directly or via some kind of background mechanism? 3) If we propagate errors directly, do we permit them to abort the renaming or prevent other rename traces from getting called? IMO: 1) Yes 2) bgerror is good, but other can work 3) the error should *not* abort the rename ---------------------------------------------------------------------- Comment By: Vince Darley (vincentdarley) Date: 2004-08-26 01:26 Message: Logged In: YES user_id=32170 Sorry, I disagree. If errors are swallowed then we have a well-defined, consistent behaviour: command renames and deletes always succeed. Therefore all trace callbacks can assume that is true. Of course this doesn't mean that a badly written callback can't accidentally throw an error and therefore not succeed and therefore leave _itself_ in an inconsistent state. But that is an example of the programmer causing themselves a problem. However, if errors propagate, then the example I gave shows how one trace callback can abort a 'rename' and therefore cause problems to another trace callback which doesn't know the rename was aborted. This means that one programmer can shoot another programmer in the foot. That's the inconsistency I don't like. There's a distinction between 'your program being in an inconsistent state' and 'this piece of code is in an inconsistent state not because it contains any bugs but because a totally different piece of code contains bugs'. It's also why I like the 'bgerror' solution because it avoids this inconsistency. (But your points about the event loop are good, and mean we effectively have to rule that solution out). Now, it may well be that allowing this inconsistency is the best way forward, trading it off for better debugging (i.e. receiving an error message), but it would be nice if we can find a more consistent approach. ---------------------------------------------------------------------- Comment By: Will Duquette (duquette) Date: 2004-08-25 16:48 Message: Logged In: YES user_id=372859 Vince said: "So, as I've said, I'm all for not swallowing errors, but I'm not very keen on inconsistent states either." Precisely. If a bug in a rename trace callback causes an error to be thrown, and it gets swallowed, then very likely your program is now in an inconsistent state. That is, it's in an inconsistent state whether the error is swallowed or not--but if the error isn't swallowed, the developer is more likely to catch it and fix it before the software is released. ---------------------------------------------------------------------- Comment By: Vince Darley (vincentdarley) Date: 2004-08-25 07:35 Message: Logged In: YES user_id=32170 I agree it is undesirable to swallow errors, but the suggested approach can still lead to inconsistencies. Looking closely, the current variable tracing approach also has a similar problem, for example: set foo 3 trace add variable foo write {puts "1: foo is $foo" ; #} trace add variable foo write {set foo 3 ; error "don't change" ; #} trace add variable foo write {puts "2: foo is $foo" ; #} If I now try to 'set foo 2', the last trace fires and thinks that foo is 2, but then the next trace actually sets foo to 3 and throws an error, aborting the change. The upshot is that one of the traces was confused, and believed the variable was set to 2 when it wasn't. A similar, but more fundamental problem will occur with rename traces, if we allow them to be aborted by an error. One trace can think something has been renamed, but then the next trace aborts the rename. Now we are in an inconsistent state. So, as I've said, I'm all for not swallowing errors, but I'm not very keen on inconsistent states either. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2004-08-25 07:27 Message: Logged In: YES user_id=79902 There are a few other places where errors are silently gobbled and IMO they're all bugs. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2004-08-25 07:20 Message: Logged In: YES user_id=80530 I think I have to disagree on part of that. It's a property of Tcl commands that they can be renamed. A package that wants to prevent the renaming of its commands seems buggy to me. Generally, though, I agree that it's best to avoid swallowing and silently hiding errors when we can. ---------------------------------------------------------------------- Comment By: Will Duquette (duquette) Date: 2004-08-25 07:15 Message: Logged In: YES user_id=372859 Vince, Using bgerror really only works well if you're running an app with an event loop. There are lots of Tcl applications that don't, and Snit (my particular interest) can be used in both. So I can't just use bgerror to flag errors in Snit's rename trace callback. You ask "which of these two is it?" Am I trying to discover programming errors, or am I trying to signal conditions that a programmer might wish to catch? The answer is, "both". I like Tcl's error handling; I really dislike having code in which errors can occur silently. For that reason, I'm philosophically against the current implementation, as it's the one place (that I'm aware of) in which errors are ignored without recourse. (Bgerror is not a real recourse, for the reason I stated above.) But in any event, it's a false dichotomy. If I have a module which--for whatever reason--contains commands that should not be renamed, it seems reasonable to use a rename trace to catch attempts to rename them, and throw an error if they do, just as some folks use variable traces to control what can be assigned to particular variables. If errors are thrown in these cases, it's probably not a "condition which a programmer might wish to catch and workaround"; it's a logical error in the program that has to be fixed. Will ---------------------------------------------------------------------- Comment By: Vince Darley (vincentdarley) Date: 2004-08-25 02:29 Message: Logged In: YES user_id=32170 Will, If your purpose is just to discover programming errors, then that's one thing (which could also be handled by dgp's suggestion of calling bgerror with the problem -- in fact calling bgerror might be better, because the programmer would not be tempted to use 'catch' to hide the problem). However, earlier you gave an example of using a trace in tcllib to prevent renaming of an structure. That's certainly not just to discover programming errors. It's to use the error to signal a certain condition, which a programmer might wish to catch and workaround. So which of these two is it? ---------------------------------------------------------------------- Comment By: Will Duquette (duquette) Date: 2004-08-23 17:24 Message: Logged In: YES user_id=372859 By "propagate normally", I mean that if rename SomeName SomeOtherName causes a trace to throw an error, then the error propagates up and is returned by "rename". I think the concern about multiple traces is misleading. After all, why would a rename callback throw an error? I can think of two reasons: 1. The callback threw an error explicitly, becausing renaming the command will cause the program to misbehave in mysterious ways. 2. There's an error in the callback, meaning it didn't do what it was supposed to do, meaning that the program is likely to misbehave in mysterious ways. In either case, there's probably no good way to recover from the error; it needs to be fixed. Better that the programmer find out ASAP, while they are developing the software. But as it is, such errors hide and can be monstrously difficult to debug. Regarding using a more permanent token rather than the command name--well, yes, obviously. If you can. ---------------------------------------------------------------------- Comment By: Don Porter (dgp) Date: 2004-08-23 10:14 Message: Logged In: YES user_id=80530 Is the real problem here one of using a tranisient, user-modifiable thing -- the command's name -- rather than a more permanent token of some kind as the index against which to store data? Simliar to having a Tcl_ObjCmdProc that changes behavior based on the value of objv[0] rather than storing and behavior controlling value in the ClientData ? Not that I wouldn't like a sensible solution to this problem, but the motivating example here might be a subtle instance of pilot error. ---------------------------------------------------------------------- Comment By: Vince Darley (vincentdarley) Date: 2004-08-23 06:10 Message: Logged In: YES user_id=32170 In Will's example, I would've said a more obvious use of rename traces in struct::stack would be to rename the array entries so that the code still works after rename. I still don't quite understand _exactly_ what Will (or anyone) means by 'if ... propagates normally'? What happens if one rename trace succeeds but then the next one throws an error? In Will's example, let's say there were two traces. Now, if the first trace succeeds (causing some side effect of renaming appropriate associated information), but a second trace fails, then the original rename will not take place and the first trace will actually have done the wrong thing. Can someone please define a consistent behaviour, if they have one in mind that is different to what we have now? ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2004-08-23 03:42 Message: Logged In: YES user_id=79902 One possibility I've used elsewhere would be to report traces using the background error message mechanism. As long as people have a [bgerror] command defined (and are running the event loop), the error message will go somewhere where it can be picked up... ---------------------------------------------------------------------- Comment By: Will Duquette (duquette) Date: 2004-08-21 11:14 Message: Logged In: YES user_id=372859 Here's an example of where the current code causes a problem: in object code (e.g., Tcllib's struct::stack) the object name is often used as an index into array in which the specific object's data is stored. If the object is renamed, this breaks. An obvious use of a rename trace is to throw an error if the object is renamed. At present, you can't do that and make it stick without installing a modified version of the "rename" command. Look at it another way--if errors in rename and delete callbacks propagated normally, the programmer could achieve the current behavior using "catch". With the code as it is, the programmer has no choice. This is not academic, either; Snit relies on the rename/destroy callbacks. ---------------------------------------------------------------------- Comment By: Vince Darley (vincentdarley) Date: 2003-06-26 06:43 Message: Logged In: YES user_id=32170 Here are the basic options: (i) If rename/delete always delete the command/proc in question. This implies either all errors in traces must be ignored, or an error in a trace stops the execution of any other trace, but lets the delete still happen afterwards anyway. In any case, if the command in question has a 'Tcl_CmdDeleteProc' which itself wants to create an error message, that error would overwrite any error thrown by a trace. (ii) If rename/delete are (like variable 'unset' traces) allowed to fail with an error, then things are conceptually much cleaner: the first trace which throws an error aborts the operation. However, this is an overall change in Tcl's documented behaviour, and would require quite careful coding to get right. ---------------------------------------------------------------------- Comment By: Hemang Lavana (hemanglavana) Date: 2003-01-17 05:18 Message: Logged In: YES user_id=81875 I think it should bail out at the first occurence of an error in a callback proc. Not doing so will make it very difficult to find/debug such errors. Moreover, execution traces bail out as soon as an error is encountered and the behavior should be the same for rename/delete traces as well. Do we need a tip to make this change? I already have a patch to fix this ... I just need to add few test cases to trace.test and update the docs also. ---------------------------------------------------------------------- Comment By: Vince Darley (vincentdarley) Date: 2003-01-17 03:32 Message: Logged In: YES user_id=32170 This is true, and the code quite explicitly ignores all errors. This is not documented however. I quite open about what the correct behaviour should be (as long as we then document it), but there is one major problem with not swallowing errors: rename and delete operations are defined to succeed always, irrespective of any errors thrown. So, what do we do if there are two traces, each of which throw an error? We discard the first error message? What if the first trace throws an error and the second is ok? Do we still discard the error? In some ways the current behaviour is the most consistent. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=110894&aid=652765&group_id=10894 |