Re: macros - max recursion depth
Hi Supun, Great work! I've gone ahead and committed your patch. I especially appreciate your attention to detail in including a good unit test and following our coding style. I found some minor issues, which I changed before committing. I list them below for future reference. Probably in the future I may bounce some of these back to you for tweaks on your end. (1) I changed the property name to velocimacro.max.depth to match similar property directive.parse.max.depth. (2) Since the patch deals with calling depth, not recursion, I changed all references in comments from recursion to calling depth. (3) In VelocimacroTestCase, there was a subtle reliance on JDK 1.5. (Remember, we require max JDK 1.4 for compiling and 1.3 for running). An int was passed as a parameter when an Integer was expected. Recommendation: set your IDE to enforce JDK 1.4 syntax. (4) In VelocimacroTestCase, one of the tests was repeated. (deleted the extra) (5) You can't change the class Directive, it's one of the public APIs. (yes, this is vague). Basically, any class that can be extended or an interface that can be implemented by changing velocity.properties has to keep the same API. Luckily, since MacroOverflowException is a subclass of RuntimeException you don't need to declare it in the Directive.render() method. WILL On 6/6/07, Will Glass-Husain [EMAIL PROTECTED] wrote: sure, please do! On 6/6/07, Supun Kamburugamuva [EMAIL PROTECTED] wrote: Hi, +1 for velocimacro.depth.max. I have alomost completed the implementation. Can I submit a patch? I'm sure there are lot of things to consider. Supun. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- Forio Business Simulations Will Glass-Husain [EMAIL PROTECTED] www.forio.com -- Forio Business Simulations Will Glass-Husain [EMAIL PROTECTED] www.forio.com
Re: macros - max recursion depth
Hi, +1 for velocimacro.depth.max. I have alomost completed the implementation. Can I submit a patch? I'm sure there are lot of things to consider. Supun. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: macros - max recursion depth
sure, please do! On 6/6/07, Supun Kamburugamuva [EMAIL PROTECTED] wrote: Hi, +1 for velocimacro.depth.max. I have alomost completed the implementation. Can I submit a patch? I'm sure there are lot of things to consider. Supun. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- Forio Business Simulations Will Glass-Husain [EMAIL PROTECTED] www.forio.com
Re: macros - max recursion depth
Hi Supun, You can see the current list of properties in RuntimeConstants.java How about velocimacro.depth.max? I wouldn't use the word recursion since this applies to macro calls in general. WILL On 6/4/07, Supun Kamburugamuva [EMAIL PROTECTED] wrote: Hi, What should be the property name for defining max number of macro calls. Supun. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- Forio Business Simulations Will Glass-Husain [EMAIL PROTECTED] www.forio.com
Re: macros - max recursion depth
I'm not sure what is the correct place to keep track of macros. I'm keeping track of Macro execution in VelociMacroFactory. Is it sufficient to use the current template name and the macro name to uniquely identify a macro in the runtime? Supun. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: macros - max recursion depth
Hi Supun, I think that makes sense. It's probably broader than is actually supported (for example, the Velocimacros can be configured so that a macro in one page does not overwrite the global macros), but it satisfies the constraint of uniqueness. Keeping track of this in the factory is fine. I was thinking you could add a method to the internal context object, but your suggestion seems better. When you have something ready to share, post a patch on JIRA and we can talk about it. Remember that ultimately you'll need: -- code to implement issue -- new test for issue -- old tests (e.g. ant test) must pass -- code should follow standard style guidelines documented on the Wiki. WILL On 6/3/07, Supun Kamburugamuva [EMAIL PROTECTED] wrote: I'm not sure what is the correct place to keep track of macros. I'm keeping track of Macro execution in VelociMacroFactory. Is it sufficient to use the current template name and the macro name to uniquely identify a macro in the runtime? Supun. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- Forio Business Simulations Will Glass-Husain [EMAIL PROTECTED] www.forio.com
Re: macros - max recursion depth
On 6/1/07, Will Glass-Husain [EMAIL PROTECTED] wrote: I agree with Claude. It seems there are two approaches -- track max recursion: for each macro call in a sequence, track the number of times you call it. (basically, maintain a map of macros with a counter for each) -- track embedded macro calls: track a single number of the macro call depth (forget recursion, just track macro calls within macro calls) I'm not sure what is the correct place to keep track of macros. Supun. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: macros - max recursion depth
I think the first challenge is to discover recursion. I was thinking about discovering recursion when a macro call is being executed. When the node tree is rendered lets say at some point it reaches a macro. At this point, if I can keep track of whether children of this node calls the same macro again before the original call finishes, I will be able to discover recursion. I have tried this method in several places i.e. ASTDirectve.render and VelocimacroProxy.render by keeping track of the state in ASTDirective class and VelocimacroProxy class. First time render method is called by a Macro invocation I kept the status in a private variable and when the next call occurs I checked this variable to see weather the previous macro call is completed. But then I realized that objects from these classes are created every time a call occurs to a macro. So I cannot keep track of the macro calling state in these classes. Supun. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: macros - max recursion depth
If it's easier to implement, it may be pertinent to just implement a max macro stack depth. Claude Le vendredi 01 juin 2007 à 09:21 -0700, Supun Kamburugamuva a écrit : I think the first challenge is to discover recursion. I was thinking about discovering recursion when a macro call is being executed. When the node tree is rendered lets say at some point it reaches a macro. At this point, if I can keep track of whether children of this node calls the same macro again before the original call finishes, I will be able to discover recursion. I have tried this method in several places i.e. ASTDirectve.render and VelocimacroProxy.render by keeping track of the state in ASTDirective class and VelocimacroProxy class. First time render method is called by a Macro invocation I kept the status in a private variable and when the next call occurs I checked this variable to see weather the previous macro call is completed. But then I realized that objects from these classes are created every time a call occurs to a macro. So I cannot keep track of the macro calling state in these classes. Supun. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: macros - max recursion depth
I agree with Claude. It seems there are two approaches -- track max recursion: for each macro call in a sequence, track the number of times you call it. (basically, maintain a map of macros with a counter for each) -- track embedded macro calls: track a single number of the macro call depth (forget recursion, just track macro calls within macro calls) I'd be happy with the latter item. If this is set to a sufficiently high number (20?) this prevents runaway macros yet will not limit typical use. WILL On 6/1/07, Claude Brisson [EMAIL PROTECTED] wrote: If it's easier to implement, it may be pertinent to just implement a max macro stack depth. Claude Le vendredi 01 juin 2007 à 09:21 -0700, Supun Kamburugamuva a écrit : I think the first challenge is to discover recursion. I was thinking about discovering recursion when a macro call is being executed. When the node tree is rendered lets say at some point it reaches a macro. At this point, if I can keep track of whether children of this node calls the same macro again before the original call finishes, I will be able to discover recursion. I have tried this method in several places i.e. ASTDirectve.render and VelocimacroProxy.render by keeping track of the state in ASTDirective class and VelocimacroProxy class. First time render method is called by a Macro invocation I kept the status in a private variable and when the next call occurs I checked this variable to see weather the previous macro call is completed. But then I realized that objects from these classes are created every time a call occurs to a macro. So I cannot keep track of the macro calling state in these classes. Supun. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- Forio Business Simulations Will Glass-Husain [EMAIL PROTECTED] www.forio.com
Re: macros - max recursion depth
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Supin, Supun Kamburugamuva wrote: I think the first challenge is to discover recursion. I was thinking about discovering recursion when a macro call is being executed. This probably won't work. Just because a macro calls itself doesn't mean that infinite recursion will result. I have several macros that are self-calling, but I take care to avoid infinite recursion. It's the infinite case (or, nearly infinite... let's say 100 levels) that you want to avoid instead of throwing a StackOverflowError. - -chris -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.7 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGYGIK9CaO5/Lv0PARAkJpAJ9OmlFBKmlU6UT+u1jT0GuKPHcp0QCeMLIK igG06kPto/AsFTO1NzrmdrM= =kzdS -END PGP SIGNATURE- - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: macros - max recursion depth
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Will, Will Glass-Husain wrote: I'd be happy with the latter item. If this is set to a sufficiently high number (20?) this prevents runaway macros yet will not limit typical use. Can I ask a stupid question: what is the problem we're trying to solve? Are we trying to avoid getting a stack overflow from runaway recursion? If so, what are we going to replace it with? I see we have two options: 1. Simply deny the macro call or 2. Throw another exception (MacroDepthExceededException?) The way I see it, neither of these options is any better than simply allowing the stack overflow to occur. Am I crazy? - -chris -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.7 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGYGKj9CaO5/Lv0PARAq/rAKCjkbaIRIYas8AysrzDO+hQora+twCfSxix 7OuLJWdY58pbW8hLR6z8A6Y= =hWSQ -END PGP SIGNATURE- - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: macros - max recursion depth
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Nathan, Nathan Bubna wrote: 2. Throw another exception (MacroDepthExceededException?) The way I see it, neither of these options is any better than simply allowing the stack overflow to occur. Stack overflows can be caused by many things. Throwing a MacroDepthException is much more informative, and in the case of 3rd party templates being introduced to a running system, can prevent DOS type stuff. Yeah... as I was typing that question, I was thinking well, stack overflow could mean many things, although I immediately assume that my template has infinite recursion in these cases ;) I hasn't really thought about 3rd-party templates. Does anyone have any data on the impact of a stack overflow on a running app server? I would imagine that a better way to perform a DOS would be to concatenate strings forever in an endless loop. There's really no checking that can be done against that. Okay. Enough nay-saying from me ;) - -chris -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.7 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGYGY39CaO5/Lv0PARAm9iAJ0cYAW0Rs6h5yfVwefQkvPcMnUmPgCgjnkV IG5pXk8OVJY+44SHv+mr/i0= =9F0i -END PGP SIGNATURE- - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: macros - max recursion depth
On 6/1/07, Christopher Schultz [EMAIL PROTECTED] wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Nathan, Nathan Bubna wrote: 2. Throw another exception (MacroDepthExceededException?) The way I see it, neither of these options is any better than simply allowing the stack overflow to occur. Stack overflows can be caused by many things. Throwing a MacroDepthException is much more informative, and in the case of 3rd party templates being introduced to a running system, can prevent DOS type stuff. Yeah... as I was typing that question, I was thinking well, stack overflow could mean many things, although I immediately assume that my template has infinite recursion in these cases ;) :) I hasn't really thought about 3rd-party templates. Does anyone have any data on the impact of a stack overflow on a running app server? I would imagine that a better way to perform a DOS would be to concatenate strings forever in an endless loop. There's really no checking that can be done against that. still plugging what holes we can ain't a bad thing :) Okay. Enough nay-saying from me ;) - -chris -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.7 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGYGY39CaO5/Lv0PARAm9iAJ0cYAW0Rs6h5yfVwefQkvPcMnUmPgCgjnkV IG5pXk8OVJY+44SHv+mr/i0= =9F0i -END PGP SIGNATURE- - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: macros - max recursion depth
Great question from Claude. Yes, I think this is an untrusted 3rd party template issue. (or perhaps just a defense against sloppy template design). Looking at the original issue (VELOCITY-297), the stated intent is to avoid StackOverflowExceptions. I see twin objectives here of of providing a more meaningful exception and to ensure that recursive macro calls have consistent behavior regardless of JVM settings and internal method structure. In otherwords, I prefer to see a Velocity related exception after 20 macro calls, rather than a stack overflow exception after umpteen method calls. As a corollary to this issue, maybe the exception message can contain a list of macro calls? Something like: org.apache.velocity.exception MacroOverflowException: message: Exceed maximum 20 macro calls. Call stack: macro1 - macro2 - macro3 - macro1 - macro1 - macro1 (obviously, with 20 calls in the list above) To implement this, you'd have to track the stack of macro calls during page rendering. WILL On 6/1/07, Christopher Schultz [EMAIL PROTECTED] wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Nathan, Nathan Bubna wrote: 2. Throw another exception (MacroDepthExceededException?) The way I see it, neither of these options is any better than simply allowing the stack overflow to occur. Stack overflows can be caused by many things. Throwing a MacroDepthException is much more informative, and in the case of 3rd party templates being introduced to a running system, can prevent DOS type stuff. Yeah... as I was typing that question, I was thinking well, stack overflow could mean many things, although I immediately assume that my template has infinite recursion in these cases ;) I hasn't really thought about 3rd-party templates. Does anyone have any data on the impact of a stack overflow on a running app server? I would imagine that a better way to perform a DOS would be to concatenate strings forever in an endless loop. There's really no checking that can be done against that. Okay. Enough nay-saying from me ;) - -chris -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.7 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFGYGY39CaO5/Lv0PARAm9iAJ0cYAW0Rs6h5yfVwefQkvPcMnUmPgCgjnkV IG5pXk8OVJY+44SHv+mr/i0= =9F0i -END PGP SIGNATURE- - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- Forio Business Simulations Will Glass-Husain [EMAIL PROTECTED] www.forio.com
macros - max recursion depth
(changing the subject line for better tracking) What are your preliminary thoughts on this -- how are you thinking of tracking the recursion level for each macro? WILL On 5/31/07, Supun Kamburugamuva [EMAIL PROTECTED] wrote: My suggestion is to run with the macro max recursion depth issue while we argue this out. I'm in the process of implementing max recursion depth issue. Supun. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED] -- Forio Business Simulations Will Glass-Husain [EMAIL PROTECTED] www.forio.com
