We use this as a QA tool to detect references that may not have been properly escaped or otherwise marked as safe. It does not run in a production environment, but does run in Selenium environment which is also under load. Given the volume of requests we always try to avoid allocating unnecessary objects.
Unfortunately the Context is the only place where we can get location information for violation messages (template stack, macro stack, etc.) IMO it would be much simple to just pass the Context to all event handlers as one more standard parameter. People who don't Context parameter will not use it. It would solve the threading issue and avoid having "Handler instanceof ContextAware" checks at the same time. It seems like there is a some unnecessary work being done initializing event cartridges, etc. and all of that just for the purpose of setting RuntimeServices and Context references on them. As far as creation of executors: I understand that it was convenient to create the method "iterateOverEventHandlers" and apply it for event handlers with different parameters, but the cost of that is creation of the new executor instance for every invocation. It would be more efficient to just have a utility that returns a combined list of handlers (if needed) instead of creating one iterator for each list (application and context). Then the callback invocation code could just walk the list and call the handlers. We have run into some other inefficient places. For example ASTStringLiteral is buffering the entire content in the StringWriter. It does not work very well for large templates. That code creates a writer which buffers up the whole thing, then does a toString() on it creating another copy. Then in some cases calls substring() that creates yet another copy. I can dig up a few other things we fixed in our version if you guys are interested. Alex On Mon, Nov 28, 2016 at 11:48 PM, Claude Brisson <cla...@renegat.net> wrote: > Hi Alex. > > Thanks for your feedback. > > On 28/11/2016 20:52, Alex Fedotov wrote: > > Hello All, > > ContextAware interface in Velocity is not really workable in a > multi-threaded environment. > The handler is a singleton and setting the Context on the singleton from > multiple threads is not going to work well, short of using some form of > thread local. > > > You are right about that. The ContextAware javadoc says: > > Important Note: Only local event handlers attached to the context (as > opposed to global event handlers initialized in the velocity.properties > file) should implement ContextAware. Since global event handlers are > singletons individual requests will not be able to count on the correct > context being loaded before a request. > > I agree that the site documentation could be clearer about it. > > Also the event handlers (such as insertion handler) are creating new > instance of executor for each inserted value which is inefficient. > > I'm not sure that using a reference insertion event handler in the first > place can be made efficient, and not sure either that the > allocation/deallocation accounts for a lot of performance loss since modern > JVMs do a really good job about allocation of small Java objects. > > The reference insertion event handler was initially meant to be a > debugging and checking tool rather than a production tool. This being said, > I known there can be some legitimate use cases to do it in production, but > can I ask what is your specific use case, out of simple curiosity? > > > Are you guys fixing any of this for 2.0? > > > It is not planned to fix the issue for 2.0, but you can easily implement a > custom implementation that will use a thread local member. > > The event management may be reviewed in a future version, but if you think > this particular point should specifically be addressed, I strongly advise > you to open an issue about it. > > > Claude > >
