[
https://issues.apache.org/jira/browse/VELOCITY-669?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12665565#action_12665565
]
Byron Foster commented on VELOCITY-669:
---------------------------------------
<qoute>
For instance in Parser.jjt you added that macroNames HashMap. Now
escapedDirective method does not ask RuntimeServices anymore if some string is
a macro or not. Instead it takes a look at macroNames. I think this is not the
same thing since it is not guaranteed that the Parser instance knows about all
macros.
</qoute>
That's true, I've changed it so that it work exactly as it did before, so that
escapedDirectives includes globals. On a side note however, this whole
mechanism supports the escaping of macros which is fundamentally broken as
discussed in VELOCITY-454 and VELOCITY-670 (And in 670 I only outline one of
the cases). And, I plan to address.
<qoute>
You also removed Macro.processAndRegister and changed so that macro is
registered late at init and it feels a bit out of place. This may have some
side effects (unnecessary reparsing of macros etc)?
</qoute>
I think that registering the macro in init, right after it is initialized is
exactly right. I think there are three clear phases to a template: Parse,
Init, Render. As it stands now, these three phases are cleanly delineated such
that no phase overlaps into another. Further, both Parse and Init are done
only once, with one thread, and initialization is done in a predictable depth
first traversal of the entire AST tree instead of initializing fragments of the
tree in some other order. I think this delineation contributes to the overall
maintainability of the code. Before these changes initialization was being
done during Render, and it was being done more then once, and you could have
potentially multiple threads in an init method.
<qoute>
Is it certain that the Node argument passed to init is the same argument
passed to processAndRegister (the macro body?).
</qoute>
As far as I can tell.
<qoute>
I tested that by simply adding "nodeTree.init(null, rsvc);" to the last line of
VelocimacroManager.MacroEntry constructor fixes this bug. Context can be null
for init calls since init isn't (and should not be) context dependent. The only
AST element that refers Context is ASTStringLiteral which can be changed to
call super.getTemplateName() instead of getting it from the Context.
</qoute>
Even though I do agree with your premiss that initialization should not be
context dependent, I don't believe this will work. Not just the SimpleNodes
are initialized with the context, but also Directives. Just taking a cursory
glance I see that BlockMacro and Define use the context. I did not look at the
other Directives. This does not include user defined directives which may or
may not assume that the context can be null.
Calling init from MacroEntry results in initialization taking place during
Parse, and creates the situation in which macros are initialized at least
twice, and maybe more. Referring back to the points I made about delineating
the phases I think the solution as it stands is cleaner.
> Concurrency bug introduced in 1.7-dev
> -------------------------------------
>
> Key: VELOCITY-669
> URL: https://issues.apache.org/jira/browse/VELOCITY-669
> Project: Velocity
> Issue Type: Bug
> Affects Versions: 1.7
> Reporter: Jarkko Viinamäki
> Priority: Blocker
>
> Warning: current SVN head is broken - it fails under heavy load. I don't have
> time to investigate right now but 1.6.1 release version does not have this
> bug (1.6.2 release candidate may have it! - I'm not sure what's included
> there). However, current SVN head fails consistently with my load testing
> suite when I run it under JRat profiling:
> -------------------------------------------------------------------------------
> Test set: org.apache.velocity.test.load.Velocity24LoadTest
> -------------------------------------------------------------------------------
> Tests run: 250, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 71.032 sec
> <<< FAILURE!
> testRendering(org.apache.velocity.test.load.Velocity24Test) Time elapsed:
> 0.125 sec <<< ERROR!
> java.lang.NullPointerException
> at java.io.Writer.write(Writer.java:110)
> at
> org.apache.velocity.runtime.parser.node.ASTText.render_$jrat(ASTText.java:83)
> at org.apache.velocity.runtime.parser.node.ASTText.render(ASTText.java)
> at
> org.apache.velocity.runtime.parser.node.ASTBlock.render_$jrat(ASTBlock.java:72)
> at
> org.apache.velocity.runtime.parser.node.ASTBlock.render(ASTBlock.java)
> at
> org.apache.velocity.runtime.directive.VelocimacroProxy.render_$jrat(VelocimacroProxy.java:222)
> at
> org.apache.velocity.runtime.directive.VelocimacroProxy.render(VelocimacroProxy.java)
> at
> org.apache.velocity.runtime.directive.RuntimeMacro.render_$jrat(RuntimeMacro.java:295)
> at
> org.apache.velocity.runtime.directive.RuntimeMacro.render(RuntimeMacro.java)
> at
> org.apache.velocity.runtime.directive.RuntimeMacro.render_$jrat(RuntimeMacro.java:215)
> at
> org.apache.velocity.runtime.directive.RuntimeMacro.render(RuntimeMacro.java)
> at
> org.apache.velocity.runtime.parser.node.ASTDirective.render_$jrat(ASTDirective.java:198)
> at
> org.apache.velocity.runtime.parser.node.ASTDirective.render(ASTDirective.java)
> at
> org.apache.velocity.runtime.parser.node.SimpleNode.render_$jrat(SimpleNode.java:342)
> at
> org.apache.velocity.runtime.parser.node.SimpleNode.render(SimpleNode.java)
> at org.apache.velocity.Template.merge_$jrat(Template.java:340)
> at org.apache.velocity.Template.merge(Template.java)
> at org.apache.velocity.Template.merge_$jrat(Template.java:248)
> at org.apache.velocity.Template.merge(Template.java)
> at
> org.apache.velocity.test.load.Velocity24Test.testRendering_$jrat(Velocity24Test.java:52)
> at
> org.apache.velocity.test.load.Velocity24Test.testRendering(Velocity24Test.java)
> Although I'm not sure, I strongly suspect that this has got something to do
> with refactoring done (2009-01-11) for VelocimacroProxy.init. In 1.6.1 the
> init function initializes the nodeTree variable but if I'm not mistaken, in
> current SVN head this does not happen(!). It seems that under certain
> conditions the engine tries to render an ASTText node that has not been
> initialized (char array is null) which causes this exception.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
